Closed Bug 1263481 Opened 4 years ago Closed 4 years ago

WebRequest multiple listeners support is broken

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Iteration:
48.3 - Apr 25
Tracking Status
firefox48 --- fixed

People

(Reporter: mao, Assigned: mao)

References

Details

(Whiteboard: [webRequest])

Attachments

(1 file)

Our current webRequest implementation stops iterating over listeners of a certain event if last callback doesn't return any value, or it throws an exception, or had no "blocking" option.

Steps to reproduce:

1. From a WebExtension register multiple webRequest.onResponseStarted listeners (this event does not support the "blocking" option) with some logging to check wether they're actually called
2. Check the logs

Expected result: all the registered listeners are called.
Actual result: only one listener gets called.
Comment on attachment 8739810 [details]
MozReview Request: Bug 1263481 - Fix webRequest multiple listeners support. r?kmag

https://reviewboard.mozilla.org/r/45381/#review42115

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest.html:163
(Diff revision 1)
>                    sendHeaders: [],
>                    responseStarted: [],
> +                  responseStarted2: [],
>                    error: [],
> -                  completed: []};
> +                  completed: [],
> +                };

Needs one more space.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest.html:422
(Diff revision 1)
>    browser.webRequest.onBeforeSendHeaders.addListener(onBeforeSendHeaders, {urls: ["<all_urls>"]}, ["blocking", "requestHeaders"]);
>    browser.webRequest.onSendHeaders.addListener(onSendHeaders, {urls: ["<all_urls>"]}, ["requestHeaders"]);
>    browser.webRequest.onBeforeRedirect.addListener(onBeforeRedirect, {urls: ["<all_urls>"]});
>    browser.webRequest.onHeadersReceived.addListener(onHeadersReceived, {urls: ["<all_urls>"]}, ["blocking", "responseHeaders"]);
>    browser.webRequest.onResponseStarted.addListener(checkIpAndRecord.bind(null, "responseStarted"), {urls: ["<all_urls>"]});
> +  browser.webRequest.onResponseStarted.addListener(checkIpAndRecord.bind(null, "responseStarted2"), {urls: ["<all_urls>"]});

Should we add similar checks for the other listeners as well?
Attachment #8739810 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8739810 [details]
MozReview Request: Bug 1263481 - Fix webRequest multiple listeners support. r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45381/diff/1-2/
https://reviewboard.mozilla.org/r/45381/#review42115

> Should we add similar checks for the other listeners as well?

I don't think so: onResponseStarted is the sweet spot to insert a test for multiple listeners calls inside the current battery without having to mind side effects on and from other tests (e.g. headers manipulation or cancelling the request).
Maybe later, after we achieve Chrome-compat and as a part of the work for Bug 1236122, I'm gonna refactor these tests to have all the events dispatched to multiple listeners, adjusting for the edge cases.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9bf63783f4dd
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.