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

Status: VERIFIED FIXED

(WebExtensions :: General, enhancement, P1)

Description

[Andrew Swan [:aswan]]

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.

Comment 1

[Andrew Swan [:aswan]]

Attachment: Bug 1447551 Part 1: Fix some issues with persistent EventManagers

- Un-lazify the startup promises in ext-toolkit.js since the
  manifest background property is handled asynchronously, so it
  races with startup and can miss the relevant events if it
  loses the race.
- Ensure that persistent events don't cause breakage when the
  background-delayed-startup preference is set to false.
- Add a wakeup() method to the fire object provided to primed
  listeners.  This method returns a Promise that resolves when
  the extension background page has started.  Events that need to
  do some work in the context of the extension can wait on the
  result of wakeup(), then continue processing after the background
  page is started, using fire.[a?]sync as normal.

Review commit: https://reviewboard.mozilla.org/r/238678/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/238678/

Comment 2

[Andrew Swan [:aswan]]

Attachment: Bug 1447551 Part 2: Convert webRequest to persistent events

Review commit: https://reviewboard.mozilla.org/r/238680/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/238680/

Comment 3

[Shane Caraveo (:mixedpuppy)]

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 4

[Andrew Swan [:aswan]]

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

[Shane Caraveo (:mixedpuppy)]

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

[Shane Caraveo (:mixedpuppy)]

(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

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8969840 [details]
Bug 1447551 Part 2: Convert webRequest to persistent events

https://reviewboard.mozilla.org/r/238680/#review244698

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8969839 [details]
Bug 1447551 Part 1: Fix some issues with persistent EventManagers

https://reviewboard.mozilla.org/r/238678/#review244860

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8969840 [details]
Bug 1447551 Part 2: Convert webRequest to persistent events

https://reviewboard.mozilla.org/r/238680/#review244862

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

[Narcis Beleuzu [:NarcisB]]

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

Comment 12

[Pulsebot]

Backout by [e-mail]:
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

[Narcis Beleuzu [:NarcisB]]

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

[Narcis Beleuzu [:NarcisB]]

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 15

[Andrew Swan [:aswan]]

New try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=00e2d3b09e63bea16535464f988930e3275f324f

Comment 16

[Andrew Swan [:aswan]]

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

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/50a1ddd35664
https://hg.mozilla.org/mozilla-central/rev/cbae9dd05293

Comment 18

[Raul Gurzau (:RaulG)]

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

Comment 19

[Andrew Swan [:aswan]]

New try with several green TV runs:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffe25840c62adf8d81aacf4e2c4626561710e06a

Comment 20

[Andrew Swan [:aswan]]

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

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/49295192e8a8
https://hg.mozilla.org/mozilla-central/rev/129e90aed47a

Comment 22

[CosminB]

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!

Comment 23

[Andrew Swan [: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.

Comment 24

[swleefers]

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

[Adam]

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

Comment 27

[marius.santa]

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.)

Comment 28

[Kestrel]

Attachment: 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

[gwarser]

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

Comment 30

[TMart]

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