1437258 - When webRequestBlocking permission not in manifest, FF should throw an error on blocking listeners

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, defect, P5)

Description

[mlissner]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180208220029

Steps to reproduce:

If you create a listener like this:

browser.webRequest.onBeforeRequest.addListener(
  redirect,
  {urls: [pattern], types: ["sub_frame"]},
  ["blocking"]
);

And you don't have the webRequestBlocking permission, it should fail or throw some kind of error. 


Actual results:

Instead of failing, it just doesn't block, which is confusing. Not knowing this required a specific permission, I assumed I misunderstood some async Javascript thing or something. Nope! Eventually I figured it out, but only after reading a bunch of docs.


Expected results:

It should have failed and thrown an error. Or at least just thrown an error.

Comment 1

[Shane Caraveo (:mixedpuppy)]

We should at least do a warning if not an error.  Need to think about any potential issues if we simply throw an error.

Comment 2

[mlissner]

Sorry, yes, that's what I meant: Throw a warning. If that makes this an easy thing, noncontroversial thing to do, I say we just go for that. It'll solve the problem quickly so others don't get bit.

Comment 3

[Shane Caraveo (:mixedpuppy)]

I'd be +1 for adding a warning, but not high on my list if you or anyone wants to jump on it.

Comment 4

[mlissner]

Alas, my FF contributions start and end at bugzilla. :)

Comment 5

[tossj]

Hi, I would like to work on this issue.

It looks like https://searchfox.org/mozilla-central/source/toolkit/modules/addons/WebRequest.jsm and https://searchfox.org/mozilla-central/source/toolkit/components/extensions/parent/ext-webRequest.js should be starting points on where to implement the required changes. Is this correct?

Comment 6

[tossj]

The existing WebRequest code does log an error when an add-on is using the "blocking" option without the webRequestBlocking permission, but the error only appears on the Browser Console, and not the add-on debugging console: https://searchfox.org/mozilla-central/rev/b28b94dc81d60c6d9164315adbd4a5073526d372/toolkit/components/extensions/parent/ext-webRequest.js#59-71

Is there a way to make this same code also log to the add-on debugging console? I tried importing ExtensionUtils.jsm and replacing the error message with an ExtensionError object with the same message, but that did not work.

Comment 7

[Shane Caraveo (:mixedpuppy)]

What does Chrome do in this case?  We'd need to remain compatible.  If chrome throws an exception, we should do that, if it doesn't, we do not want to either.

This isn't the only situation that can cause confusion.  A potential feature we could think about would be a "strict" mode where any console warning/error would instead throw.

Comment 8

[mlissner]

> the error only appears on the Browser Console, and not the add-on debugging console

Throwing this out there as an add-on developer, I *never* look in the browser console. I hardly know it exists because whenever I look, it's full of junk that doesn't seem to have anything to do with me. Maybe I'm naive, but putting a log there is about as useless as not logging at all. I certainly didn't find it there when I ran into this issue (and filed this bug).

Comment 9

[tossj]

Attachment: chrome-webRequestBlocking.png

In Chrome after I removed the webRequestBlocking permission and reloaded the extension, I got this error message.

If anyone wants to try, I tested with this extension: https://developer.chrome.com/extensions/examples/extensions/catblock.zip. The loldogs[1] redirectUrl will have to be changed to an active URL, in background.js.

Comment 10

[Caitlin Neiman [:caitmuenster]]

Hey tossj, sorry for the delayed response. Are you still interested in working on this?

Comment 11

[tossj]

Yes

Comment 12

[Caitlin Neiman [:caitmuenster]]

Awesome! I'm assigning you to the bug. :) (I'm also going to throw out a link to our WebExtensions code contributor onramp in case you would like to see an overview of the bug fixing process: https://wiki.mozilla.org/WebExtensions/Contribution_Onramp)

Shane, could you advise on next steps?

Comment 13

[Shane Caraveo (:mixedpuppy)]

Hi @tossj.  

Have you built firefox?  If not that would be the place to start[1].  

If you have, then you'd want to start looking at ext-webRequest.js[2] and WebRequest.jsm[3].

It seems like we are issuing an error in the console, but we are not setting runtime.lastError.  It looks like we should be throwing some exception instead of just calling reportError.  I would start looking at that.


[1] https://developer.mozilla.org/En/Simple_Firefox_build
[2] https://searchfox.org/mozilla-central/rev/c9272ef398954288525e37196eada1e5a93d93bf/toolkit/components/extensions/parent/ext-webRequest.js#115
[3] https://searchfox.org/mozilla-central/rev/c9272ef398954288525e37196eada1e5a93d93bf/toolkit/components/extensions/parent/ext-webRequest.js#59

Comment 14

[tossj]

I tried throwing an ExtensionError object in ext-webRequest.js but it only appears on the browser console.

