Cannot destructure property `protocol` of 'undefined' exceptions in background console

[kzar]

Environment

Chrome 65, Adblock Plus devbuild built from ​118c7856d36d.

How to reproduce

1. Open the background console for the extension.
2. Open a new tab and browse to ​https://reddit.com

Observed behaviour

This exception shows in the background console multiple times a tab is opened.

Error in event handler for webRequest.onBeforeRequest/28: TypeError: Cannot destructure property `protocol` of 'undefined' or 'null'.
    at exports.stringifyURL.url (chrome-extension://ffiagampjdjojfgakjeeoipjmfmkahnn/lib/adblockplus.js:2229:26)
    at match (chrome-extension://ffiagampjdjojfgakjeeoipjmfmkahnn/lib/adblockplus.js:3910:19)
    at exports.checkWhitelisted (chrome-extension://ffiagampjdjojfgakjeeoipjmfmkahnn/lib/adblockplus.js:3958:18)
    at browser.webRequest.onBeforeRequest.addListener.details (chrome-extension://ffiagampjdjojfgakjeeoipjmfmkahnn/lib/adblockplus.js:6662:7)

Expected behaviour

No exceptions showing in the background console.

Notes

• This is a regression caused by ​7781a9fc138e.
• The requests are coming from the "New tab page", so that Chrome can show a preview of what the recently viewed pages look like.

Hints for testers

Test that the exceptions are fixed in Chrome:

1. Ensure the "New tab page" is enabled in Chrome, so that when you open a new tab a few squares previewing recently viewed websites is shown.
2. Open the background console for the extension.
3. Open and close new tabs and make sure exceptions aren't shown.

Test that request blocking and whitelisting continues to work as expected in both Firefox and Chrome.

[sebastian]

I cannot reproduce this. I tested on Chrome 63 and 65. Also from just looking at the code, I cannot see how these changes will cause checkWhitelisted() to call match() which then calls stringifyURL() with undefined as url (if it didn't before). I might miss something, but without being able to reproduce the error, I cannot track it down.

[kzar]

Replying to sebastian:

I cannot reproduce this. I tested on Chrome 63 and 65.

Weird, since I can reproduce this 100% reliably.

Also from just looking at the code, I cannot see how these changes will cause checkWhitelisted() to call match() which then calls stringifyURL() with undefined as url (if it didn't before). I might miss something, but without being able to reproduce the error, I cannot track it down.

From running this through the debugger it *seems* like it could be since previously the onBeforeRequest listener in lib/requestBlocker.js only called checkWhitelisted if frame was truthy, but now it always does. In any case I see that page.url is undefined, but details.url isn't. Perhaps we should modify the line in onBeforeRequest where we create the ext.Page object to pass in the URL?

[sebastian]

Sounds good. I could write a patch, but since you would have to test the change anyway (as I cannot reproduce the issue), you might want to go ahead. Otherwise let me know.

[kzar]

Sure OK no problem, I'll get a patch together.

[kzar]

I see now it's the New Tab page that makes requests for each of the suggested pages so that it can show a preview of them (and I guess so they're preloaded ready).

[sebastian]

I can reproduce the error now, when I explicitly go to chrome-search://local-ntp/local-ntp.html.

[kzar]

In case it helps the troublesome requests have these URLs on my computer:

https://s2.googleusercontent.com/s2/favicons?domain_url=https://www.reddit.com/&alt=s&sz=32
https://s2.googleusercontent.com/s2/favicons?domain_url=https://intraforum.adblockplus.org/&alt=s&sz=32
https://s2.googleusercontent.com/s2/favicons?domain_url=https://issues.adblockplus.org/&alt=s&sz=32
https://s2.googleusercontent.com/s2/favicons?domain_url=http://example.com/&alt=s&sz=32
https://s2.googleusercontent.com/s2/favicons?domain_url=https://codereview.adblockplus.org/&alt=s&sz=32
https://s2.googleusercontent.com/s2/favicons?domain_url=https://news.ycombinator.com/&alt=s&sz=32
https://s2.googleusercontent.com/s2/favicons?domain_url=https://hg.adblockplus.org/adblockpluschrome&alt=s&sz=32

[abpbot]

A commit referencing this issue has landed:
​Issue 6565 - Ignore requests sent by Chrome's new tab page

[Ross]

Fixed. Requests from the new tab page do not cause exceptions in the background page and blocking/whitelisting still works as expected.

ABP 3.0.4.2042
Firefox 59 / 55 / 51 / Windows 10
Chrome 66 / 58 / 49 / Windows 7
Opera 52 / 45 / 36 / Windows 10