Opened 4 years ago

Last modified 2 years ago

#1737 reviewing defect

ABP 1.3 for IE sometimes blocks navigation to the ad from Google search results

Reported by: mapx Assignee: Mailkov
Priority: P3 Milestone:
Module: Adblock-Plus-for-Internet-Explorer Keywords:
Cc: oleksandr, sergz, erikcramer Blocked By:
Blocking: Platform: Internet Explorer
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29319002/

Description (last modified by sergz)

Environment

win 7, IE 10, ABP 1.3
win 8.1, IE 11, ABP 1.4

How to reproduce

  1. Make sure Ad Block 1.3 is Enabled on http://www.google.com and Acceptable Ads are enabled.
  2. Enter search term that leads to ads.
  3. Right click on the displayed ad and select Open in New Tab
  4. Observe the site advertised opening fine
  5. Disable Ad Block 1.3 on http://www.google.com.
  6. Enter search term that leads to ads.
  7. Right click on ads and select Open in New Tab.
  8. Observe that sometimes links from ads are only opening blank pages.

Looks like sometimes the google ad leads to googleadservices.com, which then redirects to the actual website advertised. Other times it leads to just an empty page with the URL google.com.

from
https://adblockplus.org/forum/viewtopic.php?f=16&t=26978

Observed

The newly opened tab is a blank page.

Expected

The correctly filled page as we don't block root page.

The record from log

Blocked SUBDOCUMENT - http://www.google.de/aclk?sa=l&ai=Cfvc2X_aHVYTTDMbi7gbksZCoCLz9uLQH...%26lpid%3D%26aceid%3D%26hkz%3D

Change History (18)

comment:1 Changed 4 years ago by mapx

  • Description modified (diff)

I tested the issue (reported in the forum) on IE 11, and I can reproduce it partially: the links from the normal search can be opened in a new tab thru right click.

comment:2 Changed 4 years ago by mapx

  • Summary changed from ABP 1.3 for IE - strange behaviour when "disable on..." to ABP 1.3 for IE - wrong behaviour when "disable on..."

comment:3 Changed 4 years ago by mapx

  • Cc sergz added

comment:4 Changed 4 years ago by oleksandr

  • Priority changed from Unknown to P3
  • Ready set

comment:5 Changed 4 years ago by Mailkov

I would like to work on this defect.

comment:6 Changed 4 years ago by oleksandr

  • Owner set to Mailkov

Sounds great, thanks! Can you reproduce the bug for all links or only for ad links? I can reproduce it with v 1.4, but not with the current tip.

comment:7 Changed 4 years ago by Mailkov

Hi, I'm back !!!
I tested it both v.1.4 and current tip ... The disable function seems to not work .
I'm investigating on it.

comment:9 Changed 4 years ago by philll

  • Ready unset
  • Verified working unset

Could someone please clean up this issue with a proper observed and expected behaviour section? Unset ready until this is done and it's fully reproducible.

comment:10 Changed 4 years ago by oleksandr

  • Description modified (diff)
  • Ready set

comment:11 Changed 4 years ago by oleksandr

  • Summary changed from ABP 1.3 for IE - wrong behaviour when "disable on..." to ABP 1.3 for IE sometime blocks navigation to the ad from Google search results

comment:12 Changed 4 years ago by oleksandr

  • Summary changed from ABP 1.3 for IE sometime blocks navigation to the ad from Google search results to ABP 1.3 for IE sometimes blocks navigation to the ad from Google search results

comment:13 Changed 3 years ago by sergz

  • Description modified (diff)

comment:14 follow-up: Changed 3 years ago by sergz

The request of the newly opened tab is being executed in a new process and in WBPassthruSink::BeginningTransaction tab is nullptr, m_boundDomain is empty and accept header is text/html, application/xhtml+xml, */*, so the "content type" is subdocument. (We have such configurations for valid to block page parts.) But the exception rule is @@||www.google.de^$document, as the result it's not whitelisted. In general the trouble is that we cannot say that it's "document" instead of "subdocument". The solution is to check the current number of tabs, if it's zero then assume that it's a document.

comment:15 Changed 3 years ago by sergz

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:16 in reply to: ↑ 14 ; follow-up: Changed 3 years ago by Mailkov

Replying to sergz:

The solution is to check the current number of tabs, if it's zero then assume that it's a document.

Can you explain better?
Why when is number of tabs zero we can assume that it's a 'document'?
Tabs in this situation is never zero in my opinion ...

comment:17 in reply to: ↑ 16 Changed 3 years ago by sergz

  • Tester set to Unknown

Replying to Mailkov:

Replying to sergz:

The solution is to check the current number of tabs, if it's zero then assume that it's a document.

Can you explain better?
Why when is number of tabs zero we can assume that it's a 'document'?
Tabs in this situation is never zero in my opinion ...

According to blogs.msdn.com/b/askie/archive/2009/03/09/opening-a-new-tab-may-launch-a-new-process-with-internet-explorer-8-0.aspx

In general, new processes are created until the TabProcGrowth number is met, and then tabs are load balanced across the tab processes.

So, the idea is that if there is no referrer and there is only one tab then we can assume that it's a first request of the page, thus document.

We are currently adding the current instance into CPluginClass::s_threadinstances when the tab state is changed which on practice happens after the downloading of a new tab page is initiated. Beside apparent race condition there is a number of tabs starting from which it does not work.
See also comments in review https://codereview.adblockplus.org/29319002/.

comment:18 Changed 2 years ago by fred

  • Cc erikcramer added
Note: See TracTickets for help on using tickets.