SOWs shouldn't be created for chrome objects

RESOLVED FIXED in Firefox 20

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

({regression})

unspecified
mozilla21
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox19 unaffected, firefox20+ fixed, firefox21 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Keywords: regression
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.
Attachment #704197 - Flags: review?(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?
Attachment #704197 - Flags: review?(bzbarsky) → review-
(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.
Attachment #704197 - Attachment is obsolete: true
Attachment #704647 - Flags: review?(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.
Attachment #704647 - Flags: review?(bzbarsky) → review+
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.
Attachment #704647 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/736433b66cd0
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
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.
Attachment #704647 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.