Issue 6565 - Ignore requests sent by Chrome's new tab page

Issue 6565 - Ignore requests sent by Chrome's new tab page

[Sebastian Noack, 2018-04-16 13:07:59]

This change essentially ignores any requests that we neither know the frame for
nor have an "initiator", while previously we only ignored requests if they don't
have an "initiator" and the tabId was -1. This worked for the requests loading
the suggestions when typing into the address bar, but not for the requests sent
by the new tab page.

[kzar, 2018-04-16 15:53:38]

On 2018/04/16 13:07:59, Sebastian Noack wrote:
> This change essentially ignores any requests that we neither know the frame
for
> nor have an "initiator", while previously we only ignored requests if they
don't
> have an "initiator" and the tabId was -1. This worked for the requests loading
> the suggestions when typing into the address bar, but not for the requests
sent
> by the new tab page.

I wonder if we should update the page/frame structure when a tab is created[1]?
That way we could offer to block these requests, which I'd personally actually
quite like to be able to do.

[1] - https://developer.chrome.com/extensions/tabs#event-onCreated

https://codereview.adblockplus.org/29753574/diff/29753577/lib/requestBlocker.js
File lib/requestBlocker.js (right):

https://codereview.adblockplus.org/29753574/diff/29753577/lib/requestBlocker....
lib/requestBlocker.js:160: // Ignore requests sent by extensions or by Firefox
itself:
I feel it's kind of confusing how we both make the logic generic (e.g. originUrl
could be details.originUrl or details.initiatorUrl), but then talk about the
behavior as being specific to browsers at the same time.

I wonder if we'd be better off to make the different code paths for Firefox VS
Chrome more explicit instead:

let {originUrl} = details;
if (originUrl && originUrl.protocol == "mozilla:" ...

https://codereview.adblockplus.org/29753574/diff/29753577/lib/requestBlocker....
lib/requestBlocker.js:185: if ((!frame && !originUrl) || checkWhitelisted(page,
frame, originUrl))
I'd rather not mix new check with the whitelisting check. I think they should be
separate.

[Sebastian Noack, 2018-04-16 16:42:21]

On 2018/04/16 15:53:38, kzar wrote:
> I wonder if we should update the page/frame structure when a tab is
created[1]?
> That way we could offer to block these requests, which I'd personally actually
> quite like to be able to do.
> 
> [1] - https://developer.chrome.com/extensions/tabs#event-onCreated

Interesting idea. But this would be a new/separate feature, and likely somewhat
more complex to implement. What if the tab's URL changes? Also what is about
Firefox's new tab page?

https://codereview.adblockplus.org/29753574/diff/29753577/lib/requestBlocker.js
File lib/requestBlocker.js (right):

https://codereview.adblockplus.org/29753574/diff/29753577/lib/requestBlocker....
lib/requestBlocker.js:160: // Ignore requests sent by extensions or by Firefox
itself:
On 2018/04/16 15:53:38, kzar wrote:
> I feel it's kind of confusing how we both make the logic generic (e.g.
originUrl
> could be details.originUrl or details.initiatorUrl), but then talk about the
> behavior as being specific to browsers at the same time.
> 
> I wonder if we'd be better off to make the different code paths for Firefox VS
> Chrome more explicit instead:
> 
> let {originUrl} = details;
> if (originUrl && originUrl.protocol == "mozilla:" ...

Well, the logic is generic (and covers more than just Chrome and Firefox). Here,
we want to ignore any request that originates from an extension, and that is
what the code expresses. Below, we ignore any request of unknown origin. Also
unless you want to add similar redundancy to checkWhitelisted(),
extractHostFromFrame() and getKey() we need a single URL object anyway. Also
what is about Microsoft Edge? The same logic applies there.

If the comments would match the logic they would be redundant. They exist to
provide additional context, and to explain why this logic is necessary.

https://codereview.adblockplus.org/29753574/diff/29753577/lib/requestBlocker....
lib/requestBlocker.js:185: if ((!frame && !originUrl) || checkWhitelisted(page,
frame, originUrl))
On 2018/04/16 15:53:38, kzar wrote:
> I'd rather not mix new check with the whitelisting check. I think they should
be
> separate.

Done.

[kzar, 2018-04-17 10:18:51]

LGTM