Bug 66783 – fast/loader/document-destruction-within-unload.html causes an assertion failure in the next test (currently, document-with-fragment-url-1.html)

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

[Alexey Proskuryakov]

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)

[Nate Chapin]

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.

[Nate Chapin]

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

[Alexey Proskuryakov]

This is still failing on bots. How bad is this assertion - should the test be disabled, or should the whole patch be reverted?

[Nate Chapin]

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

[Alexey Proskuryakov]

Skipped the test on Mac in <http://trac.webkit.org/changeset/94099>. It cannot be marked as failing, because it breaks the next test.

[David Kilzer (:ddkilzer)]

<rdar://problem/10448886>

[Balazs Kelemen]

I believe I am facing with the same issue on WebKit2 (with Qt port but I think it is a general bug): bug 89467.

[Alexey Proskuryakov]

Nate, do you think that you'll have a chance to look into this?

[Nate Chapin]

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

[Nate Chapin]

Created attachment 161584 [details]
patch

[Darin Adler]

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.

[Nate Chapin]

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

[Nate Chapin]

Created attachment 161785 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 161785 [details]
Patch for landing

Clearing flags on attachment: 161785

Committed r127347: <http://trac.webkit.org/changeset/127347>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Balazs Kelemen]

*** Bug 89467 has been marked as a duplicate of this bug. ***

[Nate Chapin]

*** Bug 95441 has been marked as a duplicate of this bug. ***