Bug 19891 – Broken HTML object elements cause de-reference of pointer to freed memory

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Chris Brichford]

When loading the attached test case WebCore requests that the FrameLoaderClient create multiple frames with the same owner element.  When we tear down the DOM only the frame that is pointed at by the owner element's contentFrame pointer is properly torn down.  The other frames end up using their owner element pointer to calculate topDocument during their tear down sequence.  At that point the ownerElement is free'd memory.

Steps to reproduce:
1. Apply first attached patch.  Patch works ~ revision 34962.  This patch adds debugging code that will cause a crash is Frame::ownerElement would return a reference to a free'd FrameOwnerElement.
2. Load the attached test html file via HTTP.
3. After the page loads navigate to about:blank.

I believe the second attached patch should catch the problem earier by asserting in the debug build that a new frame's owner element does not already have a content frame that points to the same owner element.

I can not get this to crash reliably in the nightly builds.

[Chris Brichford]

Created attachment 22070 [details]
Patch to add debug code to make problem 100% reproducible in Debug and Release builds.

Attaching debug code to make bug 100% reproducible.

[Chris Brichford]

Created attachment 22071 [details]
test case

[Chris Brichford]

Created attachment 22072 [details]
Patch to add ASSERT to catch problem sooner.

[Chris Brichford]

Comment on attachment 22071 [details]
test case

Attempting to change mime type of attachment.

[Chris Brichford]

Loading the test file with the debug code in the first patch directly out of this bug report causes the bug to happen for me.

[Chris Brichford]

Created attachment 22074 [details]
Patch to add debug code to make problem 100% reproducible in Debug and Release builds.

Fixing bug in debug code.

[Chris Brichford]

Interesting details about the test html file:
The object tag references a non-existent GIF.
There is a call to window.document.open in the onload handler.
The codetype on the object element is set to an image mime type.

[Mark Rowe (bdash)]

<rdar://problem/6054249>

[Chris Brichford]

This bug was introduced by changeset 30438, which fixed bug 16760

[Chris Brichford]

Created attachment 22469 [details]
Proposed fix

Attaching proposed fix.

[Dave Hyatt]

Comment on attachment 22469 [details]
Proposed fix

r=me

[Alexey Proskuryakov]

Please land this with a test case! Ideally, the patch should include its test cases in LayoutTests.

[Chris Brichford]

(In reply to comment #12)
> Please land this with a test case! Ideally, the patch should include its test
> cases in LayoutTests.
> 

I can add a LayoutTest, but the LayoutTest would have to hit the network.  The bug only happens if the we get a 404 response with a non-image content type header.  Also, I'm not sure I know how to detect that the test failed other than the ASSERT I added to WebCore::Frame's constructor.

The attached test case is a reduction of an existing LayoutTest: LayoutTests/dom/html/level2/html/HTMLBodyElement08.html

[Alexey Proskuryakov]

(In reply to comment #13)
> I can add a LayoutTest, but the LayoutTest would have to hit the network.  The
> bug only happens if the we get a 404 response with a non-image content type
> header.

That's fine, our HTTP tests can do that easily (see e.g. LayoutTests/http/tests/misc/resources/404image.php).

> Also, I'm not sure I know how to detect that the test failed other
> than the ASSERT I added to WebCore::Frame's constructor.

Well, if it's dereferencing freed memory, it's likely to fail when run as "run-webkit-tests --threaded", so it's OK to land the test even if it's not 100% reproducible in release mode.

> The attached test case is a reduction of an existing LayoutTest:
> LayoutTests/dom/html/level2/html/HTMLBodyElement08.html

This leaves me a bit confused, as this test doesn't hit the network - in which sense is it a reduction? Does this existing test use the same buggy code path?

[Chris Brichford]

(In reply to comment #14)
> (In reply to comment #13)
> > I can add a LayoutTest, but the LayoutTest would have to hit the network.  The
> > bug only happens if the we get a 404 response with a non-image content type
> > header.
> 
> That's fine, our HTTP tests can do that easily (see e.g.
> LayoutTests/http/tests/misc/resources/404image.php).
> 
> > Also, I'm not sure I know how to detect that the test failed other
> > than the ASSERT I added to WebCore::Frame's constructor.
> 
> Well, if it's dereferencing freed memory, it's likely to fail when run as
> "run-webkit-tests --threaded", so it's OK to land the test even if it's not
> 100% reproducible in release mode.
> 
> > The attached test case is a reduction of an existing LayoutTest:
> > LayoutTests/dom/html/level2/html/HTMLBodyElement08.html
> 
> This leaves me a bit confused, as this test doesn't hit the network - in which
> sense is it a reduction? Does this existing test use the same buggy code path?
> 

If you load LayoutTests/dom/html/level2/html/HTMLBodyElement08.html over an http connection it will exercise the same code path.

[Chris Brichford]

Created attachment 22484 [details]
proposed fix with test

Added LayoutTest to proposed fix.

[Alexey Proskuryakov]

Comment on attachment 22484 [details]
proposed fix with test

+	// Make sure we will not end up with two frames referencing the same owner element.
+	ASSERT((!(ownerElement->m_contentFrame)) || (ownerElement->m_contentFrame->ownerElement() != ownerElement));

Please use spaces, not tabs (pre-commit hook will deny landing the patch otherwise).

Index: LayoutTests/http/tests/misc/object-image-error-with-onload-expected.txt
===================================================================

Ugh, so the results are empty, and opening the test will leave the tester puzzled. Can this be improved (maybe with a document.write())?

+<object data="this.image.does.not.exist.gif" codetype="image/gif" height="10"></object>

It might help to have a comment saying that this relies on a text/html 404 page being generated by the server.

[Mark Rowe (bdash)]

Comment on attachment 22469 [details]
Proposed fix

Clearing review flag on obsolete patch.

[Mark Rowe (bdash)]

Comment on attachment 22484 [details]
proposed fix with test

Marking as r- due to Alexey's comments in Comment #17.

[Mihnea Ovidenie]

Created attachment 22683 [details]
Patch for bug 19891

Rework the former patch

[Alexey Proskuryakov]

Comment on attachment 22683 [details]
Patch for bug 19891

r=me (substantial changes being already reviewed by Dave Hyatt).

+        * ChangeLog:

This line will need to be removed from the ChangeLog when landing.

[Mihnea Ovidenie]

(In reply to comment #21)
> (From update of attachment 22683 [details] [edit])
> r=me (substantial changes being already reviewed by Dave Hyatt).
> 
> +        * ChangeLog:
> 
> This line will need to be removed from the ChangeLog when landing.
> 

Hello,
In order for the patch to be submitted, is there anything i can do to help further? Should i just wait for somebody with commit right to submit the changes into the main trunk?

[Alexey Proskuryakov]

(In reply to comment #22)
> Should i just wait for somebody with commit right to submit the
> changes into the main trunk?

That's correct - the mentioned correction is simple enough to be made by the committer.

Landed in <http://trac.webkit.org/changeset/35697>.