Requests blocked in whitelisted frame

[greiner]

Environment

Ubuntu 16.04
Chrome 71
Adblock Plus 3.4.3 / 3.4.2

How to reproduce

1. Add the custom filters /ad.js^ and @@/sub.htm$document
2. Create top.htm, sub.htm and ad.htm and launch static web server (see file contents below)
3. Open top.htm (e.g. ​http://localhost:8080/top.htm)

Example file contents

top.htm

<iframe src="sub.htm" height="500"></iframe>

sub.htm

<strong>With srcdoc</strong>
<iframe srcdoc=""></iframe>

<strong>Without srcdoc</strong>
<iframe></iframe>

<script>
let iframes = document.querySelectorAll("iframe");
for (let iframe of iframes) {
  iframe.contentDocument.write(`
    <script src="ad.js"></scr` + `ipt>
  `);
}
</script>

ad.js

document.write("ADVERTISEMENT");

Observed behavior

• Frame under heading "With srcdoc" is empty
• Frame under heading "Without srcdoc" shows text "ADVERTISEMENT"
• Request to ad.js is shown in DevTools as being blocked by filter /ad.js^

Expected behavior

• Both frames show text "ADVERTISEMENT"
• Request to ad.js is not shown in DevTools as being blocked

What to change

When encountering a request for which ext.getFrame() returns undefined but which specifies both a frameId and parentFrameId property, create a new about:blank frame for the one the request originates from and add it to the frame hierarchy.

For example, this could be achieved by extending ext.getFrame().

Further information

Chrome doesn't notify us of the anonymous frame with the srcdoc attribute. Therefore the extension doesn't know which frame the ad.htm request originated from. Due to that the exception rule is not applied and the request gets blocked. ​See Chromium issue 937264.

Hints for testers

Any time we touch the page-frame hierarchy code, it's possible to cause regressions. There's a lot of complexity, and subtleties with timing etc which are hard to get right. We need to thoroughly test blocking, element hiding and whitelisting. Things like $sitekey whitelisting, adverts in nested frames, different browsers + versions, that we didn't reintroduce previously fixed bugs in this code (#7065, #6953, #6808, #6746, #6692, #6595, #6191 etc).

[greiner]

One more thing to add is that I'm not quite sure why the browser doesn't notify us of certain anonymous frames. My best bet would be the usage of document.open("text/html", "replace") which results in ​no history entry being created, possibly in combination with other similar bad practices by any of the code involved in serving/rendering the ad.

[kzar]

Would you mind adding some explanation to the issue description of what this example page is doing and how come we don't handle it well so far?

Also, could you explain a bit more what your proposed change involves? From your example results it seems like you're creating frame Objects for the frame's parent frame ID as well?

Is this an issue in Firefox and Edge?

[greiner]

Would you mind adding some explanation to the issue description of what this example page is doing and how come we don't handle it well so far?

I added a bit more context regarding the example page but it is not clear yet what causes the issue so there's not much I was able to add there. Considering that this is a fallback solution for any situation for which the browser doesn't notify us of a frame being created, I considered it not being necessary to find out what exactly causes it since that would be merely an optimization that could be worked on separately (i.e. the more frames we can detect early on outside of the critical path, the fewer cases there are for which we need to fall back on adding frames while on the critical path).

However, I can understand if you disagree with this assessment.

Also, could you explain a bit more what your proposed change involves? From your example results it seems like you're creating frame Objects for the frame's parent frame ID as well?

My suggested approach is only creating a frame object for the frame the request originated from, not for its parent since we don't know what its parent's parentId is. I've tried to clarify that bit now in the description.

I assume your concern is that this approach cannot deal with a situation where a request is coming from an unknown frame whose parent is also unknown. I'd consider that to be a valid argument, I just don't know how we could handle such cases considering that all we know of the parent frame is its ID.

Is this an issue in Firefox and Edge?

I haven't tested it on Edge but it also affects Firefox.

[kzar]

I haven't tested it on Edge but it also affects Firefox.

Which version of Firefox did you test with?

[greiner]

I have now tried again to reproduce it on Firefox but was unable to so I updated the description to reflect that.

[kzar]

I failed to reproduce this last night on Chrome 72, but apparently the site's behaviour has changed a bit. Some tips about reproducing the bug from Thomas in IRC:

I noticed that the error pattern also changed so essentially, if you
see anything blocked or hidden after the pxext.gif got blocked,
something's wrong (or if you never see an ad after a dozen or so
reloads)

[greiner]

I've invested a few hours into finding out exactly under what circumstances Chrome doesn't notify us of a frame. I found the culprit, created a minimal example and replaced the reproduction steps in the description with it.

[greiner]

@Ross What about adding a test case for this to testpages.adblockplus.org?

