Closed
Bug 1447551
Opened 6 years ago
Closed 6 years ago
Let webRequest listeners see/block requests from early after browser startup
Categories
(WebExtensions :: General, enhancement, P1)
WebExtensions
General
Tracking
(firefox61 verified)
VERIFIED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | verified |
People
(Reporter: aswan, Assigned: aswan)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
This is forked from bug 1378459, beginning around comment 33 on that bug is a description of the approach we're planning and some ensuing discussion.
Assignee | ||
Updated•6 years ago
|
Priority: -- → P1
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8969839 -
Flags: review?(kmaglione+bmo)
Attachment #8969840 -
Flags: review?(mixedpuppy)
Attachment #8969840 -
Flags: review?(kmaglione+bmo)
Comment 3•6 years ago
|
||
mozreview-review |
Comment on attachment 8969840 [details] Bug 1447551 Part 2: Convert webRequest to persistent events https://reviewboard.mozilla.org/r/238680/#review244670 In general this looks fine, but the traceable channel change bothers me. I'm not seeing an obvious reason that setting the traceable channel is a problem, but from the comment in the test it's obvious there is some problem. What was going on that required this? ::: toolkit/modules/addons/WebRequest.jsm:759 (Diff revision 1) > - channel.registerTraceableChannel(opts.extension, opts.tabParent); > + data.registerTraceableChannel = (extension, tabParent) => { > + channel.registerTraceableChannel(extension, tabParent); > + }; This change does break any existing standalone use of webrequest, and complicates use. So I'd like to better understand why this was necessary. The comment in the test alludes to a problem...
Assignee | ||
Comment 4•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8969840 [details] Bug 1447551 Part 2: Convert webRequest to persistent events https://reviewboard.mozilla.org/r/238680/#review244670 > This change does break any existing standalone use of webrequest, and complicates use. So I'd like to better understand why this was necessary. The comment in the test alludes to a problem... tabParent is essentially a reference to the extension's running background page, which of course does not exist before we have started the background page (that's the whole point of this bug). So the changes here provide the tabParent only after the background page has been started (but before any listeners that might call `filterResponseData()` have been run). As for other consumers of WebRequest.jsm, they would only break if they are setting `opts.extension` which is highly tailored to WebExtensions, if anybody is doing that maybe they deserve to break :)
Comment 5•6 years ago
|
||
mozreview-review |
Comment on attachment 8969840 [details] Bug 1447551 Part 2: Convert webRequest to persistent events https://reviewboard.mozilla.org/r/238680/#review244696 ::: toolkit/modules/addons/WebRequest.jsm:759 (Diff revision 1) > - channel.registerTraceableChannel(opts.extension, opts.tabParent); > + data.registerTraceableChannel = (extension, tabParent) => { > + channel.registerTraceableChannel(extension, tabParent); > + }; Can we make this optional? if tabParent registerTraceableChannel else data.regsterTraceableChanel = ... That should preserve the ability to test this module without the extra hoop.
Comment 6•6 years ago
|
||
(In reply to Andrew Swan [:aswan] from comment #4) > Comment on attachment 8969840 [details] > > As for > other consumers of WebRequest.jsm, they would only break if they are setting > `opts.extension` which is highly tailored to WebExtensions, if anybody is > doing that maybe they deserve to break :) Failed to finish reading and missed this point.
Comment 7•6 years ago
|
||
mozreview-review |
Comment on attachment 8969840 [details] Bug 1447551 Part 2: Convert webRequest to persistent events https://reviewboard.mozilla.org/r/238680/#review244698
Attachment #8969840 -
Flags: review?(mixedpuppy) → review+
Comment 8•6 years ago
|
||
mozreview-review |
Comment on attachment 8969839 [details] Bug 1447551 Part 1: Fix some issues with persistent EventManagers https://reviewboard.mozilla.org/r/238678/#review244860
Attachment #8969839 -
Flags: review?(kmaglione+bmo) → review+
Comment 9•6 years ago
|
||
mozreview-review |
Comment on attachment 8969840 [details] Bug 1447551 Part 2: Convert webRequest to persistent events https://reviewboard.mozilla.org/r/238680/#review244862
Attachment #8969840 -
Flags: review?(kmaglione+bmo) → review+
Comment 10•6 years ago
|
||
Pushed by aswan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b6f7de464d7f Part 1: Fix some issues with persistent EventManagers r=kmag https://hg.mozilla.org/integration/autoland/rev/82d178d9966d Part 2: Convert webRequest to persistent events r=kmag,mixedpuppy
Comment 11•6 years ago
|
||
Backed out for browser-chrome failures on browser_UITour_detach_tab.js and devtools failures on browser_service_workers_not_compatible.js. Push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=82d178d9966ddd126ec1cf3c0ff90173c9ad4a80 BC log: https://treeherder.mozilla.org/logviewer.html#?job_id=175382085&repo=autoland&lineNumber=23292 DT1 failure on "browser_service_workers_not_compatible.js" Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=175382077&repo=autoland&lineNumber=3257 DT6 failure on "/browser_menu_item_02.js" Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=175382082&repo=autoland&lineNumber=17779
Flags: needinfo?(aswan)
Comment 12•6 years ago
|
||
Backout by nbeleuzu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9d3390130f27 Backed out 2 changesets for browser-chrome failures on browser_UITour_detach_tab.js and devtools failures on browser_service_workers_not_compatible.js. CLOSED TREE
Comment 13•6 years ago
|
||
Also there were browser chrome failures on "browser/components/extensions/test/browser/browser_ext_browserAction_context.js" Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=175381928&repo=autoland&lineNumber=23109
Comment 14•6 years ago
|
||
Also browser chrome failures on browser_policy_searchbar.js and browser_ext_themes_chromeparity.js Log: https://treeherder.mozilla.org/logviewer.html#?job_id=175393415&repo=autoland&lineNumber=11582
Assignee | ||
Comment 15•6 years ago
|
||
New try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=00e2d3b09e63bea16535464f988930e3275f324f
Flags: needinfo?(aswan)
Assignee | ||
Comment 16•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/50a1ddd35664449c908ea326b364d367871e78a9 Bug 1447551 Part 1: Fix some issues with persistent EventManagers r=kmag https://hg.mozilla.org/integration/mozilla-inbound/rev/cbae9dd052934b6292f8e8675eb6cf7cde78333d Bug 1447551 Part 2: Convert webRequest to persistent events r=mixedpuppy,kmag
Comment 17•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/50a1ddd35664 https://hg.mozilla.org/mozilla-central/rev/cbae9dd05293
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 18•6 years ago
|
||
Backed out 2 changesets (bug 1447551) for failing on xpcshell/test_ext_webRequest_startup.js Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/e19dc6140f5c4108fddddee62c9a977b97f08054 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=cbae9dd052934b6292f8e8675eb6cf7cde78333d Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=175499363&repo=mozilla-inbound&lineNumber=11209 Log snippet: 09:33:18 INFO - TEST-START | xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_webRequest_startup.js 09:33:19 WARNING - TEST-UNEXPECTED-FAIL | xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_webRequest_startup.js | xpcshell return code: 0
Flags: needinfo?(aswan)
Assignee | ||
Comment 19•6 years ago
|
||
New try with several green TV runs: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffe25840c62adf8d81aacf4e2c4626561710e06a
Flags: needinfo?(aswan)
Assignee | ||
Comment 20•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/49295192e8a854f21854f70d658f04cb51e8aa36 Bug 1447551 Part 1: Fix some issues with persistent EventManagers r=kmag https://hg.mozilla.org/integration/mozilla-inbound/rev/129e90aed47a91b44a0b8ac3b3c44b0b8b9c72ca Bug 1447551 Part 2: Convert webRequest to persistent events r=mixedpuppy,kmag
Comment 21•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/49295192e8a8 https://hg.mozilla.org/mozilla-central/rev/129e90aed47a
Comment 22•6 years ago
|
||
Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag. Thanks!
Flags: needinfo?(aswan)
Assignee | ||
Comment 23•6 years ago
|
||
Delayed background page startup certainly needs testing, that's broader than this specific bug. For just the webRequest related bits here, we have automated test coverage, but testing extensions that use webRequest (eg ad blockers, privacy-related extensions) during startup would be good. I know that's a little vague but its a pretty low level change and not easy to reduce to some specific concrete scenario.
Flags: needinfo?(aswan)
Comment 24•6 years ago
|
||
Would it also be relevant to test userscript extensions, like Greasemonkey? I'm a total layman, but those extensions might also want to block loading until something from a userscript has happened?
Comment 26•6 years ago
|
||
From my duplicate Bugzilla report above, the issue still happens with webRequest not catching early events.
Comment 27•6 years ago
|
||
Tested and verified on Firefox 61.0b13 (20180611134439) in Windows 10 64Bit and MacOS 10.13.4. Tested startup scenarios with the most popular Privacy & Security extensions(Adblock Plus, uBlock Origin , Avast Online Security, LastPass Password Manager, etc.)
Updated•6 years ago
|
Status: RESOLVED → VERIFIED
Comment 28•6 years ago
|
||
Still occasionally seeing ads appear on reddit when Nightly 62.0a1 (2018-06-13) starts up despite having uBlock Origin installed. https://old.reddit.com/r/all/
Comment 29•6 years ago
|
||
This bug is about network filters, what you see is handled by cosmetic (css) filtering.
Updated•6 years ago
|
Product: Toolkit → WebExtensions
Comment 30•6 years ago
|
||
>This bug is about network filters, what you see is handled by cosmetic (css) filtering.
Does this mean content like trackers might be displayed, but won't be able to send any network requests? Are there plans to allow cosmetic filtering early at browser startup?
You need to log in
before you can comment on or make changes to this bug.
Description
•