426711 - Setting window.__count__ causes a crash [@ nsWindowSH::AddProperty]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Loading the testcase triggers

###!!! ASSERTION: The JS engine lies: 'prop && obj == pobj', file /Users/jruderman/trunk/mozilla/dom/src/base/nsDOMClassInfo.cpp, line 4651

and a crash [@ nsWindowSH::AddProperty].

Comment 1

[Jesse Ruderman]

jsfunfuzz in the browser hits this (and bug 426520) every few minutes.

Comment 2

[Damon Sicore (:damons)]

Can we get more information on why this should block other than just that it's a crash?  Is this [testcase] a use case that is super common?  Please re-nom if so.

Comment 3

[Johnny Stenback (:jst)]

So this is odd, here's the stack, pay attention to the id of the property being set:

#0  0x05cde690 in nsWindowSH::AddProperty (this=0x9f5a6a0, wrapper=0x9f271c0, 
    cx=0x967bb20, obj=0x9c61340, id=1, vp=0xbfd1203c, _retval=0xbfd11948)
    at ../../../../mozilla/dom/src/base/nsDOMClassInfo.cpp:4673
#1  0x011fa605 in XPC_WN_Helper_AddProperty (cx=0x967bb20, obj=0x9c61340, 
    idval=1, vp=0xbfd1203c)
    at ../../../../../mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp:938
#2  0x0021d645 in js_SetPropertyHelper (cx=0x967bb20, obj=0x9c61340, 
    id=158536092, vp=0xbfd1203c, entryp=0x0)
    at ../../../mozilla/js/src/jsobj.c:3896
#3  0x0021dbe3 in js_SetProperty (cx=0x967bb20, obj=0x9c61340, id=158536092, 
    vp=0xbfd1203c) at ../../../mozilla/js/src/jsobj.c:3919
#4  0x01207c34 in XPC_XOW_GetOrSetProperty (cx=0x967bb20, obj=0x9a88280, 
    id=158536092, vp=0xbfd1203c, isSet=1)
    at ../../../../../mozilla/js/src/xpconnect/src/XPCCrossOriginWrapper.cpp:687
#5  0x01207dc8 in XPC_XOW_SetProperty (cx=0x967bb20, obj=0x9a88280, 
    id=158536092, vp=0xbfd1203c)
    at ../../../../../mozilla/js/src/xpconnect/src/XPCCrossOriginWrapper.cpp:732
#6  0x0021c507 in js_NativeSet (cx=0x967bb20, obj=0x9a88280, sprop=0xa8c2c90, 
    vp=0xbfd1203c) at ../../../mozilla/js/src/jsobj.c:3598
#7  0x0021dadc in js_SetPropertyHelper (cx=0x967bb20, obj=0x9a88280, 
    id=158536092, vp=0xbfd1203c, entryp=0xbfd11f74)
    at ../../../mozilla/js/src/jsobj.c:3902
#8  0x001f8ff3 in js_Interpret (cx=0x967bb20)
    at ../../../mozilla/js/src/jsinterp.c:4493

The macro ADD_PROPERTY_HELPER switches id on us from the string "__count__" to the integer 0. It uses the macro SPROP_USERID, which in this case returns sprop->shortid instead of sprop->id. Here's what sprop looks like in js_SetPropertyHelper():

(gdb) p *sprop
$1 = {id = 158536092, getter = 0x212f83 <obj_getCount>, 
  setter = 0x212f83 <obj_getCount>, slot = 45, attrs = 1 '\001', 
  flags = 4 '\004', shortid = 0, parent = 0x9a8c490, kids = 0x0, shape = 14803}

Brendan, any thoughts?

Comment 4

[Johnny Stenback (:jst)]

brendan/igor: any thoughts about my above comment?

Comment 5

[Brendan Eich [:brendan]]

Attachment: proposed fix

Johnny, can you test this? My build is going, may be a while.

/be

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 314504 [details] [diff] [review]
proposed fix

Stealing. This works and is obviously correct. On the 1.8 branch, thanks to split windows, we end up setting window[0] via window.__count__ because of this.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 314504 [details] [diff] [review]
proposed fix

One-line fix (essentially), safe and easy.

/be

Comment 8

[Johnny Stenback (:jst)]

Tested locally here too (sorry for not getting to that earlier). Works wonderfully.

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 314504 [details] [diff] [review]
proposed fix

a1.9=beltzner

Comment 10

[Brendan Eich [:brendan]]

Fixed:

js/src/jsobj.c 3.462

/be

Comment 11

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-426711.js,v  <--  regress-426711.js
initial revision: 1.1

Comment 12

[Bob Clary [:bc] (inactive)]

v 1.9.0