Closed Bug 1305217 Opened 8 years ago Closed 8 years ago

Make webRequest oop-compatible

Categories

(WebExtensions :: General, defect, P1)

Product:
WebExtensions
Component:
General
Version:
51 Branch
Type:
defect

Tracking

(firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: robwu, Assigned: kmag)

References

(Blocks 3 open bugs)

Details

(Whiteboard: [webRequest] triaged)

Attachments

(2 files)

Bug 1305217: Part 1 - Run webRequest listeners asynchronously.
58 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug 1305217: Part 2 - Run webNavigation listeners asynchronously.
58 bytes, text/x-review-board-request
mixedpuppy
: review+
Details 
      No description provided.
Priority: -- → P2
Whiteboard: [webRequest] triaged
Bill, have you started working on this already? Bug 1254204 gets us most of the way there, so I've been thinking about doing the rest.
Flags: needinfo?(wmccloskey)
Nope. Please feel free to take it. Thanks!
Flags: needinfo?(wmccloskey)
Thanks
Assignee: wmccloskey → kmaglione+bmo
Depends on: 1254204
Priority: P2 → P1
Comment on attachment 8809239 [details]
Bug 1305217: Part 1 - Run webRequest listeners asynchronously.

https://reviewboard.mozilla.org/r/91824/#review91870

