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.

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

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

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