[sebastian]

Can we please file a Chrome bug for this?

Is there a workaround that doesn't give less accurate results in other scenarios?

Please add to the issue description whether this is a regression or not. If it isn't a regression, but we have an uncontroversial fix ready to land before we are (almost) through testing I wouldn't mind including this in 3.5. But otherwise, I'm afraid it would have to wait for a future release.

As far as AAX is concerned, I highly recommend moving away from $sitekey-based whitelisting in the first place. I advised against that approach from the beginning, but now with manifest V3 and the related limitations coming up, future versions of Adblock Plus will most likely no longer be able to support the $sitekey filter option. While at it perhaps they can also avoid this dance.

[kzar]

Nice work getting to the bottom of this Thomas! I'm impressed.

Can we please file a Chrome bug for this?

I agree, definitely worth filing an issue from the sounds of it.

But otherwise, I'm afraid it would have to wait for a future release.

I strongly feel that including any (non-trivial) change relating to this page-frame structure code into the upcoming 3.5 release is a bad idea. A fix for one edge case leads to a regression of another edge case. It's happened time and time again.

[kzar]

I'm marking this as Ready. That said, while the proposed workaround sounds logical, I'd want to see it in practice before being sure it's a good idea.

[greiner]

Can we please file a Chrome bug for this?

Sure, we just need to create a more appropriate example page for them that doesn't rely on Adblock Plus being installed unless you think that it's already sufficient as is.

Is there a workaround that doesn't give less accurate results in other scenarios?

I don't know yet and as I mentioned before, I'd recommend any such workaround to be done on top of the fallback I'm proposing to reduce its inherent negative impact on performance.

It's also worth mentioning, that other than the generic fallback, I haven't found a specific workaround for this issue yet so this would require further research and could either be done together with this change or in a follow-up release.

Please add to the issue description whether this is a regression or not.

I'll check that, no problem. While this issue is causing significant problems, I also understand the risk of such late inclusions in an ongoing release so I'll leave it completely up to you guys to decide in which release you think such a fix should land.

As far as AAX is concerned, I highly recommend moving away from $sitekey-based whitelisting in the first place.

This particular issue has nothing to do with $sitekey but sure, we should let the AAX team know about this deficiency so that they can prepare for it.

Nice work getting to the bottom of this Thomas! I'm impressed.

Thanks. :)

[kzar]

Replying to greiner:

Can we please file a Chrome bug for this?

Sure, we just need to create a more appropriate example page for them that doesn't rely on Adblock Plus being installed unless you think that it's already sufficient as is.

Yea, that's probably a good idea. I usually create a minimal extension too which logs the events, or otherwise demonstrates the problem.

[greiner]

For reference, I've now also reported this issue to Chromium (see ​bug 937264).

[greiner]

The same issue occurs using Adblock Plus 3.4.2 so this is not a regression.

[greiner]

Interestingly, the fallback I proposed doesn't actually work with the example I provided because "sub_frame" requests are a special case due to the fact that their frameId refers to the new frame rather than the frame the request originated from. And since the suggested fallback only works to fill-in a single missing frame, it can't properly add the frame to the hierarchy but instead attaches it to the top frame.

I could replace <iframe src="ad.htm"></iframe> in the example with <script src="ad.js"></script> to reduce the scope of this ticket and thereby match the capabilities of the suggested change.

[kzar]

Replying to greiner:

Interestingly, the fallback I proposed doesn't actually work with the example I provided because "sub_frame" requests are a special case due to the fact that their frameId refers to the new frame rather than the frame the request originated from. And since the suggested fallback only works to fill-in a single missing frame, it can't properly add the frame to the hierarchy but instead attaches it to the top frame.

I'm curious, do you see any of the webNavigation or webRequest events which are firing for these troublesome frames? Perhaps we could add yet another listener for this case?

[greiner]

IIRC that's the first thing I checked. Also, it sounds like the browser generally doesn't know that a frame has been created, ​according to this comment that, as the origin of the regression, points to ​this commit which mentions in its commit message: "[srcdoc navigations] are no longer sent to the browser, as they make no netwrok request."

Should I check it again to make sure?

Edit: Note that we do receive the information for requests for those specific frames, it's just that we don't know about their parent frame (i.e. the one with a "srcdoc" attribute) so we don't know who the frame's grandparent frame is since all we know is the frame's own ID and its parent's ID.

[kzar]

Replying to greiner:

Should I check it again to make sure?

Well, you were already pretty thorough and I agree it sounds like a Chromium bug from what they said.

But would you mind modifying your test extension to log all the webNavigation(1) and webRequest events(2) instead of just onBeforeNavigate, onCommitted, onBeforeRequest and onHeadersReceived? It would be nice to quickly rule those out for a workaround, before going with your fallback approach.