::: toolkit/components/extensions/ExtensionChild.jsm:507
(Diff revision 1)
>  
>    addListener(listener, args) {
> -    let set = this.childApiManager.listeners.get(this.path);
> -    if (!set) {
> -      set = new Set();
> -      this.childApiManager.listeners.set(this.path, set);
> +    let map = this.childApiManager.listeners.get(this.path);
> +
> +    if (map.listeners.has(listener)) {
> +      // TODO: Called with different args?

Add a reference to https://bugzilla.mozilla.org/show_bug.cgi?id=1305216

("Use ChildAPIManager for webNavigation API")
(comment 2 mentions the need for recognizing different filters in addListener)

::: toolkit/components/extensions/ExtensionChild.jsm:565
(Diff revision 1)
>      // delegated to the ParentAPIManager.
>      this.localApis = localApis;
>  
>      this.id = `${context.extension.id}.${context.contextId}`;
>  
> -    messageManager.addMessageListener("API:RunListener", this);
> +    MessageChannel.addListener(messageManager, "API:RunListener", this);

This change adds overhead to all method calls.
If a listener for which the API does not expect a response returns a response, then an error could occur.

How about keeping the existing "API:RunListener", and add a new MessageChannel-based handler when an API explicitly needs to handle responses?
Comment on attachment 8809239 [details]
Bug 1305217: Part 1 - Run webRequest listeners asynchronously.

https://reviewboard.mozilla.org/r/91824/#review91870

> This change adds overhead to all method calls.
> If a listener for which the API does not expect a response returns a response, then an error could occur.
> 
> How about keeping the existing "API:RunListener", and add a new MessageChannel-based handler when an API explicitly needs to handle responses?

I'll deal with optimizing it if and when it becomes an issue. For now, I don't think it's a major concern.
Comment on attachment 8809240 [details]
Bug 1305217: Part 2 - Run webNavigation listeners asynchronously.

https://reviewboard.mozilla.org/r/91826/#review92166
Attachment #8809240 - Flags: review?(mixedpuppy) → review+
Blocks: 1316916
Comment on attachment 8809239 [details]
Bug 1305217: Part 1 - Run webRequest listeners asynchronously.

https://reviewboard.mozilla.org/r/91824/#review92448

I'm somewhat bothered by the redirect issue possibly allowing onHeadersReceived, but dont see a way around that, and I'm not convinced its a real problem.  Otherwise good to go.
Attachment #8809239 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8809239 [details]
Bug 1305217: Part 1 - Run webRequest listeners asynchronously.

https://reviewboard.mozilla.org/r/91824/#review92448

To be clear, that only happens when the request is redirected after the connection has been initiated (i.e., from onBeforeSendHeaders). Redirects from onBeforeRequest should still prevent a connection from being established or subsequent listeners from firing.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4efa832fc852bb0499f8518c458ff052bede560e
Bug 1305217: Part 1 - Run webRequest listeners asynchronously. r=mixedpuppy

https://hg.mozilla.org/integration/mozilla-inbound/rev/05c206c4311bc4d6e727f89243aea2b5139fca61
Bug 1305217: Part 2 - Run webNavigation listeners asynchronously. r=mixedpuppy
Backed out bug 1316914, bug 1316784 and bug 1305217 for frequent timeouts in test_ext_webrequest_upload.html:

Bug 1305217:
https://hg.mozilla.org/integration/mozilla-inbound/rev/217ae1d5285328611de99e2e48042b19751dbb54
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cbde9c5e058999ba95319391b59a7d67649ca53

Bug 1316784:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9381a00d19f81c21114f9d24cafee791fa0d9dbe

Bug 1316914:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddec56f2c469512ba99e424540f6f77a447fdea0

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=39104153&repo=mozilla-inbound

21:16:58     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test_ext_webrequest_upload.html | Binary upload size matches - Expected: 16, Actual: 16 
21:16:58     INFO - onCompleted 32 http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/file_WebRequest_page3.html?trigger=form&upload=16+bytes&enctype=multipart%2Fform-data&xhr=1
21:16:58     INFO - onBeforeRequest 33 http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/file_WebRequest_page3.html?trigger=form&upload=%7B%22textInput%22%3A%5B%22value1%22%2C%22value2%22%5D%7D&enctype=application%2Fx-www-form-urlencoded
21:16:58     INFO - onUpload http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/file_WebRequest_page3.html?trigger=form&upload=%7B%22textInput%22%3A%5B%22value1%22%2C%22value2%22%5D%7D&enctype=application%2Fx-www-form-urlencoded {"formData":{"textInput":["value1","value2"]}}
21:16:58     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test_ext_webrequest_upload.html | Intercepted upload http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/file_WebRequest_page3.html?trigger=form&upload=%7B%22textInput%22%3A%5B%22value1%22%2C%22value2%22%5D%7D&enctype=application%2Fx-www-form-urlencoded #33 {"textInput":["value1","value2"]} have a requestBody 
21:16:58     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test_ext_webrequest_upload.html | Upload http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/file_WebRequest_page3.html?trigger=form&upload=%7B%22textInput%22%3A%5B%22value1%22%2C%22value2%22%5D%7D&enctype=application%2Fx-www-form-urlencoded #33 matches form data. - Expected: {"textInput":["value1","value2"]}, Actual: {"textInput":["value1","value2"]} 
21:16:58     INFO - Buffered messages finished
21:16:58     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_ext_webrequest_upload.html | Test timed out. 
21:16:58     INFO -     reportError@SimpleTest/TestRunner.js:114:7
21:16:58     INFO -     TestRunner._checkForHangs@SimpleTest/TestRunner.js:135:7
Flags: needinfo?(kmaglione+bmo)
Also backed out the change without bug to make ESlint happy:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e2ec37ba69ffa5c78be6651815362d7c196c5f4

Message from ESlint was:
TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/components/extensions/ExtensionChild.jsm:536:5 | 'nextId' is assigned a value but never used. (no-unused-vars)
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b8b797d3583e2f4d968f64bfbde8e5dcf19bf68
Bug 1305217: Part 1 - Run webRequest listeners asynchronously. r=mixedpuppy

https://hg.mozilla.org/integration/mozilla-inbound/rev/c8980badcd5a40d474e31b41e84ba861c8301440
Bug 1305217: Part 2 - Run webNavigation listeners asynchronously. r=mixedpuppy
Flags: needinfo?(kmaglione+bmo)
https://hg.mozilla.org/mozilla-central/rev/2b8b797d3583
https://hg.mozilla.org/mozilla-central/rev/c8980badcd5a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Blocks: 1333141
Product: Toolkit → WebExtensions
Regressions: 1597107
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: