332971 - crash with iframe removing itself [@ nsAttrAndChildArray::Clear] [@ nsAttrAndChildArray::~nsAttrAndChildArray]

Status: RESOLVED FIXED

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

Description

[georgi - hopefully not receiving bugspam]

an iframe removing itself from the parent via javascript: uri causes memory
corruption after 2-3 reloads that may be automated.

on firefox $eip is often 0x0, on seamonkey there are clear signs of memory
corruption.

both trunk and branch.

Comment 1

[georgi - hopefully not receiving bugspam]

Attachment: crash

Comment 2

[georgi - hopefully not receiving bugspam]

stack

#0  0x0000000000865fb8 in ?? ()
#1  0x00002aaab0e8e6ad in nsAttrAndChildArray::Clear (this=0x1330ad0)
    at /opt/joro/suite-cvs/mozilla/content/base/src/nsAttrAndChildArray.cpp:647
#2  0x00002aaab0e8e713 in ~nsAttrAndChildArray (this=0x1330ad0)
    at /opt/joro/suite-cvs/mozilla/content/base/src/nsAttrAndChildArray.cpp:133
#3  0x00002aaab0ef3ba0 in ~nsGenericElement (this=0x1330aa0)
    at /opt/joro/suite-cvs/mozilla/content/base/src/nsGenericElement.cpp:870
#4  0x00002aaab0f72827 in nsGenericHTMLElement::~nsGenericHTMLElement$base ()
    at ../../../../dist/include/dom/nsIDOMElement.h:24
#5  0x00002aaab0f81c10 in ~nsHTMLBodyElement (this=0x1330aa0)
    at /opt/joro/suite-cvs/mozilla/content/html/content/src/nsHTMLBodyElement.cpp:294
#6  0x00002aaab0eed76d in nsGenericElement::Release (this=0x1330aa0)
    at /opt/joro/suite-cvs/mozilla/content/base/src/nsGenericElement.cpp:3141
#7  0x00002aaab0f81c6f in nsHTMLBodyElement::Release (this=0x1330aa0)
    at /opt/joro/suite-cvs/mozilla/content/html/content/src/nsHTMLBodyElement.cpp:298
#8  0x00002aaab0e8e6c1 in nsAttrAndChildArray::Clear (this=0x1316f90)
    at /opt/joro/suite-cvs/mozilla/content/base/src/nsAttrAndChildArray.cpp:648
#9  0x00002aaab0e8e713 in ~nsAttrAndChildArray (this=0x1316f90)
    at /opt/joro/suite-cvs/mozilla/content/base/src/nsAttrAndChildArray.cpp:133
#10 0x00002aaab0ef3ba0 in ~nsGenericElement (this=0x1316f60)
    at /opt/joro/suite-cvs/mozilla/content/base/src/nsGenericElement.cpp:870

Comment 3

[Jesse Ruderman]

Crashes with a scary stack trace on Mac too.  Top of the stack:

Thread 0 Crashed:
0   <<00000000>> 	0xc0000000 0 + -1073741824
1   org.mozilla.firefox      	0x005693a8 nsAttrAndChildArray::~nsAttrAndChildArray [unified]() + 36

Comment 4

[georgi - hopefully not receiving bugspam]

sure, the seamonkey stack is scary on linux

Comment 5

[Boris Zbarsky [:bzbarsky]]

sicking, any idea what could be up here?  We're running the JS from under BindToTree, sadly, but we should be handling this particular setup right, I'd think....

roc, we should try to figure out how to plug this case into your new setup.  :(

Comment 6

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

mrbkap has a patch

Comment 7

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

The problem was an extra release in the content sink caused by SinkContext::OpenContainer incrementing mStackPos much later than where it stuck things on the stack -- the parent.document.write in the iframe appends to the parent between those two points (inside the AppendChildTo call), which messes up the sink context's stack.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix

This fixes the problem, as dbaron said.

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 217628 [details] [diff] [review]
Fix

r=jst

Comment 10

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

I have to say, it would be nice if...

  if (mStackPos <= 0) {
    NS_ERROR("container w/o parent");

    return NS_ERROR_FAILURE;
  }

... were at the beginning of the function, before we potentially create and leak a node.

It would also be nice if the ++mStackPos were really *right* next to the code where we fill in mStack[mStackPos].  (I don't care whether the latter moves up or the former moves down, but it might matter.)

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: With that change

I'm convinced.

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 217630 [details] [diff] [review]
With that change

Yeah, looks better.

Comment 13

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

Comment on attachment 217630 [details] [diff] [review]
With that change

>   nsresult rv;
>   if (mStackPos + 1 > mStackSize) {
>     rv = GrowStack();
>     if (NS_FAILED(rv)) {
>       return rv;
>     }
>   }
> 
>+  if (mStackPos <= 0) {
>+    NS_ERROR("container w/o parent");
>+
>+    return NS_ERROR_FAILURE;
>+  }

But switch these two checks around.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 217630 [details] [diff] [review]
With that change

jst reviewed this.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 217630 [details] [diff] [review]
With that change

This fixes a use of free'd memory.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 217630 [details] [diff] [review]
With that change

Taking some liberties here... ;)

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.8 branch.

Comment 19

[georgi - hopefully not receiving bugspam]

seems fixed in today trunk.

but a very similar abuse from xbl:
Bug 332807
xul iframe doing parent.document.open() - open this
https://bugzilla.mozilla.org/attachment.cgi?id=217441

still works.

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #19)
> but a very similar abuse from xbl:

This was a bug in the HTML content sink code, and the fix was also in the HTML content sink code. Therefore, abuses from XBL would be unaffected by such a fix :-(.

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 217630 [details] [diff] [review]
With that change

approved for 1.8.0 branch, a=dveditz for drivers

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Checked into the 1.8.0 branch.

Comment 23

[Tracy Walker [:tracy]]

verified with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.4) Gecko/20060504 Firefox/1.5.0.4