Bug 66783 - fast/loader/document-destruction-within-unload.html causes an assertion failure in the next test (currently, document-with-fragment-url-1.html)
Summary: fast/loader/document-destruction-within-unload.html causes an assertion failu...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nate Chapin
URL:
Keywords: InRadar
: 89467 95441 (view as bug list)
Depends on: 64741
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-23 10:13 PDT by Alexey Proskuryakov
Modified: 2012-09-04 09:47 PDT (History)
6 users (show)

See Also:


Attachments
patch (5.09 KB, patch)
2012-08-30 16:42 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
Patch for landing (5.61 KB, patch)
2012-08-31 15:21 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2011-08-23 10:13:39 PDT
run-webkit-tests fast/loader/document-destruction-within-unload.html --repeat 2

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebKit              	0x00000001036707a8 -[WebHTMLView setDataSource:] + 88 (WebHTMLView.mm:3774)
1   com.apple.WebKit              	0x000000010363243d WebFrameLoaderClient::transitionToCommittedForNewPage() + 1917 (WebFrameLoaderClient.mm:1301)
2   com.apple.WebCore             	0x0000000104318d86 WebCore::FrameLoader::transitionToCommitted(WTF::PassRefPtr<WebCore::CachedPage>) + 1142 (FrameLoader.cpp:1928)
3   com.apple.WebCore             	0x0000000104317e29 WebCore::FrameLoader::commitProvisionalLoad() + 1625 (FrameLoader.cpp:1780)
4   com.apple.WebCore             	0x000000010408cf7d WebCore::DocumentLoader::commitIfReady() + 77 (DocumentLoader.cpp:281)
5   com.apple.WebCore             	0x000000010408d054 WebCore::DocumentLoader::commitLoad(char const*, int) + 84 (DocumentLoader.cpp:300)
6   com.apple.WebCore             	0x000000010408d42a WebCore::DocumentLoader::receivedData(char const*, int) + 90 (DocumentLoader.cpp:335)
7   com.apple.WebCore             	0x0000000104c09007 WebCore::MainResourceLoader::addData(char const*, int, bool) + 87 (MainResourceLoader.cpp:169)
8   com.apple.WebCore             	0x0000000104fccc5d WebCore::ResourceLoader::didReceiveData(char const*, int, long long, bool) + 269 (ResourceLoader.cpp:304)
9   com.apple.WebCore             	0x0000000104c0b1fc WebCore::MainResourceLoader::didReceiveData(char const*, int, long long, bool) + 1052 (MainResourceLoader.cpp:464)
10  com.apple.WebCore             	0x0000000104fcd7dd WebCore::ResourceLoader::didReceiveData(WebCore::ResourceHandle*, char const*, int, int) + 157 (ResourceLoader.cpp:463)
11  com.apple.WebCore             	0x0000000104fc94e2 -[WebCoreResourceHandleAsDelegate connection:didReceiveData:lengthReceived:] + 306 (ResourceHandleMac.mm:854)
Comment 1 Nate Chapin 2011-08-23 10:36:36 PDT
Sigh.  I saw a similar assertion failure while testing my patch, but I thought I had fixed those issues.

