Issue 6543 - Match requests without tabId/frameId in their originating context

Issue 6543 - Match requests without tabId/frameId in their originating context

[kzar, 2018-04-03 12:49:30]

https://codereview.adblockplus.org/29739594/diff/29739595/lib/requestBlocker.js
File lib/requestBlocker.js (right):

https://codereview.adblockplus.org/29739594/diff/29739595/lib/requestBlocker....
lib/requestBlocker.js:87: if (details.originUrl)
Nit: Please use braces since the body spans multiple lines. (same below)

https://codereview.adblockplus.org/29739594/diff/29739595/lib/requestBlocker....
lib/requestBlocker.js:96: return browser.tabs.query({}).then(tabs =>
Couldn't we pass something like `details.initiator + "*" ` for the url part of
the query? I figure we could avoid filtering the URLs manually later that way.

https://developer.chrome.com/extensions/tabs#method-query
https://developer.chrome.com/extensions/match_patterns

https://codereview.adblockplus.org/29739594/diff/29739595/lib/requestBlocker....
lib/requestBlocker.js:102: tabIds.push(tab.id);
Wouldn't this mean we sometimes log a request for a tab incorrectly, say if the
user has two tabs open for the same domain?

https://codereview.adblockplus.org/29739594/diff/29739595/lib/whitelisting.js
File lib/whitelisting.js (right):

https://codereview.adblockplus.org/29739594/diff/29739595/lib/whitelisting.js...
lib/whitelisting.js:81: return originUrl && match(page, originUrl, typeMask,
null,
How come you changed the logic to `originUrl && ...` here?

[Sebastian Noack, 2018-04-04 01:04:45]

https://codereview.adblockplus.org/29739594/diff/29739595/lib/requestBlocker.js
File lib/requestBlocker.js (right):

https://codereview.adblockplus.org/29739594/diff/29739595/lib/requestBlocker....
lib/requestBlocker.js:87: if (details.originUrl)
On 2018/04/03 12:49:30, kzar wrote:
> Nit: Please use braces since the body spans multiple lines. (same below)

No longer relevant (the block has only one line now, same below).

https://codereview.adblockplus.org/29739594/diff/29739595/lib/requestBlocker....
lib/requestBlocker.js:96: return browser.tabs.query({}).then(tabs =>
On 2018/04/03 12:49:30, kzar wrote:
> Couldn't we pass something like `details.initiator + "*" ` for the url part of
> the query? I figure we could avoid filtering the URLs manually later that way.
> 
> https://developer.chrome.com/extensions/tabs#method-query
> https://developer.chrome.com/extensions/match_patterns

In fact we can. I wasn't aware that tab.query() accepts URL patterns. Done.

https://codereview.adblockplus.org/29739594/diff/29739595/lib/requestBlocker....
lib/requestBlocker.js:102: tabIds.push(tab.id);
On 2018/04/03 12:49:30, kzar wrote:
> Wouldn't this mean we sometimes log a request for a tab incorrectly, say if
the
> user has two tabs open for the same domain?

This usually means that the request was sent by a Service Worker controlling
multiple tabs, in which case logging the request for all those tabs is correct.
Theoretically we can get false positives, e.g. if the request was sent by a
Service Worker controlling https://example.com/foo while there is another tab
for https://example.com/bar which isn't controlled by that Service Worker.
However, by default ServiceWorkers have a domain-wide scope and limiting the
scope requires to jump through some hoops. Also given the limited information,
Chrome provides us with, this seems to be the best we can do. Also FWIW,
currently, those requests are logged to all tabs. So we don't degrade here.

https://codereview.adblockplus.org/29739594/diff/29739595/lib/whitelisting.js
File lib/whitelisting.js (right):

https://codereview.adblockplus.org/29739594/diff/29739595/lib/whitelisting.js...
lib/whitelisting.js:81: return originUrl && match(page, originUrl, typeMask,
null,
On 2018/04/03 12:49:30, kzar wrote:
> How come you changed the logic to `originUrl && ...` here?

It's the same semantics for checkWhitelisted(), getKey() and
extractHostnameFromFrame(): We first try to retrieve information for the given
frame, falling back to its ancestors, and only if no information were found
(either because no frame was given in the first place or no frame in the
hierarchy was whitelisted), we fall back to the originUrl. In other words, we
give priority to information more local to the current document.

Therefore any whitelisting filter found in the frame hierarchy is now
immediately returned from inside the loop, and the code here is only reached in
case neither the frame nor any of its ancestors were whitelisted (or if no frame
was given in the first place), where we fall back to originUrl (if any).

[kzar, 2018-04-05 10:31:51]

Otherwise LGTM

https://codereview.adblockplus.org/29739594/diff/29739595/lib/requestBlocker.js
File lib/requestBlocker.js (right):

https://codereview.adblockplus.org/29739594/diff/29739595/lib/requestBlocker....
lib/requestBlocker.js:96: return browser.tabs.query({}).then(tabs =>
On 2018/04/04 01:04:45, Sebastian Noack wrote:
> On 2018/04/03 12:49:30, kzar wrote:
> > Couldn't we pass something like `details.initiator + "*" ` for the url part
of
> > the query? I figure we could avoid filtering the URLs manually later that
way.
> > 
> > https://developer.chrome.com/extensions/tabs#method-query
> > https://developer.chrome.com/extensions/match_patterns
> 
> In fact we can. I wasn't aware that tab.query() accepts URL patterns. Done.

Cool, I assumed I was wrong somehow :p. This looks much cleaner now.

https://codereview.adblockplus.org/29739594/diff/29739595/lib/requestBlocker....
lib/requestBlocker.js:102: tabIds.push(tab.id);
On 2018/04/04 01:04:45, Sebastian Noack wrote:
> On 2018/04/03 12:49:30, kzar wrote:
> > Wouldn't this mean we sometimes log a request for a tab incorrectly, say if
> the
> > user has two tabs open for the same domain?
> 
> This usually means that the request was sent by a Service Worker controlling
> multiple tabs, in which case logging the request for all those tabs is
correct.
> Theoretically we can get false positives, e.g. if the request was sent by a
> Service Worker controlling https://example.com/foo while there is another tab
> for https://example.com/bar which isn't controlled by that Service Worker.
> However, by default ServiceWorkers have a domain-wide scope and limiting the
> scope requires to jump through some hoops. Also given the limited information,
> Chrome provides us with, this seems to be the best we can do. Also FWIW,
> currently, those requests are logged to all tabs. So we don't degrade here.

Acknowledged.

https://codereview.adblockplus.org/29739594/diff/29741715/lib/requestBlocker.js
File lib/requestBlocker.js (right):

https://codereview.adblockplus.org/29739594/diff/29741715/lib/requestBlocker....
lib/requestBlocker.js:94: return Promise.resolve([]);
I wonder if there is a situation where details.initiator isn't provided either,
in which case we will no longer log those requests / hits. Problem is it's not
documented too well. Perhaps do some testing (if you didn't already) to confirm?

[Sebastian Noack, 2018-04-05 16:56:33]

https://codereview.adblockplus.org/29739594/diff/29741715/lib/requestBlocker.js
File lib/requestBlocker.js (right):

https://codereview.adblockplus.org/29739594/diff/29741715/lib/requestBlocker....
lib/requestBlocker.js:94: return Promise.resolve([]);
On 2018/04/05 10:31:51, kzar wrote:
> I wonder if there is a situation where details.initiator isn't provided
either,
> in which case we will no longer log those requests / hits. Problem is it's not
> documented too well. Perhaps do some testing (if you didn't already) to
confirm?

There are two scenarios (I have seen in my testing) where this code path is hit
(i.e. there is neither originUrl nor initiator):

* Always on Chromium <63 (same for Microsoft Edge, which however also
  doesn't support devtools panels)
* On Chromium >=63 if the request is sent by the browser

As of this change (if we resolve with an empty array here), these requests still
get logged (to all tabs), just like before. However, with the next patch under
review, we deliberately stop processing requests sent by the browser, while
other requests we don't find any tabId for won't be logged either anymore. IMO
it's not worth to bother about them if it's just for outdated Chrome versions.

[kzar, 2018-04-09 14:39:19]

https://codereview.adblockplus.org/29739594/diff/29741715/lib/requestBlocker.js
File lib/requestBlocker.js (right):

https://codereview.adblockplus.org/29739594/diff/29741715/lib/requestBlocker....
lib/requestBlocker.js:94: return Promise.resolve([]);
On 2018/04/05 16:56:33, Sebastian Noack wrote:
> ...IMO it's not worth to bother about them if it's just for outdated Chrome
versions.

Sounds reasonable to me, maybe add a note to the issue to explain that know
limitation?

[Sebastian Noack, 2018-04-09 21:08:20]

https://codereview.adblockplus.org/29739594/diff/29741715/lib/requestBlocker.js
File lib/requestBlocker.js (right):

https://codereview.adblockplus.org/29739594/diff/29741715/lib/requestBlocker....
lib/requestBlocker.js:94: return Promise.resolve([]);
On 2018/04/09 14:39:18, kzar wrote:
> On 2018/04/05 16:56:33, Sebastian Noack wrote:
> > ...IMO it's not worth to bother about them if it's just for outdated Chrome
> versions.
> 
> Sounds reasonable to me, maybe add a note to the issue to explain that know
> limitation?

There you go: https://issues.adblockplus.org/ticket/6544?action=diff&version=3