1357107 - Consider removing nsContentBlocker

Status: RESOLVED FIXED

(Core :: Permission Manager, enhancement, P2)

Description

[Nika Layzell [:nika] (ni? for response)]

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.

Comment 1

[(no longer active)]

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?

Comment 2

[(no longer active)]

(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.  :-)

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

(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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I'm fine with keeping permissions.default.image and nixing the rest.

Comment 6

[(no longer active)]

Attachment: Part 1: Move the handling of the permissions.default.image pref to imgLoader.cpp

Comment 7

[(no longer active)]

Attachment: Part 2: Remove nsContentBlocker

Comment 8

[Boris Zbarsky [: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?

Comment 9

[Boris Zbarsky [:bzbarsky]]

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.

Comment 10

[Nika Layzell [:nika] (ni? for response)]

(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.

Comment 11

[(no longer active)]

(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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 13

[(no longer active)]

Attachment: Part 1: Move the handling of the permissions.default.image pref to imgLoader.cpp

Comment 14

[(no longer active)]

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.

Comment 15

[Boris Zbarsky [: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

Comment 16

[Boris Zbarsky [:bzbarsky]]

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.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/27176f58992c
follow-up: replace some run-away tab characters

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9ed481306bc0
https://hg.mozilla.org/mozilla-central/rev/d9618183e882
https://hg.mozilla.org/mozilla-central/rev/27176f58992c

Comment 20

[(no longer active)]

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

Should I back out?

Comment 21

[(no longer active)]

(Hah! and a bugzilla component for it too, for that matter!)

Comment 22

[Boris Zbarsky [:bzbarsky]]

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?

Comment 23

[Nika Layzell [:nika] (ni? for response)]

(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

Comment 24

[(no longer active)]

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?

Comment 25

[Nika Layzell [:nika] (ni? for response)]

(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?

Comment 26

[(no longer active)]

(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.

Comment 27

[(no longer active)]

Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/bcca0465773f8a75863f35f75d6f2594df505c06

Comment 28

[Nika Layzell [:nika] (ni? for response)]

(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.

Comment 29

[(no longer active)]

(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?

Comment 30

[Nika Layzell [:nika] (ni? for response)]

(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.

Comment 31

[Boris Zbarsky [:bzbarsky]]

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.

Comment 32

[(no longer active)]

(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.

Comment 33

[Johann Hofmann [:johannh]]

Let's give this another shot! Ehsan, pbz and I talked about this and we think that nsContentBlocker should really go.

• We should add Telemetry for image blocking. Find out how many people are using it and if it's used enough to be supported (because extensions can easily do the same thing)
• Even if we have to keep supporting image blocking, we could disable nsContentBlocker unless there are any image permissions set.
• We could also do a soft deprecation where we do a migration that sets a pref for the UI when the user has image permissions, but hides the UI by default.

🔥

Comment 34

[Johann Hofmann [:johannh]]

Something I actually just realized now is that the image permission also doesn't block all images on the page, because it checks the permission per-load. So disabling images on bugzilla.mozilla.org would have almost no effect since all avatar images are hosted on Gravatar. Then I turned off images for Gravatar in the page info media tab just to see what happens, and then it turns out I can't turn it back on because, tada, you can only get the UI to show if any images were loaded. What a dumpster fire. uMatrix is a much nicer experience for this.

Comment 35

[Emma Zühlcke [:emz]]

Posted to firefox-dev: https://mail.mozilla.org/pipermail/firefox-dev/2019-November/007262.html

Comment 36

[Emma Zühlcke [:emz]]

Attachment: Bug 1357107 - Move the handling of the permissions.default.image pref to ImageBlocker.cpp. Original patch by ehsan. r=bzbarsky

Comment 37

[Emma Zühlcke [:emz]]

Attachment: Bug 1357107 - Remove nsContentBlocker. r=bzbarsky

Depends on D59695

Comment 38

[Emma Zühlcke [:emz]]

Attachment: Bug 1357107 - Remove nsContentBlocker telemetry. r=johannh

Depends on D59696

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/759b9308e69c
Remove nsContentBlocker telemetry. r=johannh

Comment 40

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/759b9308e69c

Comment 41

[Boris Zbarsky [:bzbarsky]]

The other changes still need to land.

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dea8bc3b320a
Move the handling of the permissions.default.image pref to ImageBlocker.cpp. Original patch by ehsan. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/798234088fd9
Remove nsContentBlocker. r=bzbarsky

Comment 43

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/dea8bc3b320a
https://hg.mozilla.org/mozilla-central/rev/798234088fd9