Opened 22 months ago

Last modified 21 months ago

#6565 closed defect

Cannot destructure property `protocol` of 'undefined' exceptions in background console — at Version 8

Reported by: kzar Assignee: kzar
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 (8)

comment:1 follow-up: Changed 22 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 22 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 22 months ago by kzar (previous) (diff)

comment:3 Changed 22 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 22 months ago by kzar

  • Owner set to kzar

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

comment:5 Changed 22 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 22 months ago by kzar

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

comment:7 Changed 22 months ago by kzar

  • Description modified (diff)

comment:8 Changed 22 months ago by kzar

  • Description modified (diff)
  • Status changed from reviewing to reopened
Note: See TracTickets for help on using tickets.