489501 - "Assertion failure: OBJ_SCOPE(pobj)->object == pobj" with __proto__, marquee, appendChild

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Assertion failure: OBJ_SCOPE(pobj)->object == pobj, at /Users/jruderman/tracemonkey/js/src/jsinterp.cpp:416

Happens with JIT on or off.

Comment 1

[Igor Bukanov]

Attachment: simpler testcase (crashes Firefox when loaded)

This test case contains:

  var m = document.createElement("marquee");
  m.__proto__ = document.createTextNode("x");

  var elem = document.documentElement;
  m.appendChild;
  elem.appendChild(m);
  m.appendChild;


As before, the test case crashes with:

Assertion failure: OBJ_SCOPE(pobj)->object == pobj, at /scratch/igor/m/tm/js/src/jsinterp.cpp:416

Something strange is going with resolve hooks and appendChild.

Comment 2

[Igor Bukanov]

Here is what is going on with the example.

  var m = document.createElement("marquee");
  var text_node = document.createTextNode("x");
  m.__proto__ = text_node;
  m.appendChild;

After these 3 lines the property cache contains an entry for m.appendChild with prototype depth == 2 as appendChild was found in the prototype of the text node. The text_node itself would not own the scope so shape(text_node) == shape(text_node.__proto__).

Now, when 

  document.documentElement.appendChild(m);

is executed, it eventually calls nsXBLBinding::DoInitJSClass. That creates a new object that have as its prototype the prototype of m. In this case it is text_node. Then that object is made via JS_SetPrototype a prototype om m. At this point text_node == m.__proto__.__proto.

Now the interpreter executes:

  m.appendChild 

That eventually calls js_FullTestPropertyCache. The function see that the shape of m still matches the original one stored in the cache. Moreover, the shape of the object at the prototype chain at depth, text_node, still matches the stored value in the cache since at this point shape(text_node) == shape(text_node.__proto__) still holds. After that the cache effectively treats text_node as if it was text_node.__proto__ leading the assert.

I think the issue here is that JS_SetPrototype does not change the shapes of the scopes on the prototype chain as x.__proto__ assignments do in JS.

For references, here is the stack trace when the problematic JS_SetPrototype call happens:

