Let webRequest listeners see/block requests from early after browser startup

VERIFIED FIXED in Firefox 61

Status

enhancement
P1
normal
VERIFIED FIXED
a year ago
6 months ago

People

(Reporter: aswan, Assigned: aswan)

Tracking

(Blocks 3 bugs)

unspecified
mozilla61
Dependency tree / graph

Firefox Tracking Flags

(firefox61 verified)

Details

Attachments

(3 attachments)

(Assignee)

Description

a year ago
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

a year ago
Priority: -- → P1
(Assignee)

Updated

a year ago
Depends on: 1450388
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8969839 - Flags: review?(kmaglione+bmo)
Attachment #8969840 - Flags: review?(mixedpuppy)
Attachment #8969840 - Flags: review?(kmaglione+bmo)

Comment 3

a year 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

a year 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

a year 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.
(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

a year 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

a year 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

a year 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

a year 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
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

a year 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
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

Comment 17

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/50a1ddd35664
https://hg.mozilla.org/mozilla-central/rev/cbae9dd05293
Status: NEW → RESOLVED
Last Resolved: a year 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)
(Assignee)

Comment 19

a year ago
New try with several green TV runs:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffe25840c62adf8d81aacf4e2c4626561710e06a
Flags: needinfo?(aswan)
See Also: → 1457213

Comment 22

a year 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

a year 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

a year 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?

Updated

11 months ago
Duplicate of this bug: 1461090

Comment 26

11 months ago
From my duplicate Bugzilla report above, the issue still happens with webRequest not catching early events.

Comment 27

10 months 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

10 months ago
Status: RESOLVED → VERIFIED

Comment 28

10 months ago
Posted 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/

Comment 29

10 months ago
This bug is about network filters, what you see is handled by cosmetic (css) filtering.

Updated

10 months ago
Product: Toolkit → WebExtensions

Comment 30

10 months 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?
(Assignee)

Updated

6 months ago
Duplicate of this bug: 1501159
You need to log in before you can comment on or make changes to this bug.