1 - onBeforeNavigate, onDOMContentLoaded, onCompleted, onErrorOccurred,
onReferenceFragmentUpdated and onHistoryStateUpdated events.
2 - onBeforeRequest, onBeforeSendHeaders, onSendHeaders, onHeadersReceived, onAuthRequired, onResponseStarted, onBeforeRedirect, onCompleted and onErrorOccurred

[greiner]

Below's the output of each webNavigation and webRequest listener listed on developer.chrome.com (except webNavigation.onErrorOccurred, webNavigation.onTabReplaced and webRequest.onActionIgnored). With that I can confirm that none of them notify us of this frame (IDs 366 and 369 respectively in the below log).

You can find the extended test code attached as anon-frame-srcdoc-all.zip​.

With srcdoc

410 0 -1 "webNavigation.onBeforeNavigate" "http://localhost:8080/top.htm"
410 0 -1 "webRequest.onBeforeRequest" "http://localhost:8080/top.htm"
410 0 -1 "webRequest.onBeforeSendHeaders" "http://localhost:8080/top.htm"
410 0 -1 "webRequest.onSendHeaders" "http://localhost:8080/top.htm"
410 0 -1 "webRequest.onHeadersReceived" "http://localhost:8080/top.htm"
410 0 -1 "webRequest.onResponseStarted" "http://localhost:8080/top.htm"
410 0 -1 "webRequest.onCompleted" "http://localhost:8080/top.htm"
410 0 undefined "webNavigation.onCompleted" "http://localhost:8080/top.htm"
410 0 undefined "webNavigation.onCommitted" "http://localhost:8080/top.htm"
410 365 0 "webNavigation.onBeforeNavigate" "http://localhost:8080/first.htm"
410 0 undefined "webNavigation.onDOMContentLoaded" "http://localhost:8080/top.htm"
410 365 0 "webRequest.onBeforeRequest" "http://localhost:8080/first.htm"
410 365 0 "webRequest.onBeforeSendHeaders" "http://localhost:8080/first.htm"
410 365 0 "webRequest.onSendHeaders" "http://localhost:8080/first.htm"
410 365 0 "webRequest.onHeadersReceived" "http://localhost:8080/first.htm"
410 365 0 "webRequest.onResponseStarted" "http://localhost:8080/first.htm"
410 365 0 "webRequest.onCompleted" "http://localhost:8080/first.htm"
410 365 undefined "webNavigation.onCommitted" "http://localhost:8080/first.htm"
410 367 366 "webNavigation.onBeforeNavigate" "http://localhost:8080/second.htm"
410 365 undefined "webNavigation.onDOMContentLoaded" "http://localhost:8080/first.htm"
410 367 366 "webRequest.onBeforeRequest" "http://localhost:8080/second.htm"
410 367 366 "webRequest.onBeforeSendHeaders" "http://localhost:8080/second.htm"
410 367 366 "webRequest.onSendHeaders" "http://localhost:8080/second.htm"
410 367 366 "webRequest.onHeadersReceived" "http://localhost:8080/second.htm"
410 367 366 "webRequest.onResponseStarted" "http://localhost:8080/second.htm"
410 367 366 "webRequest.onCompleted" "http://localhost:8080/second.htm"
410 367 undefined "webNavigation.onCommitted" "http://localhost:8080/second.htm"
410 367 undefined "webNavigation.onDOMContentLoaded" "http://localhost:8080/second.htm"
410 367 undefined "webNavigation.onCompleted" "http://localhost:8080/second.htm"

Without srcdoc

