388597 - [FIXr]Content policy checks for XBL loads show up in profiles

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Opening the font preferences dialog, about 20% of the total time is spent doing content policy checks for the (chrome) XBL loads for whatever it's doing.  This is a change from bug 379959.

I'd like to get bug 204140 in before addressing this, though, because that will change the loading principal.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Maybe we want to generally pass the loading principal to NS_CheckContentLoadPolicy and skip content policy checks if it's system?  And maybe change skin stuff to get the system principal the same way content stuff does?

I doubt any content policy impls really want to catch cases of chrome loading things anyway...

Longer term, passing the loading principal to content policy would be a good idea, imo.  Better than passing the origin URI in some ways.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sounds like a great plan to me.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We could even just pass the loading principal to NS_CheckContentLoadPolicy for now and let that dig out the URI when passing it to the cp impls. I always feel safer when things take principals than uris, less risk to be grabbing stuff like baseuris that way.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Hmm.  That's a good point.  Ok, lemme see what I can do here.  ;)

Comment 5

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Sounds good to me.

Comment 6

[Boris Zbarsky [:bzbarsky]]

sicking, in txSyncCompileObserver::loadURI how are aReferrerURI and mCallerPrincipal related?  Which one should be used for the content policy check?

It looks like we use aReferrerURI for the same-origin check....

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Use mCallerPrincipal.

Say that page A links to stylesheet B which links to stylesheet C. When loading stylesheet C, mCallerPrincipal will be that of page A, whereas aReferrerURI will be that of stylesheet B.

This does feel like the desirable behavior, but I'm not really sure. What does the CSS code do?

For same-origin checks it doesn't matter much right now as there are same-origin policies everywhere. But this might change in the future what with <?access-control?> and all.

Comment 8

[Boris Zbarsky [:bzbarsky]]

The CSS code uses the principal of the thing the URI came from.  So if page A loads sheet B which imports sheet C, the security and content policy checks for C are done using the principal of B.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'd be fine with doing that for XSLT too.

Unfortunately we're dealing with strings in much of the code in order to support standalone transformiix too. These days standalone transformiix does use XPCOM, but since nsIPrincipal lives in caps it probably can't be used. In any case standalone transformiix is sort of broken anyway and it's future unknown.

Axel, Peter, you got any opinion on if it's ok to use nsIPrincipal in txStylesheetCompiler? We could just keep it as null for a (at this point theoretical) standalone build.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

I did the obvious thing for XSLT for now; we can do cleanup in a followup bug, I would hope.

Comment 11

[Wladimir Palant]

I am coming a little late, but - yes, I don't know of any content policies that want to prevent chrome from loading something. So making chrome an exception from content policies sounds like a good idea to me.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Wladimir, what do you think of API changes in, say, the mozilla 2 timeframe to pass the principal or equivalent in instead of the origin URI?

Comment 13

[Wladimir Palant]

This would be very useful information. Currently I use hacks in Adblock Plus to determine the origin of a request (chrome / which website / special treatment for javascript and data URLs). A principal would make this much easier.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Is the docshell the only place where we don't have a principal? If so, could we call GetCodebasePrincipal/GetSystemPrincipal there and remove the aOriginURI argument? And file a bug on fixing the LoadInternal signature to take a principal.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 273418 [details] [diff] [review]
Proposed fix

r/sr=me with or without that.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> Is the docshell the only place where we don't have a principal?

Yes.

> If so, could we call GetCodebasePrincipal/GetSystemPrincipal there and remove
> the aOriginURI argument?

Hmm... Yes.  But then all the loads that don't have an origin URI right now would not get content policy checks, which I think is wrong.

> And file a bug on fixing the LoadInternal signature to take a principal.

Probably a good idea for moz2.  Until then, we have (effectively) frozen interfaces that call into that function that should really be taking an nsIPrincipal but aren't (nsIWebNavigation, I'm looking at you).

Comment 17

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Filed bug 391298 on the XSLT stuff.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> Hmm... Yes.  But then all the loads that don't have an origin URI right now
> would not get content policy checks, which I think is wrong.

Why not? Couldn't you just pass null for originPrincipal which would then get turned into a null for requestOrigin?

Comment 20

[Boris Zbarsky [:bzbarsky]]

Oh, hmm.  Yeah, that would work.  File a bug, and I'll see what I can do?

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Filed bug 391438