ext-cookies.js also throws an ExtensionError object, but when that error object gets to ExtensionParent.jsm it gets passed to the reply function, and seems to be logged to the debugging console after the call to context.parentMessageManager.sendAsyncMessage: https://searchfox.org/mozilla-central/rev/c9272ef398954288525e37196eada1e5a93d93bf/toolkit/components/extensions/ExtensionParent.jsm#934

Can something similar be done to catch the error at https://searchfox.org/mozilla-central/rev/c9272ef398954288525e37196eada1e5a93d93bf/toolkit/components/extensions/ExtensionParent.jsm#1025?

Comment 15

[Shane Caraveo (:mixedpuppy)]

That seems reasonable to try.  Without a thorough look, here's some thoughts:

It looks like the listener should also be removed from context.listenerProxies on a failure.

You'd need to listen for a response in ExtensionChild.js similar to the reply you linked to:

https://searchfox.org/mozilla-central/rev/c56977420df7a1b692ce0f7e499ddb364d9fd7b2/toolkit/components/extensions/ExtensionChild.jsm#996

Maybe API:AddListenerResult.  In the child, the api listener map would also need to be cleaned up on failure (see addListener in the child).

Comment 16

[tossj]

Attachment: 1437258.patch

I implemented the ideas we discussed and the error message now comes on the debugging console. Could you give any feedback on the patch?

Also, does this bug require automated tests, or is this something a QA team will check?

parent/ext-webRequest.js has another Cu.reportError call at https://searchfox.org/mozilla-central/rev/0ec9f5972ef3e4a479720630ad7285a03776cdfc/toolkit/components/extensions/parent/ext-webRequest.js#46. I haven't checked whether this does not show up on the debugging console, but if it does do you want to also change that to an ExtensionError in this bug or a separate one?

Comment 17

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 9019026 [details] [diff] [review]
1437258.patch

This is looking pretty good.  You will need a test.  I would add it to test_ext_webRequest_permission.js.  Look at test_ext_webRequest.suspend for an example using promiseConsoleOutput.

Comment 18

[tossj]

Attachment: Bug 1437258 - Move webRequestBlocking permission check to webRequest child to log error to debugging console.

With this change, ext-webRequest.js will throw an ExtensionError when "blocking" is used without the "webRequestBlocking" permission, and ExtensionParent.jsm will relay that error to ExtensionChild.jsm so an error can be logged to the extension debugging console. Previously, the error was only being logged to the browser console.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/51b417f1d2c3
Added exception handling to output webRequest blocking error to debugging console. r=mixedpuppy

Comment 20

[Cristian Brindusan [:cbrindusan]]

Backed out changeset 51b417f1d2c3 (Bug 1437258) for ESlint failure on ext-webRequest.js.

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/035d5b058729aed86f13d9309c657ffb94ba8fdb

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=51b417f1d2c3ef81f200ef37041864980b962df8&selectedJob=214979967

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=214979967&repo=mozilla-inbound&lineNumber=299

Comment 21

[tossj]

I think the wrong patch was added to mozilla-inbound. It's supposed to be this revision https://phabricator.services.mozilla.com/D9937, not the .patch file that was attached to this bug. The .patch file was only an initial solution that needed feedback.

Is there a different process for landing Phabricator revisions into the code base?

Comment 22

[Andrew Swan [:aswan]]

