Bug 19891

Summary: Broken HTML object elements cause de-reference of pointer to freed memory
Product: WebKit Reporter: Chris Brichford <chrisb>
Component: Page LoadingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Critical CC: ap
Priority: P1 Keywords: HasReduction, InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.4   
Attachments:
Description Flags
Patch to add debug code to make problem 100% reproducible in Debug and Release builds.
none
test case
none
Patch to add ASSERT to catch problem sooner.
none
Patch to add debug code to make problem 100% reproducible in Debug and Release builds.
none
Proposed fix
none
proposed fix with test
mrowe: review-
Patch for bug 19891 ap: review+

Description Chris Brichford 2008-07-03 15:30:35 PDT
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.
Comment 1 Chris Brichford 2008-07-03 15:31:50 PDT
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.
Comment 2 Chris Brichford 2008-07-03 15:32:27 PDT
Created attachment 22071 [details]
test case
Comment 3 Chris Brichford 2008-07-03 15:33:15 PDT
Created attachment 22072 [details]
Patch to add ASSERT to catch problem sooner.
Comment 4 Chris Brichford 2008-07-03 15:33:58 PDT
Comment on attachment 22071 [details]
test case

Attempting to change mime type of attachment.
Comment 5 Chris Brichford 2008-07-03 15:35:50 PDT
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.
Comment 6 Chris Brichford 2008-07-03 15:47:18 PDT
Created attachment 22074 [details]
Patch to add debug code to make problem 100% reproducible in Debug and Release builds.

Fixing bug in debug code.
Comment 7 Chris Brichford 2008-07-03 15:50:21 PDT
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.
Comment 8 Mark Rowe (bdash) 2008-07-03 16:45:13 PDT
<rdar://problem/6054249>
Comment 9 Chris Brichford 2008-07-24 13:39:13 PDT
This bug was introduced by changeset 30438, which fixed bug 16760
Comment 10 Chris Brichford 2008-07-24 13:45:04 PDT
Created attachment 22469 [details]
Proposed fix

Attaching proposed fix.
Comment 11 Dave Hyatt 2008-07-24 13:58:09 PDT
Comment on attachment 22469 [details]
Proposed fix

r=me
Comment 12 Alexey Proskuryakov 2008-07-25 07:12:22 PDT
Please land this with a test case! Ideally, the patch should include its test cases in LayoutTests.
Comment 13 Chris Brichford 2008-07-25 09:34:33 PDT
(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

Comment 14 Alexey Proskuryakov 2008-07-25 12:34:13 PDT
(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?
Comment 15 Chris Brichford 2008-07-25 13:17:48 PDT
(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.
Comment 16 Chris Brichford 2008-07-25 16:23:23 PDT
Created attachment 22484 [details]
proposed fix with test

Added LayoutTest to proposed fix.
Comment 17 Alexey Proskuryakov 2008-07-26 01:31:09 PDT
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.
Comment 18 Mark Rowe (bdash) 2008-07-26 22:11:59 PDT
Comment on attachment 22469 [details]
Proposed fix

Clearing review flag on obsolete patch.
Comment 19 Mark Rowe (bdash) 2008-07-26 22:12:40 PDT
Comment on attachment 22484 [details]
proposed fix with test

Marking as r- due to Alexey's comments in Comment #17.
Comment 20 Mihnea Ovidenie 2008-08-06 10:36:54 PDT
Created attachment 22683 [details]
Patch for bug 19891

Rework the former patch
Comment 21 Alexey Proskuryakov 2008-08-10 07:54:19 PDT
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.
Comment 22 Mihnea Ovidenie 2008-08-12 06:24:29 PDT
(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?
Comment 23 Alexey Proskuryakov 2008-08-12 07:02:37 PDT
(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.
Comment 24 mitz 2008-08-12 14:49:25 PDT
Landed in <http://trac.webkit.org/changeset/35697>.