Opened 19 months ago

Closed 18 months ago

Last modified 18 months ago

#6565 closed defect (fixed)

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

Reported by: kzar Assignee: sebastian
Priority: P2 Milestone: Adblock-Plus-3.1-for-Chrome-Opera-Firefox
Module: Platform Keywords:
Cc: sebastian, Ross, mjethani Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29750566/
https://codereview.adblockplus.org/29753574/

Description (last modified by 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.

Change History (15)

comment:1 follow-up: Changed 19 months ago by 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.

comment:2 in reply to: ↑ 1 Changed 19 months ago by 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?

Last edited 19 months ago by kzar (previous) (diff)

comment:3 Changed 18 months ago by 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.

comment:4 Changed 18 months ago by kzar

  • Owner set to kzar

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

comment:5 Changed 18 months ago by 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).

comment:6 Changed 18 months ago by kzar

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:7 Changed 18 months ago by kzar

  • Description modified (diff)

comment:8 Changed 18 months ago by kzar

  • Description modified (diff)
  • Status changed from reviewing to reopened

comment:9 Changed 18 months ago by kzar

  • Owner kzar deleted

comment:10 Changed 18 months ago by sebastian

  • Owner set to sebastian

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

comment:11 Changed 18 months ago by 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

comment:12 Changed 18 months ago by sebastian

  • Review URL(s) modified (diff)
  • Status changed from reopened to reviewing

comment:13 Changed 18 months ago by abpbot

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

comment:14 Changed 18 months ago by sebastian

  • Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:15 Changed 18 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

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

Note: See TracTickets for help on using tickets.