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

RESOLVED FIXED in Firefox 66

Status

P5
normal
RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: mlissner, Assigned: tossj, Mentored)

Tracking

({good-first-bug})

Trunk
mozilla66
good-first-bug
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
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.

Updated

a year ago
Component: Untriaged → WebExtensions: Request Handling
Product: Firefox → Toolkit
We should at least do a warning if not an error.  Need to think about any potential issues if we simply throw an error.
Priority: -- → P5
(Reporter)

Comment 2

a year ago
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.
I'd be +1 for adding a warning, but not high on my list if you or anyone wants to jump on it.
(Reporter)

Comment 4

a year ago
Alas, my FF contributions start and end at bugzilla. :)
Keywords: good-first-bug
Mentor: mixedpuppy
(Assignee)

Comment 5

11 months ago
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?
Flags: needinfo?(mixedpuppy)
(Assignee)

Comment 6

11 months ago
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.
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.
Flags: needinfo?(mixedpuppy)
(Reporter)

Comment 8

11 months ago
> 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).
(Assignee)

Comment 9

11 months ago
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.

Updated

9 months ago
Product: Toolkit → WebExtensions
Hey tossj, sorry for the delayed response. Are you still interested in working on this?
Flags: needinfo?(tossj)
(Assignee)

Comment 11

5 months ago
Yes
Flags: needinfo?(tossj)
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?
Assignee: nobody → tossj
Flags: needinfo?(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
Flags: needinfo?(mixedpuppy) → needinfo?(tossj)
(Assignee)

Comment 14

5 months ago
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?
Flags: needinfo?(tossj) → needinfo?(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).
Flags: needinfo?(mixedpuppy)
(Assignee)

Comment 16

5 months ago
Posted patch 1437258.patch (obsolete) — Splinter Review
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?
Flags: needinfo?(mixedpuppy)
Attachment #9019026 - Flags: feedback?(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.
Flags: needinfo?(mixedpuppy)
Attachment #9019026 - Flags: feedback?(mixedpuppy) → feedback+
(Assignee)

Comment 18

5 months ago
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.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Updated

4 months ago
Keywords: checkin-needed

Comment 19

4 months ago
Pushed by ccoroiu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/51b417f1d2c3
Added exception handling to output webRequest blocking error to debugging console. r=mixedpuppy
Keywords: checkin-needed
(Assignee)

Comment 21

4 months ago
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?
Flags: needinfo?(tossj)
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.
(Assignee)

Updated

4 months ago
Attachment #9019026 - Attachment is obsolete: true
(Assignee)

Updated

4 months ago
Keywords: checkin-needed

Comment 23

4 months ago
Pushed by ccoroiu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1382e8cdf8a6
Added exception handling to log webRequest blocking error to debugging console. r=mixedpuppy
Keywords: checkin-needed
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

4 months ago
Backout by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0066c3d92589
Backed out changeset 1382e8cdf8a6 for xpcshell failures CLOSED TREE
Attachment #9020432 - Attachment description: Bug 1437258 - Added exception handling to log webRequest blocking error to debugging console. → Bug 1437258 - Move webRequestBlocking permission check to webRequest child to log error to debugging console.
(Assignee)

Updated

3 months ago
Keywords: checkin-needed

Comment 26

3 months ago
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f4ae445c2717
Move webRequestBlocking permission check to webRequest child to log error to debugging console. r=mixedpuppy
Keywords: checkin-needed

Comment 27

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f4ae445c2717
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox66: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Depends on: 1514731
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

2 months ago

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

Flags: needinfo?(tossj)
(Assignee)

Comment 30

2 months ago

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.

Flags: needinfo?(tossj)

Updated

2 months ago
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.