I'll look at it this afternoon if no one beats me to it.
Comment 2 Nate Chapin 2011-08-23 16:03:05 PDT
(In reply to comment #1)
> Sigh.  I saw a similar assertion failure while testing my patch, but I thought I had fixed those issues.
> 
> I'll look at it this afternoon if no one beats me to it.

It appears my too-clever-for-its-own-good crash fix doesn't properly retain WebDocumentLoaderMac's m_dataSource, which is getting detached too early in the new test.

Need to do some more digging to see if there's a sane way to handle this.
Comment 3 Alexey Proskuryakov 2011-08-30 11:31:17 PDT
This is still failing on bots. How bad is this assertion - should the test be disabled, or should the whole patch be reverted?
Comment 4 Nate Chapin 2011-08-30 11:33:54 PDT
(In reply to comment #3)
> This is still failing on bots. How bad is this assertion - should the test be disabled, or should the whole patch be reverted?

It's an assertion fail in debug (and null deref in release), in a place where we were use-after-freeing.  I'd propose that we mark the test failing and leave this issue open (and assign it to me).
Comment 5 Alexey Proskuryakov 2011-08-30 13:10:02 PDT
Skipped the test on Mac in <http://trac.webkit.org/changeset/94099>. It cannot be marked as failing, because it breaks the next test.
Comment 6 David Kilzer (:ddkilzer) 2011-11-15 10:32:13 PST
<rdar://problem/10448886>
Comment 7 Balazs Kelemen 2012-06-28 02:04:54 PDT
I believe I am facing with the same issue on WebKit2 (with Qt port but I think it is a general bug): bug 89467.
Comment 8 Alexey Proskuryakov 2012-08-03 13:47:36 PDT
Nate, do you think that you'll have a chance to look into this?
Comment 9 Nate Chapin 2012-08-03 13:52:20 PDT
(In reply to comment #8)
> Nate, do you think that you'll have a chance to look into this?

Probably not for at least a few weeks, but I can add it to my list of things I need to do sooner rather than later.
Comment 10 Nate Chapin 2012-08-30 16:42:44 PDT
Created attachment 161584 [details]
patch
Comment 11 Darin Adler 2012-08-31 11:50:10 PDT
Comment on attachment 161584 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161584&action=review

> Source/WebCore/ChangeLog:3
> +        fast/loader/document-destruction-within-unload.html causes assertion failures on mac and qt.

Is there a way to make a test that more directly targets this problem rather than having the symptom be assertion failures in the next test that happen on only certain platforms?

> Source/WebCore/loader/FrameLoader.cpp:1619
> +    // detachChildren() can trigger this frame's unload event, and can therefore
> +    // do terrible, terrible things. For example, an unload event that calls

Whenever possible, I prefer more precise terminology than “can do terrible things”. I want our comments to help people understand better, rather than creating fear. So I’d be tempted to say something more like “therefore script code from the webpage can run and do almost anything” or something like that.

> LayoutTests/ChangeLog:9
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

Can’t land a patch with this in it.
Comment 12 Nate Chapin 2012-08-31 15:17:30 PDT
(In reply to comment #11)
> (From update of attachment 161584 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161584&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        fast/loader/document-destruction-within-unload.html causes assertion failures on mac and qt.
> 
> Is there a way to make a test that more directly targets this problem rather than having the symptom be assertion failures in the next test that happen on only certain platforms?

If there is, I haven't figured it out. It's unfortunate that assertions during detach and cleanup show up as failures in the next test (or are swallowed if the test is run by itself).

> 
> > Source/WebCore/loader/FrameLoader.cpp:1619
> > +    // detachChildren() can trigger this frame's unload event, and can therefore
> > +    // do terrible, terrible things. For example, an unload event that calls
> 
> Whenever possible, I prefer more precise terminology than “can do terrible things”. I want our comments to help people understand better, rather than creating fear. So I’d be tempted to say something more like “therefore script code from the webpage can run and do almost anything” or something like that.

Yeah, sorry about that. FrameLoader brings out my snarky, fear-mongering side.
Comment 13 Nate Chapin 2012-08-31 15:21:30 PDT
Created attachment 161785 [details]
Patch for landing
Comment 14 WebKit Review Bot 2012-08-31 18:08:39 PDT
Comment on attachment 161785 [details]
Patch for landing

Clearing flags on attachment: 161785

Committed r127347: <http://trac.webkit.org/changeset/127347>
Comment 15 WebKit Review Bot 2012-08-31 18:08:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Balazs Kelemen 2012-09-04 05:03:55 PDT
*** Bug 89467 has been marked as a duplicate of this bug. ***
Comment 17 Nate Chapin 2012-09-04 09:47:05 PDT
*** Bug 95441 has been marked as a duplicate of this bug. ***