725733 - crash js::analyze::ScriptAnalysis::addTypeBarrier

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

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 (:dmosedale, :dmose)]

This is mine.  It happened while trying to load the sprite demo at index.html here:

https://github.com/dmose/gladius/tree/sprites/example/sprites

Comment 2

[David Mandelin [:dmandelin]]

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 (:dmosedale, :dmose)]

My mistake; I meant devtools.debugger.enable; it's only in today's nightly build.

Comment 4

[David Mandelin [:dmandelin]]

How do I conveniently load the demo? I thought

  https://raw.github.com/dmose/gladius/sprites/example/sprites/index.html

would work, but I just see the HTML source.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

I just pushed it to my gh-pages branch.  This should do the trick: 

<http://dmose.github.com/gladius/example/sprites/index.html>

Comment 6

[Jason Orendorff [:jorendorff]]

I can reproduce this.

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: v1

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 [Laid off!]]

Comment on attachment 595897 [details] [diff] [review]
v1

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]]

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:
   https://crash-stats.mozilla.com/report/index/dc200b1e-1f69-4e06-b741-603b62120209

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]

Try run for 65cd443e128f is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=65cd443e128f
Results (out of 190 total builds):
    exception: 3
    success: 146
    warnings: 38
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-65cd443e128f

Comment 11

[Jason Orendorff [:jorendorff]]

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]]

I'll try again next week. We really need this fixed.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

I'm pretty the switch to the Java xpcom-bridge component & owner was inadvertent; putting it back.

Comment 14

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I am facing this crash quite often, any estimate on when this could be fixed?
Thanks!
Honza

Comment 15

[Rob Campbell [:rc] (:robcee)]

adding blocking-k9o request. This prevents shipping.

Any luck on the orange, Jason?

Comment 16

[Jason Orendorff [:jorendorff]]

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');
g.eval(
    "function f(x) { return {q: x}; }\n" +
    "var n = f('').q;\n");
var dbg = new Debugger(g);
g.eval("f(1000)");

Comment 17

[Jason Orendorff [:jorendorff]]

Forgot to mention: the shell test case in comment 16 requires -m -n -a.

Comment 18

[Jason Orendorff [:jorendorff]]

Attachment: v2

Comment 19

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=79f757e8a473

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]]

https://tbpl.mozilla.org/?tree=Try&rev=e94f19888bda

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  libxul.so!js::gc::GCCompartmentsIter::GCCompartmentsIter [jsgcinlines.h
 2  libxul.so!AutoGCSession::~AutoGCSession [jsgc.cpp : 3316 + 0x10]
 3  libxul.so!GCCycle [jsgc.cpp : 3556 + 0xd]
 4  libxul.so!Collect [jsgc.cpp : 3689 + 0x17]
 5  libxul.so!js::GC [jsgc.cpp : 3710 + 0x24]
 6  libxul.so!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]]

Attachment: prologue: use the jit_test.py --valgrind option only if we have valgrind support

Comment 22

[Jason Orendorff [:jorendorff]]

Attachment: fix: v3

Comment 23

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

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]]

Review ping. Should be pretty trivial.

Comment 25

[David Mandelin [:dmandelin]]

Patches have r+. Is this ready to go in?

Comment 26

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/4e5d09c8e1a1

Comment 27

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

And

https://hg.mozilla.org/mozilla-central/rev/c40f450c8b9c

Please note the inbound changesets in the bug.