(gdb) bt #0 0x40b3dc9a in XPCNativeInterface::GetName() const (this=0x3f8) at xpcprivate.h:1218 #1 0x40b3db90 in XPCNativeSet::FindMember(long, XPCNativeMember**, unsigned short*) const (this=0x8b493d0, name=135406484, pMember=0xbfffdeb4, pInterfaceIndex=0xbfffde7e) at xpcinlines.h:397 #2 0x40b3dc3d in XPCNativeSet::FindMember(long, XPCNativeMember**, XPCNativeInterface**) const (this=0x8b493d0, name=135406484, pMember=0xbfffdeb4, pInterface=0x3f8) at xpcinlines.h:428 #3 0x40b3d9e8 in XPCNativeSet::FindMember(long, XPCNativeMember**, XPCNativeInterface**, XPCNativeSet*, int*) const (this=0x8b493d0, name=135406484, pMember=0x3f8, pInterface=0x3f8, protoSet=0x8adb950, pIsLocal=0x3f8) at xpcinlines.h:445 #4 0x40b79144 in XPC_WN_Helper_NewResolve (cx=0x87dd5c8, obj=0x8b7e5b0, idval=135406484, flags=1, objp=0xbfffe038) at xpcwrappednativejsops.cpp:1091 #5 0x401b14e9 in js_LookupPropertyWithFlags (cx=0x87dd5c8, obj=0x8b7e5b0, id=137620232, flags=1, objp=0xbfffe0c4, propp=0xbfffe0c8) at jsobj.c:2695 #6 0x401b1247 in js_LookupProperty (cx=0x87dd5c8, obj=0x8b7e5b0, id=137620232, objp=0xbfffe0c4, propp=0xbfffe0c8) at jsobj.c:2600 #7 0x401b1cf7 in js_GetProperty (cx=0x87dd5c8, obj=0x8b7e5b0, id=137620232, vp=0xbfffe1f4) at jsobj.c:2885 #8 0x4019e0df in js_Interpret (cx=0x87dd5c8, pc=0x8b1f914 "5", result=0xbfffe2bc) at jsinterp.c:3576 #9 0x401932a0 in js_Invoke (cx=0x87dd5c8, argc=1, flags=2) at jsinterp.c:1231 #10 0x4019352e in js_InternalInvoke (cx=0x87dd5c8, obj=0x3f8, fval=1016, flags=0, argc=1, argv=0x8a85e18, rval=0xbfffe4f8) at jsinterp.c:1308 #11 0x4016bca1 in JS_CallFunctionValue (cx=0x87dd5c8, obj=0x87e85d8, fval=142956992, argc=1, argv=0x8a85e18, rval=0xbfffe4f8) at jsapi.c:4157 #12 0x413843ee in nsJSContext::CallEventHandler(JSObject*, JSObject*, unsigned, long*, long*) (this=0x87dd518, aTarget=0x87e85d8, aHandler=0x88559c0, argc=1, argv=0x8a85e18, rval=0xbfffe4f8) at nsJSEnvironment.cpp:1423 #13 0x4139bee2 in nsGlobalWindow::RunTimeout(nsTimeout*) (this=0x8982a70, aTimeout=0x8a592c0) at nsGlobalWindow.cpp:6243 #14 0x4139c79a in nsGlobalWindow::TimerCallback(nsITimer*, void*) (aTimer=0x8a59308, aClosure=0x8a592c0) at nsGlobalWindow.cpp:6602 #15 0x401047a4 in nsTimerImpl::Fire() (this=0x8a59308) at nsTimerImpl.cpp:400 #16 0x4010492a in handleTimerEvent (aEvent=0x881c468) at nsTimerImpl.cpp:467 #17 0x400fda19 in PL_HandleEvent (self=0x881c468) at plevent.c:688 #18 0x400fd8f2 in PL_ProcessPendingEvents (self=0x80daae8) at plevent.c:623 #19 0x40100472 in nsEventQueueImpl::ProcessPendingEvents() (this=0x80fa348) at nsEventQueue.cpp:417 #20 0x41d18fae in event_processor_callback (source=0x83ce9e8, condition=G_IO_IN, data=0x8b493d0) at nsAppShell.cpp:67 #21 0x40686def in g_io_unix_dispatch () from /opt/gnome/lib/libglib-2.0.so.0 #22 0x40664148 in g_main_dispatch () from /opt/gnome/lib/libglib-2.0.so.0 #23 0x406651a8 in g_main_context_dispatch () from /opt/gnome/lib/libglib-2.0.so.0 #24 0x406655a8 in g_main_context_iterate () from /opt/gnome/lib/libglib-2.0.so.0 #25 0x40665bf7 in g_main_loop_run () from /opt/gnome/lib/libglib-2.0.so.0 #26 0x403896ff in gtk_main () from /opt/gnome/lib/libgtk-x11-2.0.so.0 #27 0x41d19558 in nsAppShell::Run() (this=0x8164e38) at nsAppShell.cpp:139 #28 0x41c886a5 in nsAppStartup::Run() (this=0x8163ac0) at nsAppStartup.cpp:207 #29 0x08051663 in main1 (argc=2, argv=0xbfffeae4, nativeApp=0x80b54c8) at nsAppRunner.cpp:1248 #30 0x08051fdb in main (argc=2, argv=0xbfffeae4) at nsAppRunner.cpp:1737 (gdb)

Assertions before the crash (hundreds of them): ###!!! ASSERTION: Potential deadlock between XPCJSRuntime::mMapLockMonitor@811b230 and Monitor@bfffdb40: 'Error', file nsAutoLock.cpp, line 302 Break: at file nsAutoLock.cpp, line 302 ###!!! ASSERTION: XPConnect is being called on a scope without a 'Components' property! This is pretty much always bad. It usually means that native code is making a callback to an interface implemented in JavaScript, but the document where the JS object was created has already been cleared and the global properties of that document's window are *gone*. Generally this indicates a problem that should be addressed in the design and use of the callback code. : 'Error', file xpcwrappednativescope.cpp, line 589 Break: at file xpcwrappednativescope.cpp, line 589 ###!!! ASSERTION: Principal mismatch. Not good: '!aAllowShortCircuit || result == doGetObjectPrincipal(aCx, aObj, PR_FALSE)', file nsScriptSecurityManager.cpp, line 2169 Break: at file nsScriptSecurityManager.cpp, line 2169

Split window fallout? Not a JS engine bug, probably not an XPConnect bug. If split window, then DOM. /be

jst, can you look at this?

I think this is a dup of bug 323978.

I kinda doubt it is, but let's recheck once that's fixed, I guess.

This actually does look a lot like the crashes mrbkap and I have seen in bug 323978, but yeah, let's wait and see...

This fixes this crash. This includes mrbkap's "incomplete patch" from bug 323978 with some changes, and also fixes to our wrapper reparenting code in nsContentUtils and fixes to all our BindToTree() implementations where they weren't updating the node's nodeinfo to reflect the node's new owner document in case the node was being bound to a subtree that's not part of a document. I'll be attaching a diff -w for reviewers shortly.

diff -w for review. See previous attachment for explanation of the changes.

Oh, and this *is* the exact same crash that's mentioned in bug 323978. I decided to put the fix here since this bug is only about the crash, not about the assertions etc, which are unrelated to the crash. To elaborate a bit more on what's causing this crash, we end up in a state where xpconnect's notion of a DOM node's scope doesn't match the scope that's reachable through the node's owner document. A couple of problems cause this to happen, the first one is fixed by mrbkap's changes to make a document know it's scope even after it doesn't have a script global object any longer, that caused nsNodeSH::PreCreate() to not find the right scope in those cases. The second cause is that nsContentUtils::ReparentContentWrapper() didn't always reparent the wrappers when needed, and the third cause was that nsContentUtils::ReparentContentWrapper() *did* reparent the wrappers correctly, but the DOM nodes owner document (through its node info) wasn't updated properly to reflect the fact that the node is now part of a new scope. The patch fixes all those issues and makes stirdom run these testcases w/o crashing.

Going to review this tonight, I hope. Looks pretty reasonable in general, though. I'd like some docs on the nsIDocument change explaining clearly how this differs from GetScriptGlobalObject. And with this change, could we move over whatever script global object getters need to be moved over and remove the hack that returns the container's window if mIsGoingAway is true? That would be very wonderful, imo. And would actually fix some extant bfcache issues.

Jesse: Please file a new bug, possibly after this one has been checked in in case fixes are made to the patch.

Comment on attachment 217534 [details] [diff] [review] Fix (diff -w for reviewers). >Index: content/base/public/nsContentUtils.h > static nsresult doReparentContentWrapper(nsIContent *aChild, Please rename this parameter as you do in the .cpp file. >Index: content/base/public/nsIDocument.h Something like this, perhaps: /** * Get the object that all of the content wrappers whose owner document * this is are parented to. Unlike the script global object, this * never changes once it's set. Use this object when you're trying * to find a content wrapper in XPConnect. */ >+ virtual nsIScriptGlobalObject* GetScopeObject() = 0; >+ >Index: content/base/src/nsDocument.cpp >+ if (!mScopeObject) { Comment here that the scope object is immutable? >Index: content/base/src/nsDocument.h >+ nsWeakPtr mScopeObject; Is it worth comment that this is a weak reference to avoid reference cycles (and thus to avoid leaking). >Index: dom/src/base/nsDOMClassInfo.cpp > // Get the script global object from the document. This comment wants some updating! r=mrbkap

Comment on attachment 217534 [details] [diff] [review] Fix (diff -w for reviewers). >Index: content/base/public/nsIDocument.h > virtual nsIScriptGlobalObject* GetScriptGlobalObject() const = 0; > virtual void SetScriptGlobalObject(nsIScriptGlobalObject* aGlobalObject) = 0; > >+ virtual nsIScriptGlobalObject* GetScopeObject() = 0; The more I think about it, the more I suspect that a lot (almost all? Maybe except for nsDocument::GetWindow?) callers really want what GetScopeObject does -- get the scope object, if it exists and lives. I'm ok with having the two separate methods for now in the interests of getting this landed and maybe even onto branches, but for 1.9 I really think we should make GetScriptGlobalObject do what GetScopeObject does in this patch, fix the very few callers that would need fixing, and remove GetScopeObject. >Index: content/base/src/nsContentUtils.cpp >+ // Whether or not aChild is already wrapped we must iterate through >+ // its decendants descendants > since there's no guaratee guarantee > that a decendant descendant >@@ -900,53 +895,57 @@ nsContentUtils::ReparentContentWrapper(n >+ if (!globalObj) { >+ // No global object around; can't find the old wrapper w/o the old >+ // context or global object Well, we can find it with the new context, apparently, as long as we have the old global obj. Fix the comment? >+ return doReparentContentWrapper(aContent, cx, globalObj, newParent); So... if we're renaming the last arg to doReparentContentWrapper to aNewGlobal, this doesn't really make sense. Maybe we should just call it aObjectInNewScope or something? :( Or get the actual globak off the newParent here and pass that in? >Index: content/base/src/nsDOMAttribute.cpp >- newNodeInfo.swap(mNodeInfo); >+ mNodeInfo.swap(newNodeInfo); Why does it matter? >Index: content/base/src/nsDocument.cpp > nsDocument::SetScriptGlobalObject(nsIScriptGlobalObject *aScriptGlobalObject) > mScriptGlobalObject = aScriptGlobalObject; >+ if (!mScopeObject) { >+ mScopeObject = do_GetWeakReference(aScriptGlobalObject); >+ } #ifdef DEBUG else { nsCOMPtr<nsIScriptGlobalObject> scope = do_QueryReferent(mScopeObject); NS_ASSERTION(scope == aScriptGlobalObject, "script global and scope mismatch"); } #endif or something? Just to make sure people don't screw this up? >Index: content/base/src/nsGenericDOMDataNode.cpp >@@ -692,37 +692,47 @@ nsGenericDOMDataNode::BindToTree(nsIDocu Toss in an XXX XBL2/sXBL comment here like for generic element? >Index: content/base/src/nsGenericElement.cpp >+ // check the nodeinfo managers whether we need a new nodeinfo That's only a few blocks down.. maybe "handle a change in our ownerDocument"? >+ if (newOwnerDocument) { ... >+ rv = slots->mAttributeMap->SetOwnerDocument(newOwnerDocument); Shouldn't this be: if (newOwnerDocument && newOwnerDocument != oldOwnerDocument) or something? >Index: content/xul/content/src/nsXULElement.cpp Same comments as for nsGenericElement. Looks reasonable otherwise.

(In reply to comment #19) > >Index: content/base/src/nsDOMAttribute.cpp > >- newNodeInfo.swap(mNodeInfo); > >+ mNodeInfo.swap(newNodeInfo); > > Why does it matter? It doesn't, I just changed it for consistency's sake since all other code that changes mNodeInfo does it this way. I can undo that if you prefer to leave it... I changed the rest, I'm putting up an interdiff with the changes requested. The code in nsContentUtils.cpp changed somewhat, so please look that over in the interdiff.

This is a diff from a tree with the previous diff to a tree with the issues mentioned here fixed.

Comment on attachment 217638 [details] [diff] [review] Fix issues found by bz and mrbkap. Thanks!

Comment on attachment 217638 [details] [diff] [review] Fix issues found by bz and mrbkap. > I can undo that if you prefer to leave it... Maybe just land it in a separate checkin so it doesn't look like it's needed to fix this bug (eg on branches)? sr=bzbarsky

Could this have caused bug 333697?

(The checkin for this bug was at 2006-04-10 20:49.)

We should evaluate this for the branches.

(In reply to comment #25) > (The checkin for this bug was at 2006-04-10 20:49.) Shouldn't this bug be marked FIXED on trunk, then?

Yes, it should.

This checkin made the new_parent variable unused; it looks like it could be removed.

new_parent variable removed.

The release team is really nervous about this one. I sent email to drivers and the security group looking for advice. This fix has causes some regressions (bug 333697, bug 335333). There may be other issues lurking out there. This is coming in late so there isn't very much bake time. At the same time, this is a serious security issue. It fixes the crash part of bug 323978 which involves accessing deleted memory. This is similar to the firedrill bug for 1503.

> With the patch, I still see crashes when running Stir DOM on pages with lots of > iframes, such as the examples under http://www.squarefree.com/bug321299/. Bug 335896 covers most (possibly all) of the remaining crashes.

Did this ever get checked in on 1.8 branch?

Comment on attachment 217638 [details] [diff] [review] Fix issues found by bz and mrbkap. approved for 1.8.0 branch, a=dveditz for drivers

This will unfortunately need to wait for 1.8.0.5 as the patches in this bug don't apply cleanly at all, and even worse, they depend on the fix for bug 27382 which is something we really don't want on the branch as is. Our options are quite few here, we need to come up with a branch safe fix for bug 27382, and then successfully merge these changes to the branch(es). Not something that can reasonably be done in time for 1.8.0.4 :(

Comment on attachment 217638 [details] [diff] [review] Fix issues found by bz and mrbkap. Unapproving for 1.8.0.4 and moving to next release per comment 36

How do you guys feel about landing this on the 1.8.0 branch? Is it really safe enough given that it caused some regressions on the trunk, i.e. might it cause other regressions on the branch? And what needs to be ported for it to land? I guess at the least bug 27382 would need to be ported.

I think if we could get more baking for it (e.g. on trunk and 1.8.1 branch? Might want that since trunk testing is so low) it would be ok for 1.8.0.5, hopefully... That said, we should figure out what the plan is with the nsIDocument change in this patch as far as branches go.

Comment on attachment 217638 [details] [diff] [review] Fix issues found by bz and mrbkap. Jonas says we'll get a combined branch patch for all the related regressions.

This patch is the backporting of all of relevant patches here. However, I still see scary assertions followed by iloops or null pointer derefs in nsContentIterator code. This patch doesn't include Jonas' changes to make text nodes have an mNodeInfoManager.

The contentiterator crash could be bug 335896 maybe?

(In reply to comment #42) > The contentiterator crash could be bug 335896 maybe? Yeah, that seems very possible (I bet any use of the content iterator on a document that this has happened to will hang or crash).

Blake, does this patch include the fixes for the regressions: 333697, 335785 and 335333?

(In reply to comment #44) > Blake, does this patch include the fixes for the regressions: 333697, 335785 > and 335333? Yes, it does.

Getting nervous about landing this in 1.8.0.5, want more testing for regressions and it's very unlikely people will stumble over the conditions for this crash. Moving to 1.8.0.6

We're interested in getting a patch for this in *before* FF2beta1.

Retargeting to B2 as we are still waiting for review and this missed the 1.5.0.5 train - so b1 is no worse than shipping version. We'll pick it up in b2.

Comment on attachment 226234 [details] [diff] [review] Branch patch, maybe - In nsGenericElement::doReplaceOrInsertBefore(): if (!newContentIsXUL) { - nsContentUtils::ReparentContentWrapper(newContent, aParent, - container.GetOwnerDoc(), - old_doc); + res = nsContentUtils::ReparentContentWrapper(newContent, aParent, + container.GetOwnerDoc(), + old_doc); + NS_ENSURE_SUCCESS(res, res); This looks like the right thing to do, but we don't do this on the trunk yet. If we want to do this we should land this separately, and land it first on the trunk etc. r+sr=jst w/o the above change.

Comment on attachment 226234 [details] [diff] [review] Branch patch, maybe a=dbaron on behalf of drivers. Please check in to MOZILLA_1_8_BRANCH and mark fixed1.8.1 once you have.

(In reply to comment #50) > (From update of attachment 226234 [details] [diff] [review] [edit]) > a=dbaron on behalf of drivers. Please check in to MOZILLA_1_8_BRANCH and mark > fixed1.8.1 once you have. Would be good to get this checked in ASAP.

This is the full patch, and not just the changes that this bug incurred. The other parts of this patch are from Jonas, who backported peterv's patch from bug 27382. I realized that that part of the patch hadn't quite made it into a bug or technically gotten approval before now, and I wanted to make sure that it did.

Comment on attachment 229722 [details] [diff] [review] Full patch r+sr=jst for the nodeinfo parts as well.

Blake, does that "full patch" roll in the regression fixes too, or would those land separately?

(In reply to comment #54) > Blake, does that "full patch" roll in the regression fixes too, or would those > land separately? They're rolled in.

Checked into the 1.8 branch.

I had to back this out of the 1.8 branch. I'll investigate the problems it caused on the 1.8 branch and check it back in.

Let's try that again.

Comment on attachment 230656 [details] [diff] [review] Better full patch approved for 1.8.0. branch, a=dveditz for drivers

I just checked a combined fix (including all known regressions) into the 1.8.0 branch.

https://bugzilla.mozilla.org/attachment.cgi?id=206678, unzipped and loaded index.html from disk, ff2b2 winxp, linux no crash verified fixed 1.8

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.7pre) Gecko/20060821 Firefox/1.5.0.7pre https://bugzilla.mozilla.org/attachment.cgi?id=206678, unzipped and loaded index.html from disk no crash. verified 1.8.0.7

This bug's steps to reproduce were insane, so in-testsuite-. I will check in the testcase in bug 400349 (which was fixed by this patch) and mark that bug as in-testsuite+ instead.