Closed Bug 611432 Opened 14 years ago Closed 13 years ago

Add assertions to check that compartments are stay the same for JM activations

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: dmandelin, Assigned: dmandelin)

References

Details

(Whiteboard: compartments, softblocker, fixed-in-tracemonkey)

Attachments

(1 file, 2 obsolete files)

Discovered via bug 609244.

That bug is caused by the fact that something can change cx->compartment somewhere in a JM activation (i.e., when EnterMethodJIT is on the stack) and then fail to change it back before we come out of the JM activation. We don't have a test case--we just know it's happening in the field based on analysis of bug 609244.
David, there are some compartment mismatch assertions I've hit only when JM is enabled.  See steps to reproduce in bug 610973 and some analysis in bug 610941 (where it basically comes down to the web page picking up String.prototype from the wrong compartment).  Could that be related to what you're seeing?
blocking2.0: --- → ?
Yeah, it could easily be the same thing.
Depends on: 610941
blocking2.0: ? → betaN+
Depends on: 618871
Morphing this bug to add the assertions, so the violations can be caught and fixed in the normal course of things.
Assignee: general → dmandelin
Summary: Compartments are not getting reset after cross-compartment calls in JM → Add assertions to check that comparments are stay the same for JM activations
Attached patch Patch (obsolete) — Splinter Review
This passes jit-tests and jstests, so no current bustage. Note for reviewer: I'm not sure forcing the compartment back to the original value is the right thing to do; maybe we should just assert and not change it.
Attachment #499096 - Flags: review?(jorendorff)
Attached patch Fixed patch (obsolete) — Splinter Review
Whoops, forgot to qref before that last one. My comments about passing tests apply to this patch, not the bad one (although that passes too, because of its badness).
Attachment #499096 - Attachment is obsolete: true
Attachment #499118 - Flags: review?(jorendorff)
Attachment #499096 - Flags: review?(jorendorff)
Summary: Add assertions to check that comparments are stay the same for JM activations → Add assertions to check that compartments are stay the same for JM activations
Whiteboard: compartments → compartments,softblocker
Attachment #499118 - Flags: review?(jorendorff) → review?(gal)
Comment on attachment 499118 [details] [diff] [review]
Fixed patch

Thats a really terrible name. How about AssertCompartmentUnchanged or something like that. And I also think this should be debug only. Unbalanced compartment setting is a bad bug. We wont be shipping with code that does that.
Attachment #499118 - Flags: review?(gal) → review+
Attached patch Refixed patchSplinter Review
Can you review this again, since I changed it substantially? I used your name and took out forcing the compartment to be correct, since that is apparently just wrong.
Attachment #499118 - Attachment is obsolete: true
Attachment #501517 - Flags: review?(gal)
Attachment #501517 - Flags: review?(gal) → review+
http://hg.mozilla.org/tracemonkey/rev/acab52f46625
Status: NEW → ASSIGNED
Whiteboard: compartments,softblocker → compartments, softblocker, fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/acab52f46625
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: