543304 - Serializing a cloned node before its original, plus XTF, can lead to a JS_ASSERT through garbage collection

Status: RESOLVED INVALID

(Core Graveyard :: XTF, defect)

Description

[Alex Vincent [:WeirdAl]]

Steps to reproduce at present:
   1. hg clone http://hg.mozilla.org/releases/mozilla-1.9.2 mozilla
   2. hg clone http://hg.mozdev.org/verbosio verbosio
   3. cd verbosio
   4. hg update mozilla-bug534412
   5. cd ..
   6. Copy verbosio/src/experimental/edit-template to mozilla/extensions/edit-template
   7. In your .mozconfig, add ac_add_options --enable=extensions=default,edit-template
   8. Also add ac_add_options --enable-tests
   9. Build Mozilla Firefox.
  10. cd fx-debug
  11. python _tests/testing/mochitest/runtests.py --chrome --test-path=extensions/edit/tests/test_templates.xul
  12. When the page appears, click on the "Run Test" button, and buckle your seatbelt.

Stack trace at https://bugzilla.mozilla.org/attachment.cgi?id=422126 .

The offending code:

const serial = C_c["@mozilla.org/xmlextras/xmlserializer;1"]
                  .createInstance(C_i.nsIDOMSerializer);
function serializeNode(aNode)
{
  var node = aNode.cloneNode(true);
  return serial.serializeToString(node, "application/xml");
}

/* Much later... */

    // xeIMarkupTemplate
    showTemplate: function showTemplate(aNode)
    {
      dump("Entering showTemplate\n\n");
      // ...

      // this creates a <xul:panel/> element, and appends <markup:ui-section/>
      var panel = this.getUserInterfacePanel();

      getLoggingService().log("Templates", C_i.xeILoggingService.INFO,
                              "Panel retrieved", serializeNode(panel));

      // This should force &lt;markup:children/&gt; elements to take XBL bindings.
      aNode.ownerDocument.documentElement.appendChild(panel);
      // ...
    },

If I move the logging line after the appendChild call, the crash does not happen.

I'm well aware this is not a minimized testcase.  I will spend a significant chunk of time over the next week or so reducing it as much as I can.  The mozilla-bug534412 branch will maintain a crashing version of this test, in smaller and smaller sizes, as I continue to reduce it.

I think I can reduce it ultimately to a xpcshell testcase.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Based on http://weblogs.mozillazine.org/weirdal/archives/020409.html
you have JS based XTF implementation. That isn't tested too well, I guess, 
since XForms extension has been using C++.

The problem seems to be that during GC there is an Unbind called, and that
enters your JS code, which does something unexpected - or perhaps
just entering JS code during GC is unexpected.

Sounds like this is an XTF bug. Calling JS implemented XTF methods during
Unbinding should be delayed, at least when cycle collector is running. 

(Btw, I see no reason to reduce the testcase to xpcshell testcase, mochitests
are usually easier to understand.)

Comment 2

[timeless]

from my perspective, a stack trace is appreciatd.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Indeed.
Alex, could you perhaps attach the stack trace here.

Comment 4

[Alex Vincent [:WeirdAl]]

