Bug 66783

Summary: fast/loader/document-destruction-within-unload.html causes an assertion failure in the next test (currently, document-with-fragment-url-1.html)
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: Page LoadingAssignee: Nate Chapin <japhet>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, japhet, kbalazs, ossy, webkit-bug-importer, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 64741    
Bug Blocks:    
Attachments:
Description Flags
patch
none
Patch for landing none

Alexey Proskuryakov
Reported 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)
Attachments
patch (5.09 KB, patch)
2012-08-30 16:42 PDT, Nate Chapin 		no flags
DetailsFormatted DiffDiff
Patch for landing (5.61 KB, patch)
2012-08-31 15:21 PDT, Nate Chapin 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Nate Chapin
Comment 1 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.
Nate Chapin
Comment 2 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.
Alexey Proskuryakov
Comment 3 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?
Nate Chapin
Comment 4 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).
Alexey Proskuryakov
Comment 5 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.
David Kilzer (:ddkilzer)
Comment 6 2011-11-15 10:32:13 PST
<rdar://problem/10448886>
Balazs Kelemen
Comment 7 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.
Alexey Proskuryakov
Comment 8 2012-08-03 13:47:36 PDT
Nate, do you think that you'll have a chance to look into this?
Nate Chapin
Comment 9 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.
Nate Chapin
Comment 10 2012-08-30 16:42:44 PDT
Created attachment 161584 [details] patch
Darin Adler
Comment 11 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.
Nate Chapin
Comment 12 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.
Nate Chapin
Comment 13 2012-08-31 15:21:30 PDT
Created attachment 161785 [details] Patch for landing
WebKit Review Bot
Comment 14 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>
WebKit Review Bot
Comment 15 2012-08-31 18:08:43 PDT
All reviewed patches have been landed. Closing bug.
Balazs Kelemen
Comment 16 2012-09-04 05:03:55 PDT
*** Bug 89467 has been marked as a duplicate of this bug. ***
Nate Chapin
Comment 17 2012-09-04 09:47:05 PDT
*** Bug 95441 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.