533876 - "Assertion failure: !JSVAL_IS_NULL(child->id), at ../jsscope.cpp" with __proto__, gc

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

f = eval("function(){x=[true]}")
f()
z = x
var g = eval("function(){for(a in[0,0]){z=z.__proto__=this}}")
try {
  g()
} catch(e) {
    delete eval
}
gc()
eval("function(){x.watch(\"x\",function()/x/)}")()

asserts js debug shell on TM tip without -j when parsed in as a command-line parameter at Assertion failure: !JSVAL_IS_NULL(child->id), at ../jsscope.cpp:605

autoBisecting soon... Setting security-sensitive due to gc being involved.

$ ./js-dbg-32-tm-darwin w49746-reduced.js 
Assertion failure: !JSVAL_IS_NULL(child->id), at ../jsscope.cpp:605
Abort trap

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to bug 473228:

The first bad revision is:
changeset:   35130:c03ebf340688
user:        Brendan Eich
date:        Fri Nov 20 16:14:42 2009 -0800
summary:     Bye-bye middle-deletes and their O(n^2) worst case complexity; hello dictionary-mode scopes (473228, r=jorendorff).

Comment 2

[Jason Orendorff [:jorendorff]]

#0  0x004b7422 in __kernel_vsyscall ()
#1  0x00845f60 in raise () from /lib/tls/i686/cmov/libpthread.so.0
#2  0x08144865 in JS_Assert (s=0x8215aeb "!JSVAL_IS_NULL(child->id)", file=0x8215900 "../jsscope.cpp", ln=611) at ../jsutil.cpp:70
#3  0x08125c43 in InsertPropertyTreeChild (rt=0x82418d0, parent=((JSScopeProperty *) NULL), child=((JSScopeProperty *) 0x8275d48) JSVAL_NULL, 
    sweptChunk=0x8279830) at ../jsscope.cpp:611
#4  0x08129664 in js_SweepScopeProperties (cx=0x82746a0) at ../jsscope.cpp:2058
#5  0x080b3b05 in js_GC (cx=0x82746a0, gckind=GC_LAST_CONTEXT) at ../jsgc.cpp:3227
#6  0x0807ac3c in js_DestroyContext (cx=0x82746a0, mode=JSDCM_FORCE_GC) at ../jscntxt.cpp:762
#7  0x08060632 in JS_DestroyContext (cx=0x82746a0) at ../jsapi.cpp:1067
#8  0x0805315a in main (argc=1, argv=0xbffff3a8, envp=0xbffff3b0) at ../../shell/js.cpp:4914

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: v1

This was harder to figure out than one might guess by looking at the patch. :-\

Comment 4

[Brendan Eich [:brendan]]

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

Didn't see this one coming, but should have cleared on first principles, or at least asserted. Thanks,

/be

Comment 5

[Jason Orendorff [:jorendorff]]

Pushed.

http://hg.mozilla.org/tracemonkey/rev/3036013432ce

But there's still a path through putProperty that *might* have a bug if the caller passes SPROP_IN_DICTIONARY in the flags. The right answer is to encapsulate sprop->flags a little better. Filed follow-up bug 539829 for that.

Comment 6

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/3036013432ce

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

The test added for this is missing a reportCompare() or similar, so it gets counted as a fail.. js1_8_5 was just never in the top jstests.list, so it was never run.  I've fixed that but marked this test as failing for now -- can you guys add whatever the right reportCompare bit is and unmark it?

Comment 8

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/6dcce4f4d9dd

Comment 9

[Jason Orendorff [:jorendorff]]

Hmm, that makes the test pass in shell, but not in the browser, where it generates a syntax error. I'll have to look into it later. Backed out for now.

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #9)
> Hmm, that makes the test pass in shell, but not in the browser, where it
> generates a syntax error. I'll have to look into it later. Backed out for now.

Removing fixed-in-tracemonkey description and reopening.

Comment 11

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #10)
> (In reply to comment #9)
> > Hmm, that makes the test pass in shell, but not in the browser, where it
> > generates a syntax error. I'll have to look into it later. Backed out for now.
> 
> Removing fixed-in-tracemonkey description and reopening.

Oops, I might be wrong - was this only for the test, and not for the patch?

Comment 12

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > Hmm, that makes the test pass in shell, but not in the browser, where it
> > > generates a syntax error. I'll have to look into it later. Backed out for now.
> > 
> > Removing fixed-in-tracemonkey description and reopening.
> 
> Oops, I might be wrong - was this only for the test, and not for the patch?

Yup, Jesse confirms my error, apparently backout was only for test, reverting changes.

Comment 13

[Jason Orendorff [:jorendorff]]

Fixed the test correctly.

http://hg.mozilla.org/tracemonkey/rev/e257eb2060a4

Comment 14

[Daniel Veditz [:dveditz]]

Is it possible to get this fixed completely on the 1.9.2 branch without taking the entire 60K patch in bug 539829 also? How much of that bug is necessary to fix comment 5 and how much is clean-up?  Or is it safe clean-up that we're better off taking because it's low risk and less trunk divergence?

Comment 15

[Jason Orendorff [:jorendorff]]

(In reply to comment #14)
> Is it possible to get this fixed completely on the 1.9.2 branch without taking
> the entire 60K patch in bug 539829 also?

Yes.

> How much of that bug is necessary to
> fix comment 5 and how much is clean-up?

None of that patch is needed.

>  Or is it safe clean-up that we're
> better off taking because it's low risk and less trunk divergence?

Yes, that one is very low risk.

Comment 16

[Daniel Veditz [:dveditz]]

This one appears to have slipped through the cracks -- let's get this into 1.9.2

Comment 17

[Jason Orendorff [:jorendorff]]

I'm the world's worst owner for this. Can someone else pick it up? Cc-ing dmandelin.

Comment 18

[David Mandelin [:dmandelin]]

What needs to be done here? Just land this patch on 1.9.2?

Comment 19

[Jason Orendorff [:jorendorff]]

Yep. Backport and land. It's a tiny patch, should be super easy. I'm more worried about what happens to the debugger schedule if this ends up being more than just pushing it. I can get to it next week if nobody wants to take.

Comment 20

[David Mandelin [:dmandelin]]

I looked at this more closely. The test case in comment 0, and the new one added by the patch (taken from tip, not the original landing) both pass in a 1.9.2 debug shell build. Also, the flag added in by the patch didn't even exist in 1.9.2. So I claim that 1.9.2 is unaffected. Correct me if I'm wrong.