Handle requests that aren't associated with a browser tab

[sebastian]

Background

Adblock Plus is ​ignoring any request that isn't associated with any tab, as indicated with a tabId of -1. We did that in order to simplify the code path, by eliminating the scenario in which no context information are available, assuming that those requests aren't relevant anyway.

But now, there are two cases at least in which websites can cause requests that are not associated with the corresponding tab, potentially circumventing Adblock Plus:

• Requests sent with sendBeacon() that outlive the document (​Chrome bug 522129).
• Scripts running as a SharedWorkers and their sub-requests (​Chrome bug 705931).

Until these Chrome bugs are fixed, there is no way to block these requests based on the context, with filters like *$domain=example.com,websocket,ping. However, it would be possible to match those requests without context information, so that generic filters like ||example.com$websocket,ping work at least.

What to change

Remove the logic (in chrome/ext/background.js) that causes requests with tabId == -1 to be ignored, and adapt the logic down the code path in order to make the page/frame information optional.

Also consider the devtools panel (see lib/devtools.js), which logs requests to the active panel for the tab associated with the request. When the tabId is -1, however, it should log the request to all active panels.

Hints for testers

You can navigate to csp.kzar.co.uk and looking at the adblockplus devtools panel verifying that the shared worker connections are appearing in the list of requests. Additionally, adding the temporary websocket filter there should block the shared worker connections.

[kzar]

(Adding Wladimir since perhaps he has an opinion too.)

Interesting idea, definitely worth considering. I'm not sure it's a goodfirstbug, since while ostensibly trivial to implement, it's probably pretty hard to get the details correct. Testing might be a little tricky too.

What about a whitelisted website that indirectly creates a request through one of these methods? That could be incorrectly blocked right?

...adapt the logic down the code path in order to make the page/frame information optional.

I didn't look to carefully into it but I suspect that this part is not too trivial. I guess lib/popupblocker.js can simply ignore requests with a tabId of -1 but what about lib/requestBlocker.js?

Since the tabId is expected to be a unique identifier couldn't this cause the code to get confused if there are two of these requests with a tabId of -1? Perhaps we need to need to identify those in a different way somehow?

[sebastian]

Replying to kzar:

I'm not sure it's a goodfirstbug, since while ostensibly trivial to implement, it's probably pretty hard to get the details correct. Testing might be a little tricky too.

I don't see why this should keep an issue from being marked as goodfirstbug. The only important point is that the change itself is trivial enough, that somebody new to the codebase can realistically get around it. However, no changes are done unsupervised, as there is code review and QA. So I don't share your concerns. I'm more concerned to have nothing for new developers and contributors to work on, in order to get started. Moreover, removing dead code and text changes alone (where you seem to see the bar), aren't sufficient to improve your understanding of the codebase or to keep you interested.

What about a whitelisted website that indirectly creates a request through one of these methods? That could be incorrectly blocked right?

Yes, requests that aren't associated with any document, aren't subject to document-based whitelisting. This doesn't seem any less accurate, however, as completely ignoring those requests, technically one could even argue that this is the expected behavior. Also considering the cases outlined above in which such requests occur, it doesn't seem to cause to much of a problem. If necessary those requests can still be whitelisted individually.

I didn't look to carefully into it but I suspect that this part is not too trivial. I guess lib/popupblocker.js can simply ignore requests with a tabId of -1

It seems that lib/popupBlocker.js is completely ignoring the possibility of sourceTabId being -1, which might not be possible after all. Anyway, the code there isn't affected by the changes suggested in this issue, as it doesn't use ext.onBeforeRequest.

but what about lib/requestBlocker.js?

Yes, the ext.onBeforeRequest handler in lib/requestBlocker.js must be updated as you adapt the logic down the code path. (The purpose of the "What to change" section is not to point to every single line of code that needs to be changed, otherwise you could just write a patch instead of writing the issue.)

Since the tabId is expected to be a unique identifier couldn't this cause the code to get confused if there are two of these requests with a tabId of -1?

It would cause problems if we'd create Page and Frame objects, and treat -1 as a regular tabId, but that is not the idea. Instead, the page and frame passed to the ext.onBeforeRequest listener should be null, which then just calls defaultMatcher.matchesAny() with an empty documentHost and no sitekey. No other code path needs to be aware of this scenario (except for the devtools panel).

[greiner]

When the tabId is -1, however, it should log the request to all active panels.

Wouldn't that cause quite a bit of confusion? At least we could visually indicate that those requests didn't originate from the inspected tab.

[sebastian]

They will be visually (and technically) distinguished by having an empty Document domain. But yeah, if we could also render them differently in the devtools panel, perhaps with an icon showing a tooltip explaining that these requests aren't necessarily related to the current tab, that would be awesome. Do you want to do this? It would have to be done in adblockplusui, independently of the change here, anyway. :)

[greiner]

Replying to sebastian:

They will be visually (and technically) distinguished by having an empty Document domain. But yeah, if we could also render them differently in the devtools panel, perhaps with an icon showing a tooltip explaining that these requests aren't necessarily related to the current tab, that would be awesome. Do you want to do this? It would have to be done in adblockplusui, independently of the change here, anyway. :)

I created #5093 so that it can be worked on independently. We need some feedback from the Product team on that though.
But given that those entries at least won't have a domain, it doesn't seem necessary to block it on #5093 so no objections from me against going ahead with this in parallel.

[abpbot]

A commit referencing this issue has landed:
​Issue 5042 - Adds handling for requests which are not associated with browser tab

[Ross]

Working as described.

ABP 1.13.2.1785
Chrome 49 / 58 / Windows 7
Opera 36 / 45 / Windows 7