396851 - XOW does system principal check when it should do IsCapabilityEnabled("UniversalXPConnect") check

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 367911 comment 36 last paragraph.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Easy fix

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 281614 [details] [diff] [review]
Easy fix

r+sr=bzbarsky, but technically I'm not an xpconnect peer.  Then again, the owner/peer list seems to be kinda out of date, esp. for this code...  and this is a no-brainer.

Comment 3

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

Just worried about performance here. Not a blocker, but we'll take the patch.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 281614 [details] [diff] [review]
Easy fix

This turned out not even to compile. It turns out to do this correctly, we'd have to compute the subject principal twice, and we don't want to do that.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Perhaps we need a "same origin or has capability X" API on nsIScriptSecurityManager?

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

I was going to add a less generic api looking something like:

[noscript] void allowPropertyAccess(in JSContextPtr aJSContext,
                                    in JSObjectPtr aJSObject,
                                    in string aClassName,
                                    in nsIPrincipal aSubjectPrincipal,
                                    in nsIPrincipal aObjectPrincipal,
                                    in string aPropName,
                                    in PRUint32 aAction,
                                    out boolean isCrossOrigin);

which deals with whether the given property is allAccess, noAccess, etc. If the caller doesn't have the object principal or subject principal, the function would compute them. It might be too XOW specific, though.

Comment 7

[Mike Schroepfer]

Re-noming for 1.9 for perf reasons..

Comment 8

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

Actually, the worry here is that fixing this would make us slower.

Blake, is there any way a fix here could improve performance?

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

This bug in itself shouldn't affect perf. My proposed solution should allow us to fix this without slowing down. The worry was that without that function we'd need to walk the JS stack twice looking for the subject principal.

Comment 10

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

Ok, still saying this isn't a blocker then.

Comment 11

[Johnny Stenback (:jst)]

(In reply to comment #6)
[...]
> caller doesn't have the object principal or subject principal, the function
> would compute them. It might be too XOW specific, though.

I don't think we care too much at this point if a caps API is too specific for any one purpose. Caps is going to get way overhauled in moz2, so whatever we need right now to make XOWs more useful and easier to work with is IMO ok to do.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 342975 [details] [diff] [review]
Fix

s/chrome code/UniversalXPConnect code/ and r+sr=bzbarsky.

But for the love of all that is holy, please put 

  unified = 8

in the [diff] section of ~/.hgrc if you're going to post mq patches!

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: With mochitest

I realized that I forgot to add a mochitest, so here it is.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 343673 [details] [diff] [review]
With mochitest

Excellent

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/3b0909b12aa5

Comment 17

[Boris Zbarsky [:bzbarsky]]

Ugh.  The branch version changed nsIScriptSecurityManager on 1.9.1....  This causes people issues; see the "using 3.0 plugins with 3.5" thread in m.d.t.xpcom starting today.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

This is needed for bug 460882 on the 1.9.0 branch.