410 0 -1 "webNavigation.onBeforeNavigate" "http://localhost:8080/top.htm"
410 0 -1 "webRequest.onBeforeRequest" "http://localhost:8080/top.htm"
410 0 -1 "webRequest.onBeforeSendHeaders" "http://localhost:8080/top.htm"
410 0 -1 "webRequest.onSendHeaders" "http://localhost:8080/top.htm"
410 0 -1 "webRequest.onHeadersReceived" "http://localhost:8080/top.htm"
410 0 -1 "webRequest.onResponseStarted" "http://localhost:8080/top.htm"
410 0 -1 "webRequest.onCompleted" "http://localhost:8080/top.htm"
410 0 undefined "webNavigation.onCompleted" "http://localhost:8080/top.htm"
410 0 undefined "webNavigation.onCommitted" "http://localhost:8080/top.htm"
410 368 0 "webNavigation.onBeforeNavigate" "http://localhost:8080/first.htm"
410 0 undefined "webNavigation.onDOMContentLoaded" "http://localhost:8080/top.htm"
410 368 0 "webRequest.onBeforeRequest" "http://localhost:8080/first.htm"
410 368 0 "webRequest.onBeforeSendHeaders" "http://localhost:8080/first.htm"
410 368 0 "webRequest.onSendHeaders" "http://localhost:8080/first.htm"
410 368 0 "webRequest.onHeadersReceived" "http://localhost:8080/first.htm"
410 368 0 "webRequest.onResponseStarted" "http://localhost:8080/first.htm"
410 368 0 "webRequest.onCompleted" "http://localhost:8080/first.htm"
410 368 undefined "webNavigation.onCommitted" "http://localhost:8080/first.htm"
410 369 368 "webNavigation.onBeforeNavigate" "about:blank"
410 369 undefined "webNavigation.onCommitted" "about:blank"
410 369 undefined "webNavigation.onDOMContentLoaded" "about:blank"
410 369 undefined "webNavigation.onCompleted" "about:blank"
410 370 369 "webNavigation.onBeforeNavigate" "http://localhost:8080/second.htm"
410 368 undefined "webNavigation.onDOMContentLoaded" "http://localhost:8080/first.htm"
410 370 369 "webRequest.onBeforeRequest" "http://localhost:8080/second.htm"
410 370 369 "webRequest.onBeforeSendHeaders" "http://localhost:8080/second.htm"
410 370 369 "webRequest.onSendHeaders" "http://localhost:8080/second.htm"
410 370 369 "webRequest.onHeadersReceived" "http://localhost:8080/second.htm"
410 370 369 "webRequest.onResponseStarted" "http://localhost:8080/second.htm"
410 370 369 "webRequest.onCompleted" "http://localhost:8080/second.htm"
410 370 undefined "webNavigation.onCommitted" "http://localhost:8080/second.htm"
410 370 undefined "webNavigation.onDOMContentLoaded" "http://localhost:8080/second.htm"
410 370 undefined "webNavigation.onCompleted" "http://localhost:8080/second.htm"

[kzar]

Replying to greiner:

With that I can confirm that none of them notify us of this frame (IDs 366 and 369 respectively in the below log).

Dang, thanks for trying. Looks like your fallback approach is about our only hope for a workaround for now.

[greiner]

I'll prepare the review for it then, thanks.

[greiner]

Do you happen to know of any way for me to test whether the fix for #6565 continues to work? Because I'm unable to reproduce the issue and the fix for it relies on us being able to determine whether we know the frame which is a bit trickier to do if ext.getFrame() always returns a frame.

It seems that Chrome has since then switched to requesting images using chrome-search:// URLs and manually initiating a download from the New Tab Page also doesn't produce the described behavior.

[kzar]

I'm not sure, but I do remember people complaining that it sometimes showed up when you used the "block element" tool as well. You could try that?

[greiner]

I got a naming conflict when forking the adblockpluschrome repo on GitLab since I already forked the UI fork under the same name. I've now created a private GitLab group so that I can create the fork so please let me know if there are any issues you're having with the merge request I created from it.

[greiner]

Replying to kzar:

I'm not sure, but I do remember people complaining that it sometimes showed up when you used the "block element" tool as well. You could try that?

Thanks for the hint. I'll try that.

Update: No success with that either after trying it on ten different domains.

[greiner]

See also #7336.

[abpbot]

A commit referencing this issue has landed:
​Issue 7305 - Update frame structure for requests from unknown frames

[abpbot]

A commit referencing this issue has landed:
​Issue 7305 - Update frame structure for requests from unknown frames

[kzar]

There is a regression with this code on Edge, I'm working on fixing that with Geo now. It shouldn't take too long.

[abpbot]

Some commits referencing this issue have landed:

• ​Issue 7305 - Avoid listing onBeforeRequest resource types manually
• ​Issue 7305 - Avoid listing onBeforeRequest resource types manually

[Ross]

This still seems to be an issue in Chrome. The first frame (with srcdoc) is still blocked, the second is not and both filters are displayed as applied in the devtools panel. It works as described in Firefox.

ABP 3.5.0.2283
Chrome 73.0.3683.86

[greiner]

Sorry, forgot to take action on the subframe edge case I mentioned, which this fix doesn't cover. I've now updated the ticket with a working test case and will create a follow-up ticket for the subframe case later today.

[Ross]

Thanks for the update! This looks fixed then.

ABP 3.5.0.2283
Chrome 73.0.3683.86 / Windows 10
Chrome 49.0.2623.75 / Windows 10
Firefox 65.0.1 / Windows 10
Firefox 51.0 / Windows 10
Opera 58.0.3135.118 / Windows 10
Opera 36.0.2130.80 / Windows 10
Edge 44.17763.1.0 / Windows 10
Firefox Mobile 65.0.1 / Android 7.1.1

[greiner]

Updated expected and observed behavior sections to reflect updated example code.

I've also created #7418 for the remaining issue regarding subframes.