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)

enhancement

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.
Priority: -- → P1
Depends on: 1450388
Attachment #8969839 - Flags: review?(kmaglione+bmo)
Attachment #8969840 - Flags: review?(mixedpuppy)
Attachment #8969840 - Flags: review?(kmaglione+bmo)
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...
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 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.
(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 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 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 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+
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
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)
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
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
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
https://hg.mozilla.org/mozilla-central/rev/50a1ddd35664
https://hg.mozilla.org/mozilla-central/rev/cbae9dd05293
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
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)
New try with several green TV runs:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffe25840c62adf8d81aacf4e2c4626561710e06a
Flags: needinfo?(aswan)
See Also: → 1457213
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)
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)
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?
From my duplicate Bugzilla report above, the issue still happens with webRequest not catching early events.
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.)
Status: RESOLVED → VERIFIED
Attached image unblocked-ad.png
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/
This bug is about network filters, what you see is handled by cosmetic (css) filtering.
Product: Toolkit → WebExtensions
>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?
See Also: → 1653237
See Also: → 1821676
You need to log in before you can comment on or make changes to this bug.