Bug 147339 – Crash in WebCore::DocumentLoader::willSendRequest() with ContentFilter and AppCache

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

[Brady Eidson]

Crash in WebCore::DocumentLoader::willSendRequest() with ContentFilter and AppCache

#0	0x00000001050c1040 in WebCore::ResourceLoader::identifier() const at /Volumes/Data/git/OpenSource/Source/WebCore/loader/ResourceLoader.h:92
#1	0x00000001054b9174 in WebCore::DocumentLoader::willSendRequest(WebCore::ResourceRequest&, WebCore::ResourceResponse const&) at /Volumes/Data/git/OpenSource/Source/WebCore/loader/DocumentLoader.cpp:554
#2	0x00000001054b8ca0 in WebCore::DocumentLoader::redirectReceived(WebCore::CachedResource*, WebCore::ResourceRequest&, WebCore::ResourceResponse const&) at /Volumes/Data/git/OpenSource/Source/WebCore/loader/DocumentLoader.cpp:489
#3	0x00000001050c0088 in WebCore::CachedRawResource::didAddClient(WebCore::CachedResourceClient*) at /Volumes/Data/git/OpenSource/Source/WebCore/loader/cache/CachedRawResource.cpp:135
#4	0x00000001050c631c in WebCore::CachedResource::Callback::timerFired() at /Volumes/Data/git/OpenSource/Source/WebCore/loader/cache/CachedResource.cpp:779
...

The scenario is as follows:
- Content filters are on in Safari
- Visit a page that uses app cache, and has redirects for their main URL. Example is twitter.com, which uses app cache, and on iOS redirects to mobile.twitter.com
- When DocumentLoader adds itself as a client to the CachedRawResource for the main resource, the CachedResource doesn't actually add it synchronously. From CachedResource::addClientToSet:
        // Certain resources (especially XHRs and main resources) do crazy things if an asynchronous load returns
        // synchronously (e.g., scripts may not have set all the state they need to handle the load).
        // Therefore, rather than immediately sending callbacks on a cache hit like other CachedResources,
        // we schedule the callbacks and ensure we never finish synchronously.
        m_clientsAwaitingCallback.add(client, std::make_unique<Callback>(*this, *client));
- Before that timer fires, the main resource finishes loading, which clears the ResourceLoader from the CachedResource.
- Then the timer fires, actually adding the DocumentLoader as a client, and then all of the delegate callbacks are replayed.
- This includes the redirect, which redirects to a URL in the app cache, which sets up a substitute resource load and attempts to grab the load identifier for later use.

*phew*

Even though the steps that lead to this crash are well understood at this point, creating a layout test for it has proven to be an uphill battle so far.

There's also a further downstream crash where the existence of a ResourceLoader is incorrectly assumed. That will also be reflected in the upcoming patch.

<rdar://problem/21960398>

[Brady Eidson]

Created attachment 257612 [details]
Patch v1

[Alexey Proskuryakov]

Comment on attachment 257612 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=257612&action=review

> Source/WebCore/loader/cache/CachedResource.h:263
> +    unsigned long identifierForLoadWithoutResourceLoader() const { return m_identifierForLoadWithoutResourceLoader; }

Load without resource loader?

[WebKit Commit Bot]

Comment on attachment 257612 [details]
Patch v1

Clearing flags on attachment: 257612

Committed r187466: <http://trac.webkit.org/changeset/187466>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.