608855 - Temporarily make same-compartment checks fatal in release builds

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: Patch

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.

Comment 1

[Andreas Gal :gal]

Comment on attachment 487464 [details] [diff] [review]
Patch

Thanks Waldo. Lets put this onto trunk after b7 branched.

Comment 2

[Jeff Walden [:Waldo]]

I pushed this to TM -- next m-c merge should pick it up.

http://hg.mozilla.org/tracemonkey/rev/9e5cd5815d4d

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.

Comment 3

[Jeff Walden [:Waldo]]

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:

http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1289269411.1289269510.20772.gz&fulltext=1#err1

Comment 4

[Boris Zbarsky [:bzbarsky]]

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?).

Comment 5

[David Mandelin [:dmandelin]]

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.

Comment 6

[Jeff Walden [:Waldo]]

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.

Comment 7

[Andreas Gal :gal]

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

Comment 8

[Gregor Wagner [:gwagner]]

These are the remaining crashes. All of them include the JS_IdToValue call at the end.

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1292411833.1292412035.11011.gz&fulltext=1#err0

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1292411862.1292412587.14079.gz&fulltext=1#err0

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1292420737.1292421340.25741.gz&fulltext=1#err4

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1292407812.1292408309.24718.gz&fulltext=1#err4

Comment 9

[Gregor Wagner [:gwagner]]

Removing the inline for the checks solves the problem on my machine. I pushed to try again.

Comment 10

[Andreas Gal :gal]

Us crashing whether its "inline" or not is super scary. We need a deeper understand what happens there.

Comment 11

[Gregor Wagner [:gwagner]]

Attachment: Version with JS_NEVER_INLINE

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 ()
(gdb)

Comment 12

[LegNeato]

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)

Comment 13

[Gregor Wagner [:gwagner]]

The remaining crashes with this patch all have a similar stack:

 1  libxul.so!JS_Assert [jsutil.cpp : 83 + 0xb]
    eip = 0x01bfd54e   esp = 0xbf8a1540   ebp = 0xbf8a1598
    Found by: previous frame's frame pointer
 2  libxul.so!js::CompartmentChecker::check [jscntxtinlines.h : 541 + 0x1f]
    eip = 0x01adf5e4   esp = 0xbf8a1560   ebp = 0xbf8a1598   ebx = 0x0214101c
    esi = 0xbf8a1598
    Found by: call frame info
 3  libxul.so!JS_IdToValue [jscntxtinlines.h : 557 + 0xb]
    eip = 0x01ad4cc0   esp = 0xbf8a1580   ebp = 0xbf8a1598   ebx = 0x0214101c
    esi = 0xafd19fc0
    Found by: call frame info
 4  libxul.so!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.

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1294298969.1294299572.30791.gz&fulltext=1#err4

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

** PRODUCT DRIVERS PLEASE NOTE **

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.

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

(er updating flag to "-" as per previous comment!)

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 17

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Do we use strict aliasing on windows? This looks a bit like 684526.

Comment 18

[Jeff Walden [:Waldo]]

I thought strict aliasing was a gcc-only thing...

Comment 19

[Release mgmt bot [:suhaib / :marco/ :calixte]]

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