Closed Bug 1456677 Opened 2 years ago Closed 2 years ago

Make nsIBlocklistService a stub that defers to Blocklist.jsm

Categories

(Toolkit :: Blocklist Implementation, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

Details

(Whiteboard: [fxperf:p1])

Attachments

(1 file)

The current nsIBlocklistService setup isn't great for JS callers, which is probably reason enough to do this. The bigger problem, though, is that it's too easy to accidentally trigger loading the blocklist service just by touching it at the wrong time.

We can make the XPCOM service a stub, with an isLoaded property that returns false if the module hasn't been loaded, and other methods that queue requests until it has.
Comment on attachment 8970761 [details]
Bug 1456677: Make the blocklist service a JSM, with an XPCOM service stub.

https://reviewboard.mozilla.org/r/239514/#review245338

I'm probably wrong, but this looks incomplete to me? As far as I know, Services.blocklist still points to the actual service, and I don't see a Services.jsm change in here, but various places still call `loadBlocklistAsync` and the getPlugin{Info,Blocklist}URL methods on Services.blocklist [0], so they'll try to do that on the stub which won't work. Ditto for the getAddonBlocklistEntry callsite in XPIDatabase.jsm , and maybe others (see link).

[0] https://dxr.mozilla.org/mozilla-central/search?q=services.blocklist+-path%3Aall.js+-path%3Aobj-x86+-PREF+-Services.prefs+-onecrl.chec+-.STATE+-.isLoaded&redirect=false

::: toolkit/mozapps/extensions/test/xpcshell/test_blocklist_plugin_outdated.js:86
(Diff revision 1)
> +  // The blocklist service defers plugin request until the Blocklist
> +  // module loads. Make sure it loads, or we'll wait forever.
> +  executeSoon(() => {
> +    void Blocklist;
> +  });

Could this just `await Blocklist.loadBlocklistAsync()` instead, to make this more explicit?

(same comment for the other uses in the bits below)

::: toolkit/mozapps/extensions/test/xpcshell/test_update.js
(Diff revision 1)
> -  await promiseRestartManager();
> -

Curious, why remove this?
Attachment #8970761 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs (he/him) from comment #2)
> Comment on attachment 8970761 [details]
> Bug 1456677: Make the blocklist service a JSM, with an XPCOM service stub.
> 
> https://reviewboard.mozilla.org/r/239514/#review245338
> 
> I'm probably wrong, but this looks incomplete to me? As far as I know,
> Services.blocklist still points to the actual service, and I don't see a
> Services.jsm change in here, but various places still call
> `loadBlocklistAsync` and the getPlugin{Info,Blocklist}URL methods on
> Services.blocklist [0], so they'll try to do that on the stub which won't
> work. Ditto for the getAddonBlocklistEntry callsite in XPIDatabase.jsm , and
> maybe others (see link).

I missed some references in browser-blocklist.js that I fixed after my try run, but I think that was it.

The call in XPIDatabase.jsm I definitely fixed in this patch.

> toolkit/mozapps/extensions/test/xpcshell/test_blocklist_plugin_outdated.js:86
> (Diff revision 1)
> > +  // The blocklist service defers plugin request until the Blocklist
> > +  // module loads. Make sure it loads, or we'll wait forever.
> > +  executeSoon(() => {
> > +    void Blocklist;
> > +  });
> 
> Could this just `await Blocklist.loadBlocklistAsync()` instead, to make this
> more explicit?

It could be, but I wanted to test that loading Blocklist.jsm after a call to the stub function automatically resumed those calls.

> ::: toolkit/mozapps/extensions/test/xpcshell/test_update.js
> (Diff revision 1)
> > -  await promiseRestartManager();
> > -
> 
> Curious, why remove this?

It wasn't necessary anymore, and it no longer worked with the new mocking behavior.

I originally added the restart because the AOM code had its own cached blocklist service getter, which was stale after I replaced the service with a mock. That code changed a while ago to just use Services.blocklist, so the restart was no longer necessary.

With the new mock, though, we actually need to replace the Blocklist global on the module scope, so restarting the add-on manager actually undoes the mock, since it reloads those modules.
Comment on attachment 8970761 [details]
Bug 1456677: Make the blocklist service a JSM, with an XPCOM service stub.

https://reviewboard.mozilla.org/r/239514/#review245544

::: toolkit/mozapps/extensions/Blocklist.jsm:18
(Diff revisions 1 - 2)
> -try {
> -  // AddonManager.jsm doesn't allow itself to be imported in the child
> -  // process. We're used in the child process (for now), so guard against
> -  // this.
> +ChromeUtils.defineModuleGetter(this, "AddonManager",
> +                               "resource://gre/modules/AddonManager.jsm");
> +ChromeUtils.defineModuleGetter(this, "AddonManagerPrivate",
> +                               "resource://gre/modules/AddonManager.jsm");

I assume we're removing this because we trust we ourselves (ie the blocklist) doesnt' get used in the content process?
Attachment #8970761 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8970761 [details]
Bug 1456677: Make the blocklist service a JSM, with an XPCOM service stub.

https://reviewboard.mozilla.org/r/239514/#review245544

> I assume we're removing this because we trust we ourselves (ie the blocklist) doesnt' get used in the content process?

I don't entirely trust that (though it would probably be good to add an assertion at some point), but I more made this change because the eager AddonManager import was unnecessary, and if we hit code that tries to touch the AddonManager properties in the content process, it will throw one way or the other. Either because they're undefined, or because we refuse to load AddonManager.jsm in content processes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/35fe513112642f7d2f9c99da61538db193701a94
Bug 1456677: Make the blocklist service a JSM, with an XPCOM service stub. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/35fe51311264
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/069aa51eefe6
Port bug 1456677 - Remove nsBlocklistService.js from package manifests. rs=bustage-fix
Gijs and Kris, do we need to port anything else here? In this merge we get 10 Calendar Xpcshell failures, bug 1457087, but nothing that looks like blocklist related.:gijs
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(gijskruitbosch+bugs)
Let's take this to bug 1457087.
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(gijskruitbosch+bugs)
Thanks, but coming back to the blocklist, do we need to port more, like adding this
+  Services.tm.idleDispatchToMainThread(() => {
+    Blocklist.loadBlocklistAsyntc();
+  });
+
somewhere?
(In reply to Jorg K (GMT+1) from comment #12)
> Thanks, but coming back to the blocklist, do we need to port more, like
> adding this
> +  Services.tm.idleDispatchToMainThread(() => {
> +    Blocklist.loadBlocklistAsyntc();
> +  });
> +
> somewhere?

You shouldn't need to. The toolkit extensions component manifest ( https://dxr.mozilla.org/mozilla-central/rev/99c19a66c3a2fbf8108d4b8a161cded31e948409/toolkit/mozapps/extensions/extensions.manifest#4 ) defines a startup observer for the component that will start the blocklist. This patch forwards that to Blocklist.jsm, which in turn calls loadBlocklistAsync(). So nothing should have changed for toolkit apps.
(FWIW, this was done in https://bugzilla.mozilla.org/show_bug.cgi?id=1445990 explicitly for the purposes of supporting TB/SM and at the time I didn't hear any reports of breakage, so I'd be kind of surprised if this stubbing broke anything - functionally, nothing should have changed.)
Gijs, thanks for caring about other toolkit apps. When I landed the removal of nsBlocklistService.js from the package manifests, I was asking myself whether anything else needed doing. I expected some blocklist related test failures, but instead found the ones from bug 1457087.
Depends on: 1508979
Component: Blocklist Policy Requests → Blocklist Implementation
You need to log in before you can comment on or make changes to this bug.