Closed Bug 1445990 Opened 6 years ago Closed 6 years ago

Make blocklist initialization more sane

Categories

(Toolkit :: Blocklist Implementation, enhancement)

Product:
Toolkit
Component:
Blocklist Implementation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1445990 - fix initialization of blocklist service in non-browser apps,
59 bytes, text/x-review-board-request
florian
: review+
Details 
In bug 1371888, florian pointed out that:

(In reply to Florian Quèze [:florian] from comment #6)
> sessionstore-windows-restored is a notification coming from browser/ that
> nsBlocklistService.js depends on. That doesn't sound great (ie. I wonder
> what starts the blocklist service for Thunderbird). But given there's
> already a dependency here, I think there would be no problem starting the
> blocklist service explicitly from nsBrowserGlue, so I would do it around
> https://searchfox.org/mozilla-central/rev/
> 8fa0b32c84f924c6809c690117dbd59591f79607/browser/components/nsBrowserGlue.
> js#1146 within an idle callback.

We should:

1) do this (ie move initialization directly into nsBrowserGlue) and thus make things more sane for Firefox.
2) figure out what other consumers of the blocklists are supposed to do to ensure they load the blocklist and update plugins.

At the moment I expect initialization will be broken on Thunderbird, Android and anything else that isn't desktop Firefox. We should address that. Even if I don't think we support *any* plugins on Android (not even Flash anymore, AIUI?) and I would be confused if plugin support mattered for Thunderbird.

