Closed Bug 1443870 Opened 6 years ago Closed 6 years ago

Remove content process implementation of nsIBlocklistService

Categories

(Toolkit :: Blocklist Policy Requests, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

(Whiteboard: [fxperf:p2])

Attachments

(2 files)

After bug 1439519 and bug 1371888 I believe we can just remove the content-process implementation of the blocklist service. We should do that.
Whiteboard: [fxperf:p2]
Blocks: 1425602
Comment on attachment 8958782 [details]
Bug 1443870 - remove content-process blocklist service,

https://reviewboard.mozilla.org/r/227684/#review233466

::: toolkit/mozapps/extensions/extensions.manifest:3
(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
>  category profile-after-change nsBlocklistService @mozilla.org/extensions/blocklist;1  process=main

Is there a reason for the blocklist service to (still) be registered on the profile-after-change category for startup?
Attachment #8958782 - Flags: review?(florian) → review+
(In reply to Florian Quèze [:florian] from comment #2)
> Comment on attachment 8958782 [details]
> Bug 1443870 - remove content-process blocklist service,
> 
> https://reviewboard.mozilla.org/r/227684/#review233466
> 
> ::: toolkit/mozapps/extensions/extensions.manifest:3
> (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
> >  category profile-after-change nsBlocklistService @mozilla.org/extensions/blocklist;1  process=main
> 
> Is there a reason for the blocklist service to (still) be registered on the
> profile-after-change category for startup?

Well, it needs to register or be instantiated for sessionstore-windows-restored . The category manager doesn't support that directly. If there is a later notification we can use, that'd wfm, but I'm not sure which would be best. Do you know? Also, technically orthogonal to this removal. ;-)

Separately, it seems this is failing a test on try...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8af472275e3c90dbf8a7055f372062dcb0347ff8&selectedJob=167926840

so I'll push a cset to address that in a second. Fortunately, we can address it by what is basically only removal...
Flags: needinfo?(florian)
(In reply to :Gijs from comment #3)
> (In reply to Florian Quèze [:florian] from comment #2)
> > Comment on attachment 8958782 [details]
> > Bug 1443870 - remove content-process blocklist service,
> > 
> > https://reviewboard.mozilla.org/r/227684/#review233466
> > 
> > ::: toolkit/mozapps/extensions/extensions.manifest:3
> > (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
> > >  category profile-after-change nsBlocklistService @mozilla.org/extensions/blocklist;1  process=main
> > 
> > Is there a reason for the blocklist service to (still) be registered on the
> > profile-after-change category for startup?
> 
> Well, it needs to register or be instantiated for
> sessionstore-windows-restored . The category manager doesn't support that
> directly. If there is a later notification we can use

Err, later than profile-after-change, that is. But earlier than sessionstore-windows-restored, of course...
(In reply to :Gijs from comment #3)
> (In reply to Florian Quèze [:florian] from comment #2)
> > >  category profile-after-change nsBlocklistService @mozilla.org/extensions/blocklist;1  process=main
> > 
> > Is there a reason for the blocklist service to (still) be registered on the
> > profile-after-change category for startup?
> 
> Well, it needs to register or be instantiated for
> sessionstore-windows-restored . The category manager doesn't support that
> directly. If there is a later notification we can use, that'd wfm, but I'm
> not sure which would be best. Do you know? Also, technically orthogonal to
> this removal. ;-)

Yeah, totally something for another bug. I just mentioned it as I was looking at this file to review the patch here.

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.
Flags: needinfo?(florian)
Comment on attachment 8958955 [details]
Bug 1443870 - update tests for lack of content-process blocklist,

https://reviewboard.mozilla.org/r/227802/#review233624

::: browser/base/content/test/plugins/browser_blocking.js:38
(Diff revision 1)
>    // Prime the content process
>    await promiseTabLoadEvent(gBrowser.selectedTab, "data:text/html,<html>hi</html>");
>  
>    // Make sure the blocklist service(s) are running
>    // eslint-disable-next-line no-unused-expressions
>    Services.blocklist;

This line is probably not doing what it's trying to do. Is it hoping to synchronously load the blocklist here?
Attachment #8958955 - Flags: review?(florian) → review+
Comment on attachment 8958955 [details]
Bug 1443870 - update tests for lack of content-process blocklist,

https://reviewboard.mozilla.org/r/227802/#review233624

> This line is probably not doing what it's trying to do. Is it hoping to synchronously load the blocklist here?

I think it's just confused. I've removed it.
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1705ac540472
remove content-process blocklist service, r=florian
https://hg.mozilla.org/integration/autoland/rev/e3af0d59c8f8
update tests for lack of content-process blocklist, r=florian
https://hg.mozilla.org/mozilla-central/rev/1705ac540472
https://hg.mozilla.org/mozilla-central/rev/e3af0d59c8f8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: