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 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Jason Orendorff [:jorendorff] 2012-02-09 14:25:15 PST
I can reproduce this.
Comment 7 User image 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 User image 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 User image 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 User image 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 User image 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 User image Jason Orendorff [:jorendorff] 2012-03-21 09:19:07 PDT
I'll try again next week. We really need this fixed.
Comment 13 User image 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 User image 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 User image 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 User image 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 User image 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 User image Jason Orendorff [:jorendorff] 2012-04-21 09:47:27 PDT
Created attachment 617231 [details] [diff] [review]
Comment 19 User image 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 User image 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 User image 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 User image Jason Orendorff [:jorendorff] 2012-04-25 12:29:26 PDT
Created attachment 618390 [details] [diff] [review]
fix: v3
Comment 23 User image 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 User image Jason Orendorff [:jorendorff] 2012-04-27 15:40:41 PDT
Review ping. Should be pretty trivial.
Comment 25 User image David Mandelin [:dmandelin] 2012-05-02 17:05:08 PDT
Patches have r+. Is this ready to go in?
Comment 26 User image :Ms2ger (⌚ UTC+1/+2) 2012-05-05 03:35:27 PDT
Comment 27 User image :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.