Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#3651 closed defect (fixed)

Popup blocking misses some redirects and bails out if opener is a dynamically created document

Reported by: sebastian Assignee: sebastian
Priority: P3 Milestone: Adblock-Plus-1.11-for-Chrome-Opera-Safari
Module: Platform Keywords:
Cc: arthur, Ross Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29336278

Description (last modified by sebastian)

Background

#3568 showed up an issue where popup blocking isn't working in Adblock Plus for Firefox when the popup is opened by a programmatically created document. This issue also effects Adblock Plus for Chrome. The cause there, however, is slightly different. Dynamically created documents aren't reported to the webRequest API. Therefore these frames are missing in the frame hierarchy recorded by Adblock Plus, and as a result the popup blocking code bails out.

Moreover, we rely on chrome.tabs.onUpdated to detect when the URL of the popup changes while still loading. This API however only indicates changes visible in the address bar. But in case of multiple redirects, Chrome doesn't update the UI for every URL in the redirect chain. Hence the popup blocking code misses some of the intermediate URLs which might trigger a popup blocking filter.

What to change

  • Call chrome.webNavigation.getFrame (recursively) to discover the frame hierarchy during popup blocking, rather than relying on the frame hierarchy recorded by the webrequests code.
  • Use chrome.webRequest.onBeforeRequest (instead of chrome.tabs.onUpdated) to detect changes to the URL while the popup is loading.

Expected behaviour

Popups that are either openend by a dynamically created window and/or use a lot of redirects where one of the intermediate URLs is covered by a popup blocking filter, should be automatically closed. All requests should be indicated in the devtools panel. See #3568 for an example.

Change History (8)

comment:1 Changed 4 years ago by sebastian

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

comment:2 Changed 4 years ago by sebastian

  • Description modified (diff)

comment:3 Changed 4 years ago by sebastian

  • Milestone set to Adblock-Plus-for-Chrome-Opera-Safari-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:4 Changed 4 years ago by Ross

The example site in #3568 doesn't behave as it did before and seems to open one popup to (what I'm guessing) was a redirect but is now an error page. A) Should this error popup be blocked or doesn't it match a filter? (I tried to check) B) Is there another example URL with dynamically created windows?

comment:5 Changed 4 years ago by sebastian

  • Cc arthur Ross added

It seems that that website changed their behavior. From what I can tell Adblock Plus behaves correctly now, but there simply isn't a filter blocking the new popup. That behavior is also consistent with Adblock Plus for Firefox.

comment:6 Changed 4 years ago by arthur

Checked it with this page: http://neodrive.co/embed/QDHS20ASC0DUDU4YDWG3J6EEQ

From what I can see it's working as expected. I think it makes sense to add an additional filter like ||fabolele.com^$popup,third-party though.

comment:7 Changed 4 years ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Thanks Arthur. Works as expected except in Opera 19.

ABP 1.10.2.1559
Chrome 30 / 35 / 48 / Windows 7 x64
Chrome 43 / OS X 10.11
Chrome 48 / Ubuntu 14.04 x64
Opera 23 / 27 / Windows 7 x64
Opera 28 / OS X 10.11
Opera 35 / Ubuntu 14.04 x64

comment:8 Changed 4 years ago by sebastian

Assuming that popup blocking generally works in Opera 19, just not in this corner case, and that we didn't introduce a new regression, I think we can ignore the issue, there.

Note: See TracTickets for help on using tickets.