Last Comment Bug 725733 - crash js::analyze::ScriptAnalysis::addTypeBarrier
: crash js::analyze::ScriptAnalysis::addTypeBarrier
[paladin] [chrome-debug][k9o:p1:fx15]
: crash
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All Mac OS X
: -- critical (vote)
: mozilla15
Assigned To: Jason Orendorff [:jorendorff]
: Jason Orendorff [:jorendorff]
Depends on: 754201
Blocks: minotaur
  Show dependency treegraph
Reported: 2012-02-09 10:47 PST by Panos Astithas [:past]
Modified: 2012-05-11 08:51 PDT (History)
13 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

v1 (1.10 KB, patch)
2012-02-09 16:09 PST, Jason Orendorff [:jorendorff]
bhackett1024: review+
Details | Diff | Splinter Review
v2 (2.84 KB, patch)
2012-04-21 09:47 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
prologue: use the --valgrind option only if we have valgrind support (1.92 KB, patch)
2012-04-25 12:28 PDT, Jason Orendorff [:jorendorff]
jimb: review+
Details | Diff | Splinter Review
fix: v3 (3.38 KB, patch)
2012-04-25 12:29 PDT, Jason Orendorff [:jorendorff]
wmccloskey: review+
Details | Diff | Splinter Review

Description Panos Astithas [:past] 2012-02-09 10:47:45 PST
This bug was filed from the Socorro interface and is 
report bp-dc200b1e-1f69-4e06-b741-603b62120209 .

User comment:

Set devtools.debug.enable to true. Selected "Script Debugger" from the menu while on a page with a WebGL demo running. Boom.
Comment 1 Dan Mosedale (:dmose) 2012-02-09 11:02:41 PST
This is mine.  It happened while trying to load the sprite demo at index.html here:
Comment 2 David Mandelin [:dmandelin] 2012-02-09 11:46:13 PST
I set devtools.debug.enable to true, but I don't have "Script Debugger" in my Tools/Web Developer menu. Do I need a special build?
Comment 3 Dan Mosedale (:dmose) 2012-02-09 11:49:22 PST
My mistake; I meant devtools.debugger.enable; it's only in today's nightly build.
Comment 4 David Mandelin [:dmandelin] 2012-02-09 11:55:09 PST
How do I conveniently load the demo? I thought

would work, but I just see the HTML source.
Comment 5 Dan Mosedale (:dmose) 2012-02-09 12:41:38 PST
I just pushed it to my gh-pages branch.  This should do the trick: 

Comment 6 Jason Orendorff [:jorendorff] 2012-02-09 14:25:15 PST
I can reproduce this.
Comment 7 Jason Orendorff [:jorendorff] 2012-02-09 16:09:57 PST
Created attachment 595897 [details] [diff] [review]

I can't get this to reproduce in the shell, so no reduced testcase. :-P

Brian, I can (in the shell) create a new compartment, run some code in it, turn on the debugger, then run some more code. So it seems like I *should* be able to reproduce this bug if I can get the right kind of analysis to occur. Any suggestions?
Comment 8 Brian Hackett (:bhackett) 2012-02-16 14:43:56 PST
Comment on attachment 595897 [details] [diff] [review]

Which situation are you trying to repro in the shell?  Not sure what chain of events is causing this crash.
Comment 9 Jason Orendorff [:jorendorff] 2012-02-25 13:12:33 PST
Well, I don't either. That's the problem. The chain of events is *like* this:

1. Make some analysis happen in compartment X.

2. Turn on debug mode for compartment X, e.g. by doing |new Debugger(X)|
   from some other compartment.
   This causes JSCompartment::updateForDebugMode to run, see patch.

   At this point analysis is in some kind of inconsistent state.

3. Run more code in compartment X. This causes a crash reading null:

So the test would look like this (run with -m -n):

    var g = newGlobal('new-compartment');
    g.eval(???);                   // step 1
    var dbg = new Debugger(g);     // step 2
    g.eval(???);                   // step 3

It might be really simple. I just don't know what kind of code triggers the creation of the particular kind of data structure that updateForDebugMode screws up.
Comment 10 Mozilla RelEng Bot 2012-03-02 13:03:39 PST
Try run for 65cd443e128f is complete.
Detailed breakdown of the results available here:
Results (out of 190 total builds):
    exception: 3
    success: 146
    warnings: 38
    failure: 3
Builds (or logs if builds failed) available at:
Comment 11 Jason Orendorff [:jorendorff] 2012-03-08 07:40:55 PST
This patch causes some Linux-only orange. I don't know what's wrong. Last time I looked I couldn't reproduce it, but I'll try again next week.
Comment 12 Jason Orendorff [:jorendorff] 2012-03-21 09:19:07 PDT
I'll try again next week. We really need this fixed.
Comment 13 Dan Mosedale (:dmose) 2012-03-21 10:21:40 PDT
I'm pretty the switch to the Java xpcom-bridge component & owner was inadvertent; putting it back.
Comment 14 Jan Honza Odvarko [:Honza] 2012-04-15 01:49:05 PDT
I am facing this crash quite often, any estimate on when this could be fixed?
Comment 15 Rob Campbell [:rc] (:robcee) 2012-04-20 09:37:28 PDT
adding blocking-k9o request. This prevents shipping.

Any luck on the orange, Jason?
Comment 16 Jason Orendorff [:jorendorff] 2012-04-20 15:09:10 PDT
The patch has rotted. The "FreeOp" work makes this harder to hack around in this way. I'll try to fix it some other way.

I managed to produce a shell test case by error and error. (It's like trial and error, but with more errors.)

var g = newGlobal('new-compartment');
    "function f(x) { return {q: x}; }\n" +
    "var n = f('').q;\n");
var dbg = new Debugger(g);
Comment 17 Jason Orendorff [:jorendorff] 2012-04-20 15:09:51 PDT
Forgot to mention: the shell test case in comment 16 requires -m -n -a.
Comment 18 Jason Orendorff [:jorendorff] 2012-04-21 09:47:27 PDT
Created attachment 617231 [details] [diff] [review]
Comment 19 Jason Orendorff [:jorendorff] 2012-04-24 08:59:02 PDT

The orange is apparently due to running tests with --valgrind when we didn't configure with --enable-valgrind. (That means we don't have valgrind-readable annotations in the executable about the conservative stack scanning, which reads uninitialized stack locations. So valgrind has no reason to consider the GC sane.) But given that we have that (ridiculous) bug in the Makefile, I don't understand why we do not have huge amounts of orange all the time. Anyone?
Comment 20 Jason Orendorff [:jorendorff] 2012-04-25 09:11:46 PDT

Fixed the valgrind thing. It looks like now I'm tripping an assertion that happens at the end of GC:

  Assertion failure: it < end, at ../../../js/src/jsgcinlines.h:402
 1!js::gc::GCCompartmentsIter::GCCompartmentsIter [jsgcinlines.h
 2!AutoGCSession::~AutoGCSession [jsgc.cpp : 3316 + 0x10]
 3!GCCycle [jsgc.cpp : 3556 + 0xd]
 4!Collect [jsgc.cpp : 3689 + 0x17]
 5!js::GC [jsgc.cpp : 3710 + 0x24]
 6!JSCompartment::updateForDebugMode [jscompartment.cpp : 718 + 0x17]

GCCompartmentsIter asserts that at least one compartment isCollecting(). The only way I see that this could be false is if the compartment itself is being destroyed during GC. 

I will fix this using a kung fu death grip (a.k.a. AutoHoldCompatment), just to make sure my understanding of the problem is correct, but I *hope* the right answer is to remove the assertion, and billm will tell me that on review...
Comment 21 Jason Orendorff [:jorendorff] 2012-04-25 12:28:42 PDT
Created attachment 618389 [details] [diff] [review]
prologue: use the --valgrind option only if we have valgrind support
Comment 22 Jason Orendorff [:jorendorff] 2012-04-25 12:29:26 PDT
Created attachment 618390 [details] [diff] [review]
fix: v3
Comment 23 [PTO to Dec5] Bill McCloskey (:billm) 2012-04-25 12:41:36 PDT
Comment on attachment 618390 [details] [diff] [review]
fix: v3

Review of attachment 618390 [details] [diff] [review]:

::: js/src/jscompartment.cpp
@@ +713,5 @@
> +    // compartment. Because !hasScriptsOnStack(), it suffices to do a garbage
> +    // collection cycle or to finish the ongoing GC cycle. The necessary
> +    // cleanup happens in JSCompartment::sweep.
> +    if (!rt->gcRunning) {
> +        scheduleGC();

Could you change this to PrepareCompartmentForGC(this)?
Comment 24 Jason Orendorff [:jorendorff] 2012-04-27 15:40:41 PDT
Review ping. Should be pretty trivial.
Comment 25 David Mandelin [:dmandelin] 2012-05-02 17:05:08 PDT
Patches have r+. Is this ready to go in?
Comment 26 :Ms2ger (⌚ UTC+1/+2) 2012-05-05 03:35:27 PDT
Comment 27 :Ms2ger (⌚ UTC+1/+2) 2012-05-05 03:36:54 PDT

Please note the inbound changesets in the bug.

Note You need to log in before you can comment on or make changes to this bug.