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)
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.
(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.
This is still failing on bots. How bad is this assertion - should the test be disabled, or should the whole patch be reverted?
(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).
Skipped the test on Mac in <http://trac.webkit.org/changeset/94099>. It cannot be marked as failing, because it breaks the next test.
<rdar://problem/10448886>
I believe I am facing with the same issue on WebKit2 (with Qt port but I think it is a general bug): bug 89467.
Nate, do you think that you'll have a chance to look into this?
(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.
Created attachment 161584 [details] patch
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.
(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.
Created attachment 161785 [details] Patch for landing
Comment on attachment 161785 [details] Patch for landing Clearing flags on attachment: 161785 Committed r127347: <http://trac.webkit.org/changeset/127347>
All reviewed patches have been landed. Closing bug.
*** Bug 89467 has been marked as a duplicate of this bug. ***
*** Bug 95441 has been marked as a duplicate of this bug. ***