321299 - Crash [@ XPCNativeInterface::GetName] involving frames

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Mats Palmgren (inactive)]

(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)     

Comment 2

[Mats Palmgren (inactive)]

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

Comment 4

[Brendan Eich [:brendan]]

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

/be

Comment 6

[Mats Palmgren (inactive)]

Attachment: (frame content for Testcase #1)

Comment 8

[Jesse Ruderman]

jst, can you look at this?

Comment 9

[Jesse Ruderman]

I think this is a dup of bug 323978.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

Comment 11

[Johnny Stenback (:jst)]

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

Comment 12

[Johnny Stenback (:jst)]

Attachment: Fix

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.

Comment 13

[Johnny Stenback (:jst)]

Attachment: Fix (diff -w for reviewers).

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

Comment 14

[Johnny Stenback (:jst)]

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.

Comment 16

[Boris Zbarsky [:bzbarsky]]

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.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

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 19

[Boris Zbarsky [:bzbarsky]]

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.

Comment 20

[Johnny Stenback (:jst)]

(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.

Comment 21

[Johnny Stenback (:jst)]

Attachment: Fix issues found by bz and mrbkap.

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

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

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

Thanks!

Comment 23

[Boris Zbarsky [:bzbarsky]]

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

Comment 24

[Boris Zbarsky [:bzbarsky]]

Could this have caused bug 333697?

Comment 25

[Jesse Ruderman]

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

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

We should evaluate this for the branches.

Comment 27

[Daniel Veditz [:dveditz]]

(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?

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

Yes, it should.

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 30

[Johnny Stenback (:jst)]

new_parent variable removed.

Comment 31

[Tim Riley [:timr]]

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.

Comment 33

[Jesse Ruderman]

> 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.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Did this ever get checked in on 1.8 branch?

Comment 35

[Daniel Veditz [:dveditz]]

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

approved for 1.8.0 branch, a=dveditz for drivers

Comment 36

[Johnny Stenback (:jst)]

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 37

[Daniel Veditz [:dveditz]]

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

Comment 38

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 39

[Boris Zbarsky [:bzbarsky]]

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 40

[Daniel Veditz [:dveditz]]

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.

Comment 41

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Branch patch, maybe

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.

Comment 42

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The contentiterator crash could be bug 335896 maybe?

Comment 43

[Blake Kaplan (:mrbkap) (inactive)]

(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).

Comment 44

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 45

[Blake Kaplan (:mrbkap) (inactive)]

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

Yes, it does.

Comment 46

[Daniel Veditz [:dveditz]]

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

Comment 47

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 48

[Mike Schroepfer]

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 49

[Johnny Stenback (:jst)]

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 50

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 51

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 52

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Full patch

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 53

[Johnny Stenback (:jst)]

Comment on attachment 229722 [details] [diff] [review]
Full patch

r+sr=jst for the nodeinfo parts as well.

Comment 54

[Boris Zbarsky [:bzbarsky]]

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

Comment 55

[Blake Kaplan (:mrbkap) (inactive)]

(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.

Comment 56

[Blake Kaplan (:mrbkap) (inactive)]

Checked into the 1.8 branch.

Comment 57

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 58

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Better full patch

Comment 59

[Blake Kaplan (:mrbkap) (inactive)]

Let's try that again.

Comment 60

[Daniel Veditz [:dveditz]]

Comment on attachment 230656 [details] [diff] [review]
Better full patch

approved for 1.8.0. branch, a=dveditz for drivers

Comment 61

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 62

[Bob Clary [:bc] (inactive)]

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

verified fixed 1.8

Comment 63

[alice nodelman [:alice] [:anode]]

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

Comment 66

[Jesse Ruderman]

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.