832512 - SOWs shouldn't be created for chrome objects

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

The code for bug 820577 causes us to create SOWs for chrome code, which is wrong. As it stands, it's slower and uses more memory than it needs to, but it might regress other things in unexpected ways. Fixing this is pretty straightforward, so I think we should uplift it.

Comment 1

[Alex Keybl [:akeybl]]

Straight forward fix, could cause chrome regressions (any guidance as to what type would be helpful for testing/feedback).

For future reference (our reference), SOW=same-compartment security wrapper

Comment 2

[Bobby Holley (:bholley)]

(In reply to Alex Keybl [:akeybl] from comment #1)
> For future reference (our reference), SOW=same-compartment security wrapper

Not quite. SOW = system only wrapper, which most-commonly appears as a same-compartment security wrapper. They're nasty, and I'm working on abolishing them.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Don't create SOWs for chrome code. v1

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 704197 [details] [diff] [review]
Don't create SOWs for chrome code. v1

I don't think this is right for two reasons:

1)  It won't catch things going through WrapNewBindingObject.  Adding some tests would have caught this, I bet.

2)  It's adding the system-principal check to the fast path of MaybeWrapValue, which doesn't seem great either.

Why not add the security check at the point in nsINode::WrapObject where we'd call SetSystemOnlyWrapper?  In particular, seems like we can just add !IsSystemPrincipal(NodePrincipal()) to the conditions there, right?  Or would that not do the right thing?

Comment 5

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #4)
> Why not add the security check at the point in nsINode::WrapObject where
> we'd call SetSystemOnlyWrapper?  In particular, seems like we can just add
> !IsSystemPrincipal(NodePrincipal()) to the conditions there, right?  Or
> would that not do the right thing?

Yeah, good call. This will mean that we can still end up with in-chrome SOWs when reparenting nodes from content to chrome, but I don't think that's a big deal. In particular, I don't think the old behavior handled that edge case either.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Don't create SOWs for chrome code. v2

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 704647 [details] [diff] [review]
Don't create SOWs for chrome code. v2

Is the test tab-indented or something?  In any case, just make sure your indentation matches the test's.

r=me with that.

Comment 8

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/736433b66cd0

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 704647 [details] [diff] [review]
Don't create SOWs for chrome code. v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
bug 815149

User impact if declined: Probably not much, but it could cause very hard-to-diagnose bugs in extensions. This patch makes us stick with our historical behavior.
 
Testing completed (on m-c, etc.): Just pushed to m-i.

Risk to taking this patch (and alternatives if risky): Not risky. Alternative is to just not take the patch, which isn't that bad either. I think it's just a marginal win to take it. 

String or UUID changes made by this patch: None.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/736433b66cd0

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 704647 [details] [diff] [review]
Don't create SOWs for chrome code. v2

seems pretty straightforward and worth the marginal win on aurora, approving.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ee5b2e692449