WebKit crashes while trying to load the content from page cache. (gdb) bt #0 0x00007ffff67ac452 in WebCore::CachedRawResource::responseReceived (this=0x7fffdea40a00, response=...) at /home/cswei/project/tot/Source/WebCore/loader/cache/CachedRawResource.cpp:132 #1 0x00007ffff680ad6e in WebCore::SubresourceLoader::didReceiveResponse (this=0x7fffdea00800, response=...) at /home/cswei/project/tot/Source/WebCore/loader/SubresourceLoader.cpp:184 #2 0x00007ffff67c764e in substituteResourceDeliveryTimerFired (this=<optimized out>) at /home/cswei/project/tot/Source/WebCore/loader/DocumentLoader.cpp:1100 #3 WebCore::DocumentLoader::substituteResourceDeliveryTimerFired (this=<optimized out>) at /home/cswei/project/tot/Source/WebCore/loader/DocumentLoader.cpp:1081 #4 0x00007ffff6964ea2 in WebCore::ThreadTimers::sharedTimerFiredInternal (this=0x7fffde9f80f0) at /home/cswei/project/tot/Source/WebCore/platform/ThreadTimers.cpp:129 #5 0x00007ffff3e6ff19 in QObject::event(QEvent*) () from /usr/local/Qt-5.1.0/lib/libQt5Core.so.5 #6 0x00007ffff4d339d4 in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /usr/local/Qt-5.1.0/lib/libQt5Widgets.so.5 #7 0x00007ffff4d36c91 in QApplication::notify(QObject*, QEvent*) () from /usr/local/Qt-5.1.0/lib/libQt5Widgets.so.5 #8 0x00007ffff3e47724 in QCoreApplication::notifyInternal(QObject*, QEvent*) () from /usr/local/Qt-5.1.0/lib/libQt5Core.so.5 #9 0x00007ffff3e90acc in QTimerInfoList::activateTimers() () from /usr/local/Qt-5.1.0/lib/libQt5Core.so.5 #10 0x00007ffff3e912dd in ?? () from /usr/local/Qt-5.1.0/lib/libQt5Core.so.5 #11 0x00007fffef264a5d in g_main_context_dispatch () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 #12 0x00007fffef265258 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 #13 0x00007fffef265429 in g_main_context_iteration () from /lib/x86_64-linux-gnu/libglib-2.0.so.0 #14 0x00007ffff3e91ab4 in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/local/Qt-5.1.0/lib/libQt5Core.so.5 #15 0x00007ffff3e464bb in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/local/Qt-5.1.0/lib/libQt5Core.so.5 #16 0x00007ffff3e4a580 in QCoreApplication::exec() () from /usr/local/Qt-5.1.0/lib/libQt5Core.so.5 #17 0x00000000004156b0 in launcherMain (app=...) at /home/cswei/project/tot/Tools/QtTestBrowser/qttestbrowser.cpp:54 #18 main (argc=1, argv=<optimized out>) at /home/cswei/project/tot/Tools/QtTestBrowser/qttestbrowser.cpp:318 (gdb) list 131 126 } 127 128 void CachedRawResource::responseReceived(const ResourceResponse& response) 129 { 130 CachedResourceHandle<CachedRawResource> protect(this); 131 if (!m_identifier) 132 m_identifier = m_loader->identifier(); 133 CachedResource::responseReceived(response); 134 CachedResourceClientWalker<CachedRawResourceClient> w(m_clients); 135 while (CachedRawResourceClient* c = w.next()) (gdb) p m_loader $1 = {m_ptr = 0x0} This is caused by the commit: b124248928cde28a4a55c7982b2dcb6e7200d492 for bug: https://bugs.webkit.org/show_bug.cgi?id=104969, and commit a3265403bade6d561d38c58139bddd93ff271496 for https://bugs.webkit.org/show_bug.cgi?id=112722, both of which merge MainResourceLoader into DocumentLoader. Before these patches, when a main resource loading fails, the MainResourceLoader will try appcache before notifying the DocumentLoader, and will keep the loading status appropriate when loading the substitute data from the AppCache. After these patches, this functionality was moved to DocumentLoader::mainReceivedError(), which tries to load from Appcache. But when the application reaches here, the ResourceLoader(SubResourceLoader) is already is in finishing state after ResourceLoader::didFail() and SubresourceLoader::didFail(), and the CachedResources associated with them already destroyed. We should move the ApplicationCache check the first thing when a resource loading fails (info ResourceLoader::didFail(ResourceHandle, Error)). which now only takes care of Subresource but not MainResource.
Created attachment 202960 [details] Patch
Comment on attachment 202960 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=202960&action=review > Source/WebCore/ChangeLog:8 > + Webkit crashes while loading content from Application Cache > + https://bugs.webkit.org/show_bug.cgi?id=116814 > + > + Reviewed by NOBODY (OOPS!). > + > + No new tests, refactor appcache loading. This doesn't make sense. If this is fixing a crash, it is not just refactoring. And if is a crash fix, it should have a test case.
For Apple engineers: see also <rdar://problem/13744574>.
(In reply to comment #3) > For Apple engineers: see also <rdar://problem/13744574>. And possibly 13997217
Created attachment 203188 [details] Patch
Comment on attachment 203188 [details] Patch Attachment 203188 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/693290 New failing tests: fast/dom/location-new-window-no-crash.html
Created attachment 203191 [details] Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-16 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3
Comment on attachment 203188 [details] Patch Don't think that regression is relevant. re-raise the commit flag to try
I do get a crash with this test on Mac, although with a different stack trace: Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.WebCore 0x00007fff927e2c61 WebCore::ApplicationCacheHost::scheduleLoadFallbackResourceFromApplicationCache(WebCore::ResourceLoader*, WebCore::ApplicationCache*) + 273 1 com.apple.WebCore 0x00007fff92905429 WebCore::ApplicationCacheHost::maybeLoadFallbackForMainError(WebCore::ResourceRequest const&, WebCore::ResourceError const&) + 153 2 com.apple.WebCore 0x00007fff92c9e746 WebCore::DocumentLoader::mainReceivedError(WebCore::ResourceError const&) + 38 3 com.apple.WebCore 0x00007fff927bd3b6 WebCore::CachedResource::checkNotify() + 166 4 com.apple.WebCore 0x00007fff927e2e15 WebCore::SubresourceLoader::didFail(WebCore::ResourceError const&) + 197
Comment on attachment 203188 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203188&action=review > LayoutTests/http/tests/appcache/main-resource-fallback-for-network-error-crash.html:13 > +function setNetworkEnabled(state) This function is never called. I don't think that the test verifies that the bug is fixed when running in an automated manner. Please verify tests by removing the associated code changes, and confirming that they actually fail without those. > LayoutTests/http/tests/appcache/main-resource-fallback-for-network-error-crash.html:34 > + if (window.testRunner) > + { > + var evt = document.createEvent("MouseEvents"); > + evt.initMouseEvent("click"); > + var link = document.getElementById("Link"); > + link.dispatchEvent(evt); > + return; > + } There is no need to simulate a click to navigate, you can just do window.location = "resources/does-not-exist";
Created attachment 203410 [details] Patch
Comment on attachment 203410 [details] Patch Does this test crash without the fix?
(In reply to comment #12) > (From update of attachment 203410 [details]) > Does this test crash without the fix? Yes.
Does it crash in DumpRenderTree? There is nothing in the test that implements "please make sure the server is down or disconnect your device from any data connection so the connection request to the server will fail".
(In reply to comment #14) > Does it crash in DumpRenderTree? > > There is nothing in the test that implements "please make sure the server is down or disconnect your device from any data connection so the connection request to the server will fail". Yes, we don't have the mechanism to simulate the network error(not http error 3xx, 4xx, 5xx) with DumpRenderTree (at least I am not aware of). So we can't reproduce the problem with automation test. So I moved them to ManualTests. If you know the way to simulate the physical network error, please let me know and I can move them to automation test under LayoutTests.
We have a way to simulate a disconnect, and we use it in most application cache tests. You had this function in the second version of the patch, setNetworkEnabled().
Created attachment 203552 [details] Patch
(In reply to comment #16) > We have a way to simulate a disconnect, and we use it in most application cache tests. You had this function in the second version of the patch, setNetworkEnabled(). Thanks for the comments, Alex. I come up with an automation test case that can reproduce the crash following your suggestions, please see that in the new patch above.
Comment on attachment 203552 [details] Patch Should we fix this bug in ApplicationCacheHost::maybeLoadFallbackForError instead? It can tell if the resource is the main resource. I think we could just eliminate maybeLoadFallbackForMainError or make it a private function. Seems that would be more elegant. Naturally we’d still delete the function call from DocumentLoader::mainReceivedError, but we’d leave ResourceLoader::didFail alone. What do you think? r=me to do this fix, but I think my refinement could make the division of responsibilities between ResourceLoader and ApplicationCacheHost better.
(In reply to comment #19) > (From update of attachment 203552 [details]) > Should we fix this bug in ApplicationCacheHost::maybeLoadFallbackForError instead? It can tell if the resource is the main resource. I think we could just eliminate maybeLoadFallbackForMainError or make it a private function. Seems that would be more elegant. Naturally we’d still delete the function call from DocumentLoader::mainReceivedError, but we’d leave ResourceLoader::didFail alone. > > What do you think? > > r=me to do this fix, but I think my refinement could make the division of responsibilities between ResourceLoader and ApplicationCacheHost better. That sounds like a reasonable suggestion. I'll try that, thanks.
Created attachment 203557 [details] Patch
Comment on attachment 203557 [details] Patch Commit it.
Comment on attachment 203557 [details] Patch Clearing flags on attachment: 203557 Committed r151099: <http://trac.webkit.org/changeset/151099>
All reviewed patches have been landed. Closing bug.