Opened 16 months ago

Closed 16 months ago

Last modified 16 months ago

#6841 closed defect (fixed)

Snippets do not work in Chrome 49 — at Version 19

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

https://codereview.adblockplus.org/29850574/

Description (last modified by mjethani)

Environment

ABP 3.2.2102
Chrome 49 / Windows 10

Could not reproduce in (snippets work fine):
Chrome 66 / 55 / 50 / Windows 10
Firefox 61 / 55 / 51 / Windows 10

How to reproduce

  1. Add snippet to custom filters adblockplus.org#$#log 'Hello, world!'
  2. Navigate to https://adblockplus.org

Observed behaviour

The snippet does not seem to run. "Hello, world!" is not printed to the console as in other browsers/versions.

Expected behaviour

Snippets to work in Chrome 49 (unless there is a reason they cannot).

Notes

  • adblockpluschrome/lib/contentFiltering.js is calling browser.tabs.executeScript, passing a frameId of 0 which causes an exception in Chrome 49 (which we silently ignore):
browser.tabs.executeScript failed Error: Invalid value for argument 2. Property 'frameId': Unexpected property.
    at validate (extensions::schemaUtils:34:13)
    at Object.normalizeArgumentsAndValidate (extensions::schemaUtils:117:3)
    at Object.<anonymous> (extensions::binding:374:30)
    at Object.value [as executeScript] (chrome-extension://ffiagampjdjojfgakjeeoipjmfmkahnn/polyfill.js:108:14)
    at executeScript (chrome-extension://ffiagampjdjojfgakjeeoipjmfmkahnn/lib/adblockplus.js:11490:18)
    at port.on (chrome-extension://ffiagampjdjojfgakjeeoipjmfmkahnn/lib/adblockplus.js:11537:9)
    at Object._onMessage (chrome-extension://ffiagampjdjojfgakjeeoipjmfmkahnn/lib/adblockplus.js:3758:22)
    at Object._dispatch (chrome-extension://ffiagampjdjojfgakjeeoipjmfmkahnn/ext/common.js:41:22)
    at browser.runtime.onMessage.addListener (chrome-extension://ffiagampjdjojfgakjeeoipjmfmkahnn/ext/background.js:604:26)
    at wrapper (chrome-extension://ffiagampjdjojfgakjeeoipjmfmkahnn/polyfill.js:156:20)

Hints for testers

Given the filter 127.0.0.1#$#log 'Hello from the main frame' and the following document loaded off 127.0.0.1:

<html>
  <body>
    <h1>Main Frame</h1>
    <iframe src="http://example.com"></iframe>
  </body>
</html>

You should see only one instance of the text "Hello from the main frame" printed to the console. Likewise, changing the filter to example.com#$#log 'Hello from example.com' should print "Hello from example.com" only once. But changing the filter to 127.0.0.1,example.com#$#log 'Hello' should print "Hello" twice.

Make sure that snippets intended for a subframe are not injected into the top frame. To do this, use this document:

<html>
  <head>
    <script>window.console = null;</script>
  </head>
  <body>
    <h1>Main Frame</h1>
    <iframe src="http://example.com"></iframe>
  </body>
</html>

The filter example.com#$#trace 'Hello from example.com' should print "Hello from example.com"; if an error is printed to the console (because the inline script assigns the console object to null), it means the snippet is being injected into the top frame.

Please test this on both Chrome and Firefox.

Change History (19)

comment:1 Changed 16 months ago by mjethani

Ross, can you add details about what filter lists you're subscribed to?

I am unable to reproduce this at my end.

comment:2 Changed 16 months ago by Ross

I've tried with the full set of default filter lists just after an install, just with EasyList, with AA on and off.

The snippet adblockplus.org#$#log 'Hello, world!' prints to the console in Chrome 55, but does not for me in Chrome 49. I don't see any errors in any consoles. Does it print in 49 for you?

comment:3 Changed 16 months ago by Ross

  • Description modified (diff)

It also works in Chrome 50.

comment:4 Changed 16 months ago by mjethani

My bad, I was testing with a newer version of Chrome. I do see now that it doesn't work.

However the issue appears to be something else. I would be surprised if element hiding was working at all. I see this error in the background console:

extensions::uncaught_exception_handler:8 Error in response to tabs.get: Error: Invalid value for argument 1. Property 'path': Value does not match any valid type choices.
    at Object._applyChanges (chrome-extension://bhignfpcigccnlfapldlodmhlidjaion/ext/background.js:333:35)
    at Object.browser.tabs.get [as callback] (chrome-extension://bhignfpcigccnlfapldlodmhlidjaion/ext/background.js:388:16)
    at Object.value [as get] (chrome-extension://bhignfpcigccnlfapldlodmhlidjaion/polyfill.js:95:23)
    at Object._queueChanges (chrome-extension://bhignfpcigccnlfapldlodmhlidjaion/ext/background.js:368:20)
    at Object._addChange (chrome-extension://bhignfpcigccnlfapldlodmhlidjaion/ext/background.js:397:14)
    at Object.setIcon (chrome-extension://bhignfpcigccnlfapldlodmhlidjaion/ext/background.js:404:12)
    at setIcon (chrome-extension://bhignfpcigccnlfapldlodmhlidjaion/lib/adblockplus.js:10446:26)
    at FilterNotifier.on (chrome-extension://bhignfpcigccnlfapldlodmhlidjaion/lib/adblockplus.js:10463:5)
    at Object.emit (chrome-extension://bhignfpcigccnlfapldlodmhlidjaion/lib/adblockplus.js:4460:7)

comment:5 Changed 16 months ago by Ross

Hmm, I don't see that error at all and the Element Hiding and Element Hiding Emulation filter test pages seem to pass as expected in 49 for me.

I'm using Chrome 49 x64 on Windows 10, not that that should make much of a difference.

comment:6 Changed 16 months ago by mjethani

OK, there are multiple issues here.

First of all, at least on macOS 10.12.6, Chrome 49 does not support certain APIs. Secondly, there appears to be a bug in Chrome 49 with arrow functions.

As for the specific issue of snippets, I get this error (silently ignored in the development build):

Error: Invalid value for argument 2. Property 'frameId': Unexpected property.

It seems the frameId property was added in a later release. In that case, we cannot really run snippets reliably on Chrome 49 without messing something up. It's best to leave this as it is. i.e. "won't fix" (snippets are not supported on Chrome 49, at least for now).

comment:7 Changed 16 months ago by mjethani

Ross, in chrome://extensions can you click on the background page link and see if there are any other errors in the console? Switch between tabs, load a page in the browser, and see if you're getting any errors. These might be separate issues, or maybe it's just me (my build of Chrome 49 may not be the final release build).

comment:8 Changed 16 months ago by kzar

I can reproduce this, I can't see any exceptions. I can see that getScriptsForDomain is being called correctly and is returning the correct result. Continuing to investigate...

comment:9 Changed 16 months ago by kzar

browser.tabs.executeScript is failing, I'm getting this exception but we silently ignore it:

browser.tabs.executeScript failed Error: Invalid value for argument 2. Property 'frameId': Unexpected property.
    at validate (extensions::schemaUtils:34:13)
    at Object.normalizeArgumentsAndValidate (extensions::schemaUtils:117:3)
    at Object.<anonymous> (extensions::binding:374:30)
    at Object.value [as executeScript] (chrome-extension://ffiagampjdjojfgakjeeoipjmfmkahnn/polyfill.js:108:14)
    at executeScript (chrome-extension://ffiagampjdjojfgakjeeoipjmfmkahnn/lib/adblockplus.js:11490:18)
    at port.on (chrome-extension://ffiagampjdjojfgakjeeoipjmfmkahnn/lib/adblockplus.js:11537:9)
    at Object._onMessage (chrome-extension://ffiagampjdjojfgakjeeoipjmfmkahnn/lib/adblockplus.js:3758:22)
    at Object._dispatch (chrome-extension://ffiagampjdjojfgakjeeoipjmfmkahnn/ext/common.js:41:22)
    at browser.runtime.onMessage.addListener (chrome-extension://ffiagampjdjojfgakjeeoipjmfmkahnn/ext/background.js:604:26)
    at wrapper (chrome-extension://ffiagampjdjojfgakjeeoipjmfmkahnn/polyfill.js:156:20)

comment:10 Changed 16 months ago by kzar

It's since we're passing the frameId value to browser.tabs.executeScript. I suggest if frameId is 0 we just omit it. That seems to work. For other frames I guess we have to accept it won't work on Chrome 49. What do you think?

comment:11 Changed 16 months ago by kzar

  • Component changed from Core to Platform
  • Description modified (diff)
  • Owner set to kzar
  • Priority changed from Unknown to P1
  • Ready set

Coming up with a patch for that...

comment:12 Changed 16 months ago by kzar

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

comment:13 Changed 16 months ago by kzar

  • Description modified (diff)

comment:14 Changed 16 months ago by abpbot

A commit referencing this issue has landed:
Issue 6841 - Avoid passing frameId of 0 to tabs.executeScript

comment:15 Changed 16 months ago by kzar

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

comment:16 Changed 16 months ago by kzar

  • Cc rscott added

Adding you as Cc Rick in case you missed this. We just pushed another hotfix for the release, sorry!

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

comment:17 Changed 16 months ago by mjethani

  • Description modified (diff)

comment:18 Changed 16 months ago by kzar

Thanks for adding those testing hints Manish, it will be interesting to see how Chrome 49 handles injecting snippets into frames. It's possible that won't work either, but there won't be much we can do there, and it'll fail silently at least.

comment:19 Changed 16 months ago by mjethani

  • Description modified (diff)
Note: See TracTickets for help on using tickets.