Closed
Bug 1443870
Opened 6 years ago
Closed 6 years ago
Remove content process implementation of nsIBlocklistService
Categories
(Toolkit :: Blocklist Policy Requests, enhancement)
Toolkit
Blocklist Policy Requests
Tracking
()
RESOLVED
FIXED
mozilla61
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.
Assignee | ||
Updated•6 years ago
|
Whiteboard: [fxperf:p2]
Comment hidden (mozreview-request) |
Comment 2•6 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 3•6 years ago
|
||
(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)
Assignee | ||
Comment 4•6 years ago
|
||
(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...
Comment hidden (mozreview-request) |
Comment 6•6 years ago
|
||
(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 7•6 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 8•6 years ago
|
||
mozreview-review-reply |
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.
Comment hidden (mozreview-request) |
Comment 10•6 years ago
|
||
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
Comment 11•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1705ac540472 https://hg.mozilla.org/mozilla-central/rev/e3af0d59c8f8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 12•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/2ba05f9a5032 https://hg.mozilla.org/releases/mozilla-beta/rev/3f2e1e8f3d63
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•