Bug 116814 – Webkit crashes while loading content from Application Cache

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

[Charles Wei]

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.

[Charles Wei]

Created attachment 202960 [details]
Patch

[Sam Weinig]

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.

[Alexey Proskuryakov]

For Apple engineers: see also <rdar://problem/13744574>.

[Brady Eidson]

(In reply to comment #3)
> For Apple engineers: see also <rdar://problem/13744574>.

And possibly 13997217

[Charles Wei]

Created attachment 203188 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Charles Wei]

Comment on attachment 203188 [details]
Patch

Don't think that regression is relevant. re-raise the commit flag to try

[Alexey Proskuryakov]

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

[Alexey Proskuryakov]

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";

[Charles Wei]

Created attachment 203410 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 203410 [details]
Patch

Does this test crash without the fix?

[Charles Wei]

(In reply to comment #12)
> (From update of attachment 203410 [details])
> Does this test crash without the fix?

Yes.

[Alexey Proskuryakov]

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

[Charles Wei]

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

[Alexey Proskuryakov]

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

[Charles Wei]

Created attachment 203552 [details]
Patch

[Charles Wei]

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

[Darin Adler]

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.

[Charles Wei]

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

[Charles Wei]

Created attachment 203557 [details]
Patch

[Charles Wei]

Comment on attachment 203557 [details]
Patch

Commit it.

[WebKit Commit Bot]

Comment on attachment 203557 [details]
Patch

Clearing flags on attachment: 203557

Committed r151099: <http://trac.webkit.org/changeset/151099>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.