tossj, the process is the same, but to make it clear which patch should be landed, you can mark the old one as obsolete (by going to "Edit Details" for the attachment.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1382e8cdf8a6
Added exception handling to log webRequest blocking error to debugging console. r=mixedpuppy

Comment 24

[Stefan Hindli [:stefan_hindli]]

Backed out changeset 1382e8cdf8a6 (Bug 1437258) for xpcshell failures CLOSED TREE

https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&fromchange=0bbae43c6e6ef271e7287c224d98ac268b18fdbe&selectedJob=215016918

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=215016918&repo=autoland&lineNumber=2550

[task 2018-12-01T02:49:41.541Z] 02:49:41     INFO -  /builds/worker/workspace/build/tests/xpcshell/head.js:_execute_test:546
[task 2018-12-01T02:49:41.542Z] 02:49:41     INFO -  -e:null:1
[task 2018-12-01T02:49:41.542Z] 02:49:41     INFO -  exiting test
[task 2018-12-01T02:49:41.543Z] 02:49:41     INFO -  "CONSOLE_MESSAGE: (error) [JavaScript Error: "[Exception... "Abort"  nsresult: "0x80004004 (NS_ERROR_ABORT)"  location: "JS frame :: /builds/worker/workspace/build/tests/xpcshell/head.js :: _abort_failed_test :: line 759"  data: no]"]
[task 2018-12-01T02:49:41.544Z] 02:49:41     INFO -  _abort_failed_test@/builds/worker/workspace/build/tests/xpcshell/head.js:759:9
[task 2018-12-01T02:49:41.544Z] 02:49:41     INFO -  do_report_result@/builds/worker/workspace/build/tests/xpcshell/head.js:866:5
[task 2018-12-01T02:49:41.545Z] 02:49:41     INFO -  Assert<@/builds/worker/workspace/build/tests/xpcshell/head.js:56:5
[task 2018-12-01T02:49:41.546Z] 02:49:41     INFO -  proto.report@resource://testing-common/Assert.jsm:214:5
[task 2018-12-01T02:49:41.546Z] 02:49:41     INFO -  proto.ok@resource://testing-common/Assert.jsm:234:5
[task 2018-12-01T02:49:41.547Z] 02:49:41     INFO -  handleResult@resource://testing-common/ExtensionXPCShellUtils.jsm:301:9
[task 2018-12-01T02:49:41.547Z] 02:49:41     INFO -  emit@resource://gre/modules/ExtensionCommon.jsm:310:24
[task 2018-12-01T02:49:41.548Z] 02:49:41     INFO -  receiveMessage@resource://gre/modules/Extension.jsm:1467:7
[task 2018-12-01T02:49:41.549Z] 02:49:41     INFO -  _do_main@/builds/worker/workspace/build/tests/xpcshell/head.js:226:3
[task 2018-12-01T02:49:41.549Z] 02:49:41     INFO -  _execute_test@/builds/worker/workspace/build/tests/xpcshell/head.js:546:5
[task 2018-12-01T02:49:41.550Z] 02:49:41     INFO -  @-e:1:1
[task 2018-12-01T02:49:41.550Z] 02:49:41     INFO -  "
[task 2018-12-01T02:49:41.551Z] 02:49:41     INFO -  "CONSOLE_MESSAGE: (error) [JavaScript Error: "[Exception... "Abort"  nsresult: "0x80004004 (NS_ERROR_ABORT)"  location: "JS frame :: /builds/worker/workspace/build/tests/xpcshell/head.js :: _abort_failed_test :: line 759"  data: no]"]
[task 2018-12-01T02:49:41.551Z] 02:49:41     INFO -  _abort_failed_test@/builds/worker/workspace/build/tests/xpcshell/head.js:759:9
[task 2018-12-01T02:49:41.552Z] 02:49:41     INFO -  do_report_result@/builds/worker/workspace/build/tests/xpcshell/head.js:866:5
[task 2018-12-01T02:49:41.553Z] 02:49:41     INFO -  Assert<@/builds/worker/workspace/build/tests/xpcshell/head.js:56:5
[task 2018-12-01T02:49:41.553Z] 02:49:41     INFO -  proto.report@resource://testing-common/Assert.jsm:214:5
[task 2018-12-01T02:49:41.554Z] 02:49:41     INFO -  proto.ok@resource://testing-common/Assert.jsm:234:5
[task 2018-12-01T02:49:41.554Z] 02:49:41     INFO -  handleResult@resource://testing-common/ExtensionXPCShellUtils.jsm:301:9
[task 2018-12-01T02:49:41.555Z] 02:49:41     INFO -  emit@resource://gre/modules/ExtensionCommon.jsm:310:24
[task 2018-12-01T02:49:41.555Z] 02:49:41     INFO -  receiveMessage@resource://gre/modules/Extension.jsm:1467:7
[task 2018-12-01T02:49:41.556Z] 02:49:41     INFO -  _do_main@/builds/worker/workspace/build/tests/xpcshell/head.js:228:3
[task 2018-12-01T02:49:41.557Z] 02:49:41     INFO -  _execute_test@/builds/worker/workspace/build/tests/xpcshell/head.js:546:5
[task 2018-12-01T02:49:41.557Z] 02:49:41     INFO -  @-e:1:1
[task 2018-12-01T02:49:41.558Z] 02:49:41     INFO -  "

Comment 25

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0066c3d92589
Backed out changeset 1382e8cdf8a6 for xpcshell failures CLOSED TREE

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f4ae445c2717
Move webRequestBlocking permission check to webRequest child to log error to debugging console. r=mixedpuppy

Comment 27

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/f4ae445c2717

Comment 28

[Caitlin Neiman [:caitmuenster]]

Woohoo! Congrats, tossj, and thanks for the patch! Your contribution has been added to our recognition wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition

Would you be interested in creating an account on mozillians.org? I'd be happy to vouch for you!

Comment 29

[Vlad Jiman]

Will this bug require validation from the QA team? if so please include some more info on howto correctly test it, thanks.

Comment 30

[tossj]

After consulting with :mixedpuppy, the xpcshell test that was written for this bug is enough, and it does not require validation from the QA team.