(In reply to comment #2)
> from my perspective, a stack trace is appreciated.

Excerpted from attachment 422126 [details]:

 	KernelBase.dll!_DebugBreak@0()  + 0x2 bytes	
 	js3250.dll!JS_Assert(const char * s=0x6f3e6478, const char * file=0x6f3e6444, int ln=110)  Line 65	C++
 	js3250.dll!js_FillPropertyCache(JSContext * cx=0x04120970, JSObject * obj=0x05143120, unsigned int scopeIndex=0, unsigned int protoIndex=0, JSObject * pobj=0x05143120, JSScopeProperty * sprop=0x06149a58, int adding=0)  Line 110 + 0x24 bytes	C++
 	js3250.dll!js_GetPropertyHelper(JSContext * cx=0x04120970, JSObject * obj=0x05143120, int id=59513756, int cacheResult=1, int * vp=0x002fa900)  Line 4274 + 0x1d bytes	C++
 	js3250.dll!js_Interpret(JSContext * cx=0x04120970)  Line 1520 + 0x23 bytes	C++
 	js3250.dll!js_Invoke(JSContext * cx=0x04120970, unsigned int argc=1, int * vp=0x0613f3c8, unsigned int flags=0)  Line 1368 + 0x9 bytes	C++
 	xpc3250.dll!nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS * wrapper=0x060cdc28, unsigned short methodIndex=9, const XPTMethodDescriptor * info=0x04421ce0, nsXPTCMiniVariant * nativeParams=0x002fadc8)  Line 1696 + 0x1b bytes	C++
 	xpc3250.dll!nsXPCWrappedJS::CallMethod(unsigned short methodIndex=9, const XPTMethodDescriptor * info=0x04421ce0, nsXPTCMiniVariant * params=0x002fadc8)  Line 571	C++
 	xpcom_core.dll!PrepareAndDispatch(nsXPTCStubBase * self=0x081f29e0, unsigned int methodIndex=9, unsigned int * args=0x002fae88, unsigned int * stackBytesToPop=0x002fae78)  Line 114 + 0x21 bytes	C++
 	xpcom_core.dll!SharedStub()  Line 142	C++
>	gklayout.dll!nsXTFElementWrapper::UnbindFromTree(int aDeep=0, int aNullParent=1)  Line 231	C++
 	gklayout.dll!nsAttrAndChildArray::Clear()  Line 664	C++
 	gklayout.dll!nsAttrAndChildArray::~nsAttrAndChildArray()  Line 135	C++
 	gklayout.dll!nsGenericElement::~nsGenericElement()  Line 1792 + 0xb bytes	C++
 	gklayout.dll!nsXULElement::~nsXULElement()  + 0x1a bytes	C++
 	gklayout.dll!nsXULElement::`scalar deleting destructor'()  + 0xf bytes	C++
 	gklayout.dll!nsNodeUtils::LastRelease(nsINode * aNode=0x06701b98)  Line 272 + 0x21 bytes	C++
 	gklayout.dll!nsGenericElement::Release()  Line 4155 + 0xc8 bytes	C++
 	gklayout.dll!nsXULElement::Release()  Line 369 + 0xd bytes	C++
 	xpc3250.dll!XPC_SWN_Finalize(JSContext * cx=0x04120970, JSObject * obj=0x05143240)  Line 1535 + 0xe bytes	C++
 	js3250.dll!FinalizeObject(JSContext * cx=0x04120970, JSObject * obj=0x05143240)  Line 3190 + 0x10 bytes	C++
 	js3250.dll!js_GC(JSContext * cx=0x04120970, JSGCInvocationKind gckind=GC_NORMAL)  Line 3622 + 0xd bytes	C++
 	js3250.dll!JS_GC(JSContext * cx=0x04120970)  Line 2439 + 0xb bytes	C++


smaug: I agree that XTF shoulders most of the blame.  I just don't have any idea what constructs in C++ could delay the execution of JS code until GC finishes (especially since I think the XTF element and its JS component would be destroyed next, if the assert hadn't happened).  I'd love to learn, though.

I also have a funny feeling about how this might have happened.  The original document of the original panel was a data document, created from DOMParser.  In comment 0, I mentioned that if the serializeNode call happens after the appendChild call, no crash.  Could it be DOMSerializer requires an ownerDocument reference?  If so, that could explain how I triggered this crash:  the data document could have been garbage-collected too at some point - whereas the document receiving the panel is the mochitest document.

But yeah, if XTF can prevent calls into JS land when JS land is being shaken up, that would be nice.

Comment 5

[Alex Vincent [:WeirdAl]]

XTF is dead.