#0  JS_SetPrototype (cx=0xf2bb5308, obj=0xf1c68880, proto=0xf2191120) at /scratch/igor/m/tm/js/src/jsapi.cpp:2838
#1  0xf353638d in nsXBLBinding::DoInitJSClass (cx=0xf2bb5308, global=0xf1c41f40, obj=0xf1c68880, aClassName=@0x8a8a2c0, aProtoBinding=0x8a1c098, aClassObject=0xffe77bac) at /scratch/igor/m/tm/content/xbl/src/nsXBLBinding.cpp:1355
#2  0xf353aa68 in nsXBLPrototypeBinding::InitClass (this=0x8a1c098, aClassName=@0x8a8a2c0, aContext=0xf2bb5308, aGlobal=0xf1c41f40, aScriptObject=0xf1c68880, aClassObject=0xffe77bac) at /scratch/igor/m/tm/content/xbl/src/nsXBLPrototypeBinding.cpp:846
#3  0xf35488c1 in nsXBLProtoImpl::InitTargetObjects (this=0x8a8a2c0, aBinding=0x8a1c098, aContext=0xf2bb52d0, aBoundElement=0x8ab6a58, aScriptObjectHolder=0xffe77bb0, aTargetClassObject=0xffe77bac) at /scratch/igor/m/tm/content/xbl/src/nsXBLProtoImpl.cpp:148
#4  0xf3548a39 in nsXBLProtoImpl::InstallImplementation (this=0x8a8a2c0, aBinding=0x8a1c098, aBoundElement=0x8ab6a58) at /scratch/igor/m/tm/content/xbl/src/nsXBLProtoImpl.cpp:80
#5  0xf353ad20 in nsXBLPrototypeBinding::InstallImplementation (this=0x8a1c098, aBoundElement=0x8ab6a58) at /scratch/igor/m/tm/content/xbl/src/nsXBLPrototypeBinding.cpp:541
#6  0xf3533678 in nsXBLBinding::InstallImplementation (this=0x83e93d0) at /scratch/igor/m/tm/content/xbl/src/nsXBLBinding.cpp:941
#7  0xf35335cd in nsXBLBinding::InstallImplementation (this=0x8bf4af8) at /scratch/igor/m/tm/content/xbl/src/nsXBLBinding.cpp:935
#8  0xf355621a in nsXBLService::LoadBindings (this=0x846a618, aContent=0x8ab6a58, aURL=0x84952a0, aOriginPrincipal=0x81becc8, aAugmentFlag=0, aBinding=0xffe77e5c, aResolveStyle=0xffe77e90) at /scratch/igor/m/tm/content/xbl/src/nsXBLService.cpp:670
#9  0xf2fd3a93 in nsCSSFrameConstructor::AddFrameConstructionItemsInternal (this=0x855f2c8, aState=@0xffe77f38, aContent=0x8ab6a58, aParentFrame=0xf1c7662c, aTag=0x83d50dc, aNameSpaceID=0, aStyleContext=0xf1c74634, aFlags=3, aItems=@0xffe77f80) at /scratch/igor/m/tm/layout/base/nsCSSFrameConstructor.cpp:5229
#10 0xf2fd4271 in nsCSSFrameConstructor::AddFrameConstructionItems (this=0x855f2c8, aState=@0xffe77f38, aContent=0x8ab6a58, aParentFrame=0xf1c7662c, aItems=@0xffe77f80) at /scratch/igor/m/tm/layout/base/nsCSSFrameConstructor.cpp:5195
#11 0xf2fe29a2 in nsCSSFrameConstructor::ContentAppended (this=0x855f2c8, aContainer=0xf1c4a378, aNewIndexInContainer=2) at /scratch/igor/m/tm/layout/base/nsCSSFrameConstructor.cpp:6233
#12 0xf305942d in PresShell::ContentAppended (this=0x855e018, aDocument=0xf1c47378, aContainer=0xf1c4a378, aNewIndexInContainer=2) at /scratch/igor/m/tm/layout/base/nsPresShell.cpp:4834
#13 0xf334c46d in nsNodeUtils::ContentAppended (aContainer=0xf1c4a378, aNewIndexInContainer=2) at /scratch/igor/m/tm/content/base/src/nsNodeUtils.cpp:119
#14 0xf3331c38 in nsGenericElement::doInsertChildAt (aKid=0x8ab6a58, aIndex=2, aNotify=1, aParent=0xf1c4a378, aDocument=0xf1c47378, aChildArray=@0xf1c4a394) at /scratch/igor/m/tm/content/base/src/nsGenericElement.cpp:3279
#15 0xf3331df4 in nsGenericElement::InsertChildAt (this=0xf1c4a378, aKid=0x8ab6a58, aIndex=2, aNotify=1) at /scratch/igor/m/tm/content/base/src/nsGenericElement.cpp:3205
#16 0xf3330e12 in nsGenericElement::doReplaceOrInsertBefore (aReplace=0, aNewChild=0x8ab6a78, aRefChild=0x0, aParent=0xf1c4a378, aDocument=0xf1c47378, aReturn=0xffe783dc) at /scratch/igor/m/tm/content/base/src/nsGenericElement.cpp:3953
#17 0xf3330f71 in nsGenericElement::InsertBefore (this=0xf1c4a378, aNewChild=0x8ab6a78, aRefChild=0x0, aReturn=0xffe783dc) at /scratch/igor/m/tm/content/base/src/nsGenericElement.cpp:3527
#18 0xf34207b6 in nsHTMLHtmlElement::InsertBefore (this=0xf1c4a378, newChild=0x8ab6a78, refChild=0x0, _retval=0xffe783dc) at /scratch/igor/m/tm/content/html/content/src/nsHTMLHtmlElement.cpp:57
#19 0xf3319350 in nsGenericElement::AppendChild (this=0xf1c4a378, aNewChild=0x8ab6a78, aReturn=0xffe783dc) at /scratch/igor/m/tm/content/svg/content/src/../../../base/src/nsGenericElement.h:500
#20 0xf34201e3 in nsHTMLHtmlElement::AppendChild (this=0xf1c4a378, newChild=0x8ab6a78, _retval=0xffe783dc) at /scratch/igor/m/tm/content/html/content/src/nsHTMLHtmlElement.cpp:57
#21 0xf640a8ee in nsIDOMNode_AppendChild (cx=0xf2bb5308, argc=1, vp=0x855c0b4) at dom_quickstubs.cpp:3509
#22 0xf7dc7251 in js_Interpret (cx=0xf2bb5308) at /scratch/igor/m/tm/js/src/jsinterp.cpp:5100

Comment 3

[Igor Bukanov]

Attachment: v1

Here is untested patch. It regenerates the shapes for prototype chain for JS_SetPrototype call in the same way as assignments to __proto__ do.

To simplify and share the code the patch always creates own scope for the obj with changing proto. Currently the code tries to optimize that for assignments from the script, but the code complexity IMO is just not worth it.

Comment 4

[Igor Bukanov]

Attachment: v2

The new patch is v1 synchronized with the trunk. It passed the try server. For details see the comment 3.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 375268 [details] [diff] [review]
v2

>+    JSPackedBool        cycleError;     /* true if failed due to a cycle */

Nit: call this just |cycle| so there's no confusion about an error already having been reported when it is found set to true.

>+    if (!checkForCycles || !pobj) {

Uber-nit: swap these || operands.

r=me with these picked.

/be

Comment 6

[Igor Bukanov]

Attachment: v3

addressing the nits

Comment 7

[Igor Bukanov]

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

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/f474676a65df

Comment 9

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/526c36e3e75a

Comment 10

[Aakash Desai [:aakashd]]

verified FIXED using attached testcases:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090528 Minefield/3.6a1pre ID:20090528031326

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090528 Shiretoko/3.5pre ID:20090528031233