Open Bug 608855 Opened 14 years ago Updated 1 year ago

Temporarily make same-compartment checks fatal in release builds


(Core :: JavaScript Engine, defect)




Tracking Status
blocking2.0 --- -


(Reporter: Waldo, Unassigned)



(Keywords: sec-want, Whiteboard: [sg:want][compartments][softblocker])


(2 files)

Attached patch PatchSplinter Review
Andreas suggests this either for b7 or for trunk.

I could go either way, but looking at where some of these checks lie, I wonder at the performance impact of doing so in a beta release where we're aiming to make quite a perf-impact splash.  I haven't checked, but I'd be willing to bet these checks, scattered and ubiquitous as they are, would be enough to take us from in the lead on SunSpider to at least a few milliseconds back from the leaders.  (Sure, we'll get to the same place -- in the lead -- in the end, but we can make more hay from a win than we can from a not-quite win plus a later, much-smaller improvement.)  Worth doing only for trunk builds then, maybe?  *shrug*  We report, drivers decide.
Attachment #487464 - Flags: review?(gal)
Comment on attachment 487464 [details] [diff] [review]

Thanks Waldo. Lets put this onto trunk after b7 branched.
Attachment #487464 - Flags: review?(gal) → review+
I pushed this to TM -- next m-c merge should pick it up.

Although, hm, that means it'll show up on awfy and stuff.  Pushing just to m-c will have it show up as an issue at merge time.  I don't see a clean way to keep up appearances ;-) yet get testing with this.  If someone has particular thoughts on this, speak up -- easy enough to do something different than I've done so far.
Whiteboard: fixed-in-tracemonkey
Target Milestone: mozilla2.0b7 → mozilla2.0b8
Hm, backed this out -- was hitting orange, oddly enough, trying to do the assertion underneath JS_IdToValue for the nsDOMClassInfo::GetArrayIndexFromId call spot.  Some tinderboxen just show it as a crash in one of the inlined compartment methods, but at least one box is showing a crash from within the assert I made always-fatal:
Whiteboard: fixed-in-tracemonkey
By the way, the regress graphs on awfy seem to show this as a huge regression for TM (but not for JM or JM+TM?).
I was surprised to see that too, but I think I know the answer: the checks affect the interpreter only, and we spend substantial time in the interpreter in TM-only mode.
blocking2.0: --- → beta8+
Whiteboard: [compartments]
Yeah, I roughly expected a perf regression from this, most probably from doing same-compartment assertions for every single interpreter stack push.  (Yes, really.)  That's why this would be an in-during-development, out-for-betas thing.

I don't have time to investigate the orange that tinderboxen were sometimes picking up on this, unfortunately.  I'm busy on the last big-ticket part of strict mode, and without that I'm not even sure if I'd want to ship strict mode without it.
blocking2.0: beta8+ → beta9+
We need this to go into the tree right after b8 ships, and we have to get the tree green, before landing bug 605662 (per-compartment gc).
Removing the inline for the checks solves the problem on my machine. I pushed to try again.
Us crashing whether its "inline" or not is super scary. We need a deeper understand what happens there.
this patch fails make check for an optimized build shell for /js/src/jit-test/tests/jaeger/bug549602.js

The address looks similar to the browser cause:
Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x00007ffffffff000
0x000000010001de79 in js::assertSameCompartment<unsigned long long> ()
(gdb) bt
#0  0x000000010001de79 in js::assertSameCompartment<unsigned long long> ()
#1  0x0000000100019882 in JS_ValueToString ()
Whiteboard: [compartments] → [compartments][softblocker]
As per today's meeting, beta 9 will be a time-based release. Marking these all betaN+. Please move it back to beta9+ if  you believe it MUST be in the next beta (ie: trunk is in an unshippable state without this)
blocking2.0: beta9+ → betaN+
No longer blocks: compartmentGC
The remaining crashes with this patch all have a similar stack:

 1!JS_Assert [jsutil.cpp : 83 + 0xb]
    eip = 0x01bfd54e   esp = 0xbf8a1540   ebp = 0xbf8a1598
    Found by: previous frame's frame pointer
 2!js::CompartmentChecker::check [jscntxtinlines.h : 541 + 0x1f]
    eip = 0x01adf5e4   esp = 0xbf8a1560   ebp = 0xbf8a1598   ebx = 0x0214101c
    esi = 0xbf8a1598
    Found by: call frame info
 3!JS_IdToValue [jscntxtinlines.h : 557 + 0xb]
    eip = 0x01ad4cc0   esp = 0xbf8a1580   ebp = 0xbf8a1598   ebx = 0x0214101c
    esi = 0xafd19fc0
    Found by: call frame info
 4!nsDOMClassInfo::GetArrayIndexFromId [nsDOMClassInfo.cpp : 4235 + 0xe]
    eip = 0x0135cfc2   esp = 0xbf8a15b0   ebp = 0xbf8a1628   ebx = 0x0214101c
    esi = 0xafd19fc0   edi = 0x995cda60
    Found by: call frame info

*** Compartment mismatch 0xb42fe000 vs. 0x565101, defaultComp: 0xb5d98000
I don't think we have a compartment mismatch here because the actual object doesn't seem to be heap allocated.
0xb42fe000 is the expected compartment and 0x565101 is the compartment pointer for the current object. That's definitely not a compartment pointer.

This bug is one of 19 being moved from blocking2.0:betaN+ to blocking2.0:- as we reached the endgame of Firefox 4. The rationale for the move is:

 - the bug had been identified as a "soft" blocker which could be fixed in some follow up release
 - the bug had been identified as one requiring beta coverage, thus is not appropriate for a ".x" stability & security release

The owner of the bug may wish to renominate for .x consideration.
blocking2.0: betaN+ → .x+
(er updating flag to "-" as per previous comment!)
blocking2.0: .x+ → -
I looked at this a while ago and it seems to trigger a compiler bug. Jesse asked me to add a dependency on the MSVC 2010 upgrade, since that would probably fix it.
Depends on: msvc2010
Whiteboard: [compartments][softblocker] → [sg:want][compartments][softblocker]
Do we use strict aliasing on windows? This looks a bit like 684526.
I thought strict aliasing was a gcc-only thing...

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Assignee: jwalden → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.