Closed
Bug 1305217
Opened 8 years ago
Closed 8 years ago
Make webRequest oop-compatible
Categories
(WebExtensions :: General, defect, P1)
Tracking
(firefox52 fixed)
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: robwu, Assigned: kmag)
References
(Blocks 3 open bugs)
Details
(Whiteboard: [webRequest] triaged)
Attachments
(2 files)
No description provided.
Updated•8 years ago
|
Priority: -- → P2
Whiteboard: [webRequest] triaged
Assignee | ||
Comment 1•8 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 6•8 years ago
|
||
mozreview-review |
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?
Assignee | ||
Comment 7•8 years ago
|
||
mozreview-review-reply |
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 8•8 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 11•8 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 12•8 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 13•8 years ago
|
||
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
![]() |
||
Comment 14•8 years ago
|
||
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)
![]() |
||
Comment 15•8 years ago
|
||
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)
Assignee | ||
Comment 16•8 years ago
|
||
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
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(kmaglione+bmo)
Comment 17•8 years ago
|
||
bugherder |
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
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•