What's the latest-in-startup point that exists in toolkit, rather than just in browser/? Florian, do you have ideas?
Flags: needinfo?(florian)
(In reply to :Gijs from comment #0)
> In bug 1371888, florian pointed out that

Err, copy-paste failure, I meant bug 1443870.
(In reply to :Gijs from comment #0)

> What's the latest-in-startup point that exists in toolkit, rather than just
> in browser/? Florian, do you have ideas?

Are you looking for something that could be used for non-Firefox applications, or that could be used in all cases?

I don't think startup performance matters to non-Firefox applications to the point of caring about when the blocklist service starts its async initialization.

So here's one possible way:
- for Firefox, ifdef out the profile-after-change line in the manifest
- for Firefox, load the blocklist service from nsBrowserGlue
- from the blocklist service js file, never wait for the sessionrestore notification, just start the async load when the service is first created.
Flags: needinfo?(florian)
Comment on attachment 8959194 [details]
Bug 1445990 - fix initialization of blocklist service in non-browser apps,

https://reviewboard.mozilla.org/r/228080/#review233958

Looks good to me, thanks!

::: toolkit/mozapps/extensions/extensions.manifest:4
(Diff revision 1)
>  component {66354bc9-7ed1-4692-ae1d-8da97d6b205e} nsBlocklistService.js process=main
>  contract @mozilla.org/extensions/blocklist;1 {66354bc9-7ed1-4692-ae1d-8da97d6b205e} process=main
> +#ifndef MOZ_BUILD_APP_IS_BROWSER
>  category profile-after-change nsBlocklistService @mozilla.org/extensions/blocklist;1  process=main

It's tempting to fix the double space here while we touch this.

::: toolkit/mozapps/extensions/nsBlocklistService.js:786
(Diff revision 1)
>      this._addonEntries = null;
>      this._gfxEntries = null;
>      this._pluginEntries = null;
>    },
>  
> -  async _preloadBlocklist() {
> +  async loadBlocklistAsync() {

You need to update toolkit/mozapps/extensions/test/xpcshell/test_asyncBlocklistLoad.js with this rename.
Attachment #8959194 - Flags: review?(florian) → review+
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3b82709eca8a
fix initialization of blocklist service in non-browser apps, r=florian
Backed out changeset 3b82709eca8a (bug 1445990) for xpcshell failures at js/xpconnect/tests/unit/test_defineModuleGetter.js on a CLOSED TREE

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=3b82709eca8a87fafb5c4aef4253baba2ea2255d

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=168373528&repo=autoland&lineNumber=1624

Backout: https://hg.mozilla.org/integration/autoland/rev/d38ac5fa34a02e8b91766e74e72b6c0ba40cc324
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Cristina Coroiu [:ccoroiu] from comment #7)
> Backed out changeset 3b82709eca8a (bug 1445990) for xpcshell failures at
> js/xpconnect/tests/unit/test_defineModuleGetter.js on a CLOSED TREE
> 
> Failure push:
> https://treeherder.mozilla.org/#/
> jobs?repo=autoland&revision=3b82709eca8a87fafb5c4aef4253baba2ea2255d
> 
> Failure log:
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=168373528&repo=autoland&lineNumber=1624
> 
> Backout:
> https://hg.mozilla.org/integration/autoland/rev/
> d38ac5fa34a02e8b91766e74e72b6c0ba40cc324


[10:58:16]	Gijs_away	and as far as I can tell it's basically a test that is meant to check that Services.jsm loading on some object works
[10:58:18]	Gijs_away	and it does
[10:58:43]	Gijs_away	but because Assert.jsm is clownshoes, it tries to construct a message for the assertion that stringifies both the expected and actual value, even if the test passes
[10:58:57]	Gijs_away	which means it tries to stringify the Services object
[10:59:03]	Gijs_away	which means it eagerly resolves every single getter
[10:59:09]	Gijs_away	in an xpcshell test that has no profile
[10:59:34]	Gijs_away	and I introduced code that triggers a load of a file that's supposed to be in the profile in a service that's on Services.jsm
[10:59:47]	Gijs_away	which triggers an uncaught promise rejection.
[11:01:06]	Gijs_away	only on android though!



It does point out a legitimate bug, which is that if someone tries to instantiate the service before profile-after-change, things go pearshaped. I'm adding a fix for this.
Flags: needinfo?(gijskruitbosch+bugs)
I'll file some bugs to update the test and fix Assert.jsm so this stuff doesn't happen in future.
Comment on attachment 8959542 [details]
Bug 1445990 - delay lazy loading if we happen to be loaded prior to profile-after-change,

https://reviewboard.mozilla.org/r/228342/#review234238

::: toolkit/mozapps/extensions/nsBlocklistService.js:260
(Diff revision 1)
>    _gfxEntries: null,
>    _pluginEntries: null,
>  
>    shutdown() {
>      Services.obs.removeObserver(this, "xpcom-shutdown");
>      Services.ppmm.removeMessageListener("Blocklist:content-blocklist-updated", this);

Did you mean to also remove this in your previous patch?

::: toolkit/mozapps/extensions/nsBlocklistService.js:798
(Diff revision 1)
> +    try {
> +      // Getting the profile path can fail if we're called before there's a profile.
> +      profPath = OS.Path.join(OS.Constants.Path.profileDir, FILE_BLOCKLIST);
> +    } catch (ex) {
> +      // Wait until we have a profile and don't try to load now.
> +      Services.obs.addObserver(this, "profile-after-change");

Is it a problem that this observer is never removed?

Is this for xpcshell tests (if so, isn't the category entry in the manifest enough to make us get a profile-after-change observer notification even if the service is already initialized?), or do you worry that this may happen for Firefox (if so, should we just make it fail loudly if it ever happens?)
(In reply to Florian Quèze [:florian] from comment #11)
> Comment on attachment 8959542 [details]
> Bug 1445990 - delay lazy loading if we happen to be loaded prior to
> profile-after-change,
> 
> https://reviewboard.mozilla.org/r/228342/#review234238
> 
> ::: toolkit/mozapps/extensions/nsBlocklistService.js:260
> (Diff revision 1)
> >    _gfxEntries: null,
> >    _pluginEntries: null,
> >  
> >    shutdown() {
> >      Services.obs.removeObserver(this, "xpcom-shutdown");
> >      Services.ppmm.removeMessageListener("Blocklist:content-blocklist-updated", this);
> 
> Did you mean to also remove this in your previous patch?

Ugh, yes, fixed, good catch.

> ::: toolkit/mozapps/extensions/nsBlocklistService.js:798
> (Diff revision 1)
> > +    try {
> > +      // Getting the profile path can fail if we're called before there's a profile.
> > +      profPath = OS.Path.join(OS.Constants.Path.profileDir, FILE_BLOCKLIST);
> > +    } catch (ex) {
> > +      // Wait until we have a profile and don't try to load now.
> > +      Services.obs.addObserver(this, "profile-after-change");
> 
> Is it a problem that this observer is never removed?

It's probably not a big problem either way, but I should have added the removeObserver call in the observe() itself, so that's fixed now.

> Is this for xpcshell tests

No, for that the catch is enough.

> (if so, isn't the category entry in the manifest
> enough to make us get a profile-after-change observer notification even if
> the service is already initialized?)

This is really interesting. I assumed not, and just submitted a patch based on that assumption. I would expect the category manager to just instantiate the component, nothing else, but the MDN docs actually say that the observe() method also gets called.

> , or do you worry that this may happen
> for Firefox (if so, should we just make it fail loudly if it ever happens?)

No, I'm not worried about that - we don't call loadBlocklistAsync in that case, and nsBrowserGlue is guaranteed to call it at the 'right' time.

In any case, I added this only for the benefit of TB/SM/whatever, but it seems like based on the observe() already being called by catman, I can considerably simplify the patch. Another revision incoming after the one I just sent, sorry...
Attachment #8959542 - Attachment is obsolete: true
Attachment #8959542 - Flags: review?(florian)
Comment on attachment 8959194 [details]
Bug 1445990 - fix initialization of blocklist service in non-browser apps,

Re-requesting review through bugzilla...
Attachment #8959194 - Flags: review+ → review?(florian)
Comment on attachment 8959194 [details]
Bug 1445990 - fix initialization of blocklist service in non-browser apps,

Looks good to me, thanks!
Attachment #8959194 - Flags: review?(florian) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5e2d47159461
fix initialization of blocklist service in non-browser apps, r=florian
See Also: → 1446554
See Also: → 1446558
https://hg.mozilla.org/mozilla-central/rev/5e2d47159461
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Component: Blocklist Policy Requests → Blocklist Implementation
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: