Consider removing nsContentBlocker

RESOLVED WONTFIX

Status

()

enhancement
RESOLVED WONTFIX
2 years ago
2 years ago

People

(Reporter: Nika, Assigned: Ehsan)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

nsContentBlocker is a in-tree content policy which uses permissions (such as "document") to control whether a particular document, image, etc. should be blocked.

This unfortunately meant that in bug 1353179 I had to add a workaround to send these permissions used by nsContentBlocker to the content process at startup.

As far as I know we currently provide no UI for nsContentBlocker permissions or preferences, so it may be worthwhile to remove the feature entirely if it is not used. We may want to ship telemetry to check for whether nsContentBlocker permissions are used.
Assignee

Updated

2 years ago
Blocks: 1347376
Assignee

Comment 1

2 years ago
I doubt that adding a telemetry here can give us really actionable information.  I think we will definitely find some users out there that have some permissions like this set.  The question to answer is whether we want to continue supporting disallowing the load of arbitrary content types like this.

I think we need to keep 

My proposal is to just lift the handling of permissions.default.image into imgLoader, and remove support for the permissions to control loading of all other content types and remove nsContentBlocker completely.

Boris, Olli, what do you guys think?
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bugs)
Assignee

Comment 2

2 years ago
(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #1)
> I think we need to keep 

Runaway sentence that needs to be ignored.  :-)
I agree with comment 1. (except "I think we need to keep"  :) )
Given that image is the only case FF UI uses, moving that pref check elsewhere makes sense.

addons/485200/content/options.xul does seem to use also other prefs than just .image.
but https://addons.mozilla.org/en-US/firefox/addon/block-content/ hasn't been updated for ages.
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #3)
> I agree with comment 1. (except "I think we need to keep"  :) )
> Given that image is the only case FF UI uses, moving that pref check
> elsewhere makes sense.
> 
> addons/485200/content/options.xul does seem to use also other prefs than
> just .image.
> but https://addons.mozilla.org/en-US/firefox/addon/block-content/ hasn't
> been updated for ages.

Those prefs are easy enough to support if we needed to, although getting rid of them would be nicer. The problematic part for the permission manager is the "document" etc. per-origin permissions. 

I took a quick look at the reviews on block-content and it seems that people mostly like the ability to block the loading of videos on sites like facebook - but that feature isn't even implemented using nsContentBlocker, so I don't think we have to worry about removing it too much.
I'm fine with keeping permissions.default.image and nixing the rest.
Flags: needinfo?(bzbarsky)
Assignee

Updated

2 years ago
Assignee: nobody → ehsan
Assignee

Comment 7

2 years ago
Attachment #8861397 - Flags: review?(bzbarsky)
Comment on attachment 8861396 [details] [diff] [review]
Part 1: Move the handling of the permissions.default.image pref to imgLoader.cpp

This doesn't look right, because for images the exact _reason_ (REJECT_TYPE vs REJECT_SERVER) the load failed matters, in a highly user-visible way.  See the various discussion in bug 1267075 about trying to get this right in the AsyncOpen2 world.  I suspect that doing an error return like this gets absolutely none of that right...  I wish we had tests for this stuff.  :(

This check really needs to be done somewhere where it will interact correctly with the rest of the image loading.

Also, this is dropping support for the BEHAVIOR_NOFOREIGN value, right?
Attachment #8861396 - Flags: review?(bzbarsky) → review-
Comment on attachment 8861397 [details] [diff] [review]
Part 2: Remove nsContentBlocker

Fwiw, I think we should keep this as a content policy and just simplify it.  We could rename to nsImageBlocker if we want...  But the complexity of trying to move this off nsIContentPolicy, and especially the interactions of that with bug 1267075, are probably not worth it.
Attachment #8861397 - Flags: review?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #9)
> Fwiw, I think we should keep this as a content policy and just simplify it. 
> We could rename to nsImageBlocker if we want...  But the complexity of
> trying to move this off nsIContentPolicy, and especially the interactions of
> that with bug 1267075, are probably not worth it.

I'm inclined to agree with this - I would actually leave most things the way they are with this module, just removing the permissions from the content blocker, so that we don't need to send them down eagerly in nsPermissionManager.cpp. I don't see any particular reason to remove the entire class unless the fact that we always load a content policy is causing visible hitches in profiles.
Assignee

Comment 11

2 years ago
(In reply to Michael Layzell [:mystor] from comment #10)
> (In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no
> decent message, automatic r-) from comment #9)
> > Fwiw, I think we should keep this as a content policy and just simplify it. 
> > We could rename to nsImageBlocker if we want...  But the complexity of
> > trying to move this off nsIContentPolicy, and especially the interactions of
> > that with bug 1267075, are probably not worth it.
> 
> I'm inclined to agree with this - I would actually leave most things the way
> they are with this module, just removing the permissions from the content
> blocker, so that we don't need to send them down eagerly in
> nsPermissionManager.cpp. I don't see any particular reason to remove the
> entire class unless the fact that we always load a content policy is causing
> visible hitches in profiles.

Well the class itself is certainly doomed in many ways, it sits in extensions/permissions, its purpose is to block content based on permissions.  I think having a new simpler content policy line ImageBlocker in imagelib is less questionable.

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #8)
> Comment on attachment 8861396 [details] [diff] [review]
> Part 1: Move the handling of the permissions.default.image pref to
> imgLoader.cpp
> 
> This doesn't look right, because for images the exact _reason_ (REJECT_TYPE
> vs REJECT_SERVER) the load failed matters, in a highly user-visible way. 
> See the various discussion in bug 1267075 about trying to get this right in
> the AsyncOpen2 world.  I suspect that doing an error return like this gets
> absolutely none of that right...  I wish we had tests for this stuff.  :(

You're right.  I suspect we want REJECT_TYPE here only, right?

> This check really needs to be done somewhere where it will interact
> correctly with the rest of the image loading.

Just to verify that I understand the suggestion correct, that would be a content policy, right?

> Also, this is dropping support for the BEHAVIOR_NOFOREIGN value, right?

Yeah.  That is an value that gets ported from the ancient network.image.imageBehavior pref <http://searchfox.org/mozilla-central/rev/baf47b352e873d4516d7656186d6d7c7447d3873/extensions/permissions/nsContentBlocker.cpp#109> from ~2004 era.  The Firefox UI for image blocking only ever exposed the accept/reject options, so I think dropping support for BEHAVIOR_NOFOREIGN is fine.
> I think having a new simpler content policy line ImageBlocker in imagelib is less questionable.

Sure.

> You're right.  I suspect we want REJECT_TYPE here only, right?

That seems correct.

> Just to verify that I understand the suggestion correct, that would be a content policy, right?

At the moment, that's the simplest thing to do, yes.
Assignee

Updated

2 years ago
Flags: needinfo?(ehsan)
Assignee

Updated

2 years ago
Attachment #8861396 - Attachment is obsolete: true
Flags: needinfo?(ehsan)
Assignee

Comment 14

2 years ago
Comment on attachment 8861397 [details] [diff] [review]
Part 2: Remove nsContentBlocker

I ended up adding a new cleaner content policy specifically for this to imagelib.
Attachment #8861397 - Flags: review?(bzbarsky)
Comment on attachment 8861397 [details] [diff] [review]
Part 2: Remove nsContentBlocker

>+Bug 1357107 - Removing a directory from extensions/ requires re-running configure to update MOZ_EXTESIONS

MOZ_EXTENSIONS
Attachment #8861397 - Flags: review?(bzbarsky) → review+
Comment on attachment 8862267 [details] [diff] [review]
Part 1: Move the handling of the permissions.default.image pref to imgLoader.cpp

>+ImageBlocker::ShouldLoad(uint32_t aContentType,

This will start blocking chrome:// image loads from non-system-principal about: documents and the like.  We probably don't wan that.

In other words, you should probably keep the scheme check for ftp/http/https.

>+ImageBlocker::ShouldProcess(uint32_t aContentType,

This should call ShouldLoad, not just return ACCEPT.

r=me with those bits fixed.
Attachment #8862267 - Flags: review?(bzbarsky) → review+

Comment 17

2 years ago
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ed481306bc0
Part 1: Move the handling of the permissions.default.image pref to imgLoader.cpp; r=bzbarsky
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9618183e882
Part 2: Remove nsContentBlocker; r=bzbarsky

Comment 18

2 years ago
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/27176f58992c
follow-up: replace some run-away tab characters

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9ed481306bc0
https://hg.mozilla.org/mozilla-central/rev/d9618183e882
https://hg.mozilla.org/mozilla-central/rev/27176f58992c
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

2 years ago
Depends on: 1362299
Assignee

Comment 20

2 years ago
Yikes...  We actually have UI to expose this... :-(  See bug 1362299.

Should I back out?
Flags: needinfo?(bzbarsky)
Assignee

Comment 21

2 years ago
(Hah! and a bugzilla component for it too, for that matter!)
Michael, were all these permissions a problem, or just the ones for "document"?

That is, could we remove the non-image bits but restore the permissions checks for images and still get what we wanted out of this bug?
Flags: needinfo?(bzbarsky) → needinfo?(michael)
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #22)
> Michael, were all these permissions a problem, or just the ones for
> "document"?
> 
> That is, could we remove the non-image bits but restore the permissions
> checks for images and still get what we wanted out of this bug?

I think that the current workaround which I have written for the permission manager is OK. I simply count the permissions used by nsContentBlocker as permissions which need to be sent down at startup for all domains. It should not impact anyone who doesn't use this feature at all, but ensures that the feature keeps working.

We have 2 basic options here, either:
A) we teach the new system about the "image" permission, and continue to send down the "image" permission at startup, but lose the permissions for other types like "document", or
B) we back this out and leave things the way they are.

Both of these options just leave in place the workaround which sends down some permissions to the child process at startup, so I don't really see a benefit to removing the non-image permissions unless this new code is more maintainable or considerably faster, so I would be inclined to say we just back this out.

This is the place where we define the permissions which need to be loaded at startup:
http://searchfox.org/mozilla-central/rev/6580dd9a4eb0224773897388dba2ddf5ed7f4216/extensions/cookie/nsPermissionManager.cpp#87-122
Flags: needinfo?(michael)
Assignee

Comment 24

2 years ago
The problem that *I* was trying to address was that checking these permissions is needlessly expensive and I wanted to try to not have to do it at all.  If we have to do the work then we have to do it, I guess.  I'm not aware of any other problem this bug was intended to solve?
(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #24)
> The problem that *I* was trying to address was that checking these
> permissions is needlessly expensive and I wanted to try to not have to do it
> at all.  If we have to do the work then we have to do it, I guess.  I'm not
> aware of any other problem this bug was intended to solve?

I'm not either. This code is on a hot path, however (it happens every resource load), so we might want to put in the effort to optimize it a bit more. It shouldn't be too hard to have a static flag which is set whenever any of the nsContentBlocker permissions are present in the content process, and we could check that flag before talking to the permission manager, which might save us some time when doing resource loads, especially because the vast majority of people will never set a single nsContentBlocker permission. What do you think?
Flags: needinfo?(ehsan)
Assignee

Comment 26

2 years ago
(In reply to Michael Layzell [:mystor] from comment #25)
> (In reply to :Ehsan Akhgari (super long backlog, slow to respond) from
> comment #24)
> > The problem that *I* was trying to address was that checking these
> > permissions is needlessly expensive and I wanted to try to not have to do it
> > at all.  If we have to do the work then we have to do it, I guess.  I'm not
> > aware of any other problem this bug was intended to solve?
> 
> I'm not either. This code is on a hot path, however (it happens every
> resource load), so we might want to put in the effort to optimize it a bit
> more. It shouldn't be too hard to have a static flag which is set whenever
> any of the nsContentBlocker permissions are present in the content process,
> and we could check that flag before talking to the permission manager, which
> might save us some time when doing resource loads, especially because the
> vast majority of people will never set a single nsContentBlocker permission.
> What do you think?

That would work as a last resort.  But IIRC the specific thing that showed up in the profile was the fact that we were creating a principal out of a URL somewhere and that was the expensive part of the permission check!  I think I should back this out, and then we should reprofile and first try to see if we can somehow make that code fast.  The details of the profile escape me right now, I profiled this a few week ago.
Flags: needinfo?(ehsan)
Assignee

Updated

2 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 27

2 years ago
Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/bcca0465773f8a75863f35f75d6f2594df505c06
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → WONTFIX
(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #26)
> That would work as a last resort.  But IIRC the specific thing that showed
> up in the profile was the fact that we were creating a principal out of a
> URL somewhere and that was the expensive part of the permission check!  I
> think I should back this out, and then we should reprofile and first try to
> see if we can somehow make that code fast.  The details of the profile
> escape me right now, I profiled this a few week ago.

This change would avoid that principal creation. There would be a single flag for if _any_ principal had one of these permissions set, which would be false for probably ~99% of our users, eliminating the principal creation entirely.
Assignee

Comment 29

2 years ago
(In reply to Michael Layzell [:mystor] from comment #28)
> (In reply to :Ehsan Akhgari (super long backlog, slow to respond) from
> comment #26)
> > That would work as a last resort.  But IIRC the specific thing that showed
> > up in the profile was the fact that we were creating a principal out of a
> > URL somewhere and that was the expensive part of the permission check!  I
> > think I should back this out, and then we should reprofile and first try to
> > see if we can somehow make that code fast.  The details of the profile
> > escape me right now, I profiled this a few week ago.
> 
> This change would avoid that principal creation. There would be a single
> flag for if _any_ principal had one of these permissions set, which would be
> false for probably ~99% of our users, eliminating the principal creation
> entirely.

Yeah, your solution will definitely fix this performance issue.

But here is what I mean.  The principal creation code IIRC is <https://searchfox.org/mozilla-central/rev/6580dd9a4eb0224773897388dba2ddf5ed7f4216/extensions/cookie/nsPermissionManager.cpp#209> so the performance issue that I was referring to affects any calls to nsIPermissionManager::TestPermission().  What I meant to say was that I preferred the approach of first trying to see if we can somehow make that somehow faster...  (Actually making CreateCodebasePrincipal faster would benefit many other things as well!)  If that approach doesn't work then we have your solution as fallback.

What do you think?
(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #29)
> Yeah, your solution will definitely fix this performance issue.
> 
> But here is what I mean.  The principal creation code IIRC is
> <https://searchfox.org/mozilla-central/rev/
> 6580dd9a4eb0224773897388dba2ddf5ed7f4216/extensions/cookie/
> nsPermissionManager.cpp#209> so the performance issue that I was referring
> to affects any calls to nsIPermissionManager::TestPermission().  What I
> meant to say was that I preferred the approach of first trying to see if we
> can somehow make that somehow faster...  (Actually making
> CreateCodebasePrincipal faster would benefit many other things as well!)  If
> that approach doesn't work then we have your solution as fallback.
> 
> What do you think?

Ah, so you're talking about a more general "The permission manager is slow, let's make it fast" problem. Makes sense. The transition which I made during my internship from URIs to nsIPrincipal definitely made it do more work every time you check a permission as it has to create principals and extract their origins now.
Note that it might still be worth not doing the maybe-slow permissions check on non-image subresource loads, in addition to speeding up the check itself.
Assignee

Comment 32

2 years ago
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #31)
> Note that it might still be worth not doing the maybe-slow permissions check
> on non-image subresource loads, in addition to speeding up the check itself.

Yeah...  So I did what I was suggesting in bug 1362791, and after that the nsContentBlocker stuff is still showing up a bit in the profile.  :-(  Here is an excerpt:

-    3.84%     0.00%  Web Content  libxul.so  [.] imgLoader::LoadImage                                                              ▒
   - imgLoader::LoadImage                                                                                                           ▒
      - 2.10% mozilla::net::HttpChannelChild::AsyncOpen2                                                                            ▒
         + 1.75% mozilla::net::HttpChannelChild::AsyncOpen                                                                          ▒
         - 0.34% nsContentSecurityManager::doContentSecurityCheck                                                                   ▒
            - 0.17% NS_CheckContentLoadPolicy                                                                                       ▒
                 nsContentPolicy::ShouldLoad                                                                                        ▒
               - nsContentPolicy::CheckPolicy                                                                                       ▒
                  - 0.05% nsContentBlocker::ShouldLoad                                                                              ▒
                       mozilla::net::nsStandardURL::GetScheme                                                                       ▒
                       nsACString::Assign                                                                                           ▒
                       nsACString::Assign                                                                                           ▒
                       nsACString::ReplacePrep                                                                                      ▒
                  + 0.05% nsMixedContentBlocker::ShouldLoad                                                                         ▒
                  + 0.04% nsCategoryCache<nsIContentPolicy>::GetEntries                                                             ▒
                  + 0.04% nsCOMArray_base::~nsCOMArray_base                                                                         ▒

So I think we should do what Michael was suggesting on top of that.
Target Milestone: mozilla55 → ---
You need to log in before you can comment on or make changes to this bug.