1263481 - WebRequest multiple listeners support is broken

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Giorgio Maone [:ma1]]

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 1

[Giorgio Maone [:ma1]]

Attachment: MozReview Request: Bug 1263481 - Fix webRequest multiple listeners support. r?kmag

Review commit: https://reviewboard.mozilla.org/r/45381/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45381/

Comment 2

[Kris Maglione [:kmag]]

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?

Comment 3

[Giorgio Maone [:ma1]]

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/

Comment 4

[Giorgio Maone [:ma1]]

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.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/9bf63783f4dd

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9bf63783f4dd