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

RESOLVED FIXED

Status

()

--
critical
RESOLVED FIXED
9 years ago
4 years ago

People

(Reporter: gkw, Unassigned)

Tracking

(Blocks: 1 bug, {assertion, regression, testcase})

Trunk
x86
Mac OS X
assertion, regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(status1.9.2 unaffected, status1.9.1 unaffected)

Details

(Whiteboard: [needs answer to comment 14 from jorendorff] fixed-in-tracemonkey)

Attachments

(1 attachment)

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
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).
Blocks: 473228
#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
Assignee: general → jorendorff
Created attachment 421537 [details] [diff] [review]
v1

This was harder to figure out than one might guess by looking at the patch. :-\
Attachment #421537 - Flags: review?(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
Attachment #421537 - Flags: review?(brendan) → review+
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.
Whiteboard: fixed-in-tracemonkey

Comment 6

9 years ago
http://hg.mozilla.org/mozilla-central/rev/3036013432ce
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
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?
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.
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: fixed-in-tracemonkey
(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?
(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.
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-tracemonkey
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?
blocking1.9.2: --- → needed
status1.9.1: --- → unaffected
status1.9.2: --- → wanted
Whiteboard: fixed-in-tracemonkey → [needs answer to comment 14 from jorendorff] fixed-in-tracemonkey
(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.
This one appears to have slipped through the cracks -- let's get this into 1.9.2
blocking1.9.2: needed → .20+
I'm the world's worst owner for this. Can someone else pick it up? Cc-ing dmandelin.
Assignee: jorendorff → general
What needs to be done here? Just land this patch on 1.9.2?
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.
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.
blocking1.9.2: .20+ → ---
status1.9.2: wanted → unaffected
Group: core-security
You need to log in before you can comment on or make changes to this bug.