Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Temporarily make same-compartment checks fatal in release builds

ASSIGNED
Assigned to

Status

()

Core
JavaScript Engine
ASSIGNED
7 years ago
5 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

({sec-want})

unspecified
mozilla2.0b8
sec-want
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 -)

Details

(Whiteboard: [sg:want][compartments][softblocker])

Attachments

(2 attachments)

Created attachment 487464 [details] [diff] [review]
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.
Attachment #487464 - Flags: review?(gal)

Comment 1

7 years ago
Comment on attachment 487464 [details] [diff] [review]
Patch

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.

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

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

Comment 4

7 years ago
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.

Updated

7 years ago
blocking2.0: --- → beta8+

Updated

7 years ago
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.

Updated

7 years ago
blocking2.0: beta8+ → beta9+

Comment 7

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

Updated

7 years ago
Blocks: 605662
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
Removing the inline for the checks solves the problem on my machine. I pushed to try again.

Comment 10

7 years ago
Us crashing whether its "inline" or not is super scary. We need a deeper understand what happens there.
Created attachment 498420 [details] [diff] [review]
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)
Whiteboard: [compartments] → [compartments][softblocker]

Comment 12

7 years ago
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+

Updated

7 years ago
No longer blocks: 605662
Blocks: 605662
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
** 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.
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: 563318

Updated

6 years ago
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...
Keywords: sec-want
You need to log in before you can comment on or make changes to this bug.