Bug 182861 – Use ResourceLoader to load appcache manifest

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

Use ResourceLoader to load apache manifest

[youenn fablet]

Created attachment 334001 [details]
Patch

[youenn fablet]

Created attachment 334003 [details]
Patch

[youenn fablet]

Created attachment 334005 [details]
Patch

[EWS Watchlist]

Comment on attachment 334005 [details]
Patch

Attachment 334005 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6530809

New failing tests:
http/tests/appcache/auth.html

[EWS Watchlist]

Created attachment 334012 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 334005 [details]
Patch

Attachment 334005 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6530807

New failing tests:
http/tests/appcache/auth.html

[EWS Watchlist]

Created attachment 334014 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6

[youenn fablet]

Created attachment 334044 [details]
Patch

[Alex Christensen]

Comment on attachment 334044 [details]
Patch

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

This is exciting!  Do you want it reviewed?

> Source/WebCore/loader/appcache/ApplicationCacheResourceLoader.cpp:14
> + *     * Neither the name of Google Inc. nor the names of its

This copyright should be the two-clause copyright used in new files now.

> LayoutTests/http/tests/appcache/manifest-redirect-2-expected.txt:2
> +CONSOLE MESSAGE: Application Cache manifest could not be fetched, because a redirection was attempted.

Is not supporting redirection in the manifest a new thing, or is this just a new message?

[youenn fablet]

(In reply to Alex Christensen from comment #9)
> Comment on attachment 334044 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334044&action=review
> 
> This is exciting!  Do you want it reviewed?
> 
> > Source/WebCore/loader/appcache/ApplicationCacheResourceLoader.cpp:14
> > + *     * Neither the name of Google Inc. nor the names of its
> 
> This copyright should be the two-clause copyright used in new files now.

Will fix.

> > LayoutTests/http/tests/appcache/manifest-redirect-2-expected.txt:2
> > +CONSOLE MESSAGE: Application Cache manifest could not be fetched, because a redirection was attempted.
>
> Is not supporting redirection in the manifest a new thing, or is this just a
> new message?

It is part of preexisting code checks, in ApplicationCacheGroup::didReceiveManifestResponse (removed in this patch).

[youenn fablet]

Created attachment 334053 [details]
Patch

[youenn fablet]

Created attachment 334054 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 334054 [details]
Patch

Clearing flags on attachment: 334054

Committed r228575: <https://trac.webkit.org/changeset/228575>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/37616550>

[Ryan Haddad]

Reverted r228575 for reason:

Introduced LayoutTest crashes under GuardMalloc.

Committed r228591: <https://trac.webkit.org/changeset/228591>

[Alex Christensen]

Comment on attachment 334054 [details]
Patch

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

> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:448
> +    m_loader = ApplicationCacheResourceLoader::create(documentLoader.cachedResourceLoader(), WTFMove(request), [this] (auto&& resourceOrError) {

I'll bet you need to protect something here to make it not UAF.

[youenn fablet]

(In reply to Alex Christensen from comment #17)
> Comment on attachment 334054 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334054&action=review
> 
> > Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:448
> > +    m_loader = ApplicationCacheResourceLoader::create(documentLoader.cachedResourceLoader(), WTFMove(request), [this] (auto&& resourceOrError) {
> 
> I'll bet you need to protect something here to make it not UAF.

I am not sure.
In ApplicationCacheGroup destructor, we are stopping the loads, triggering the callback with an abort error.

[Chris Dumez]

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x000000010f1db231 WebCore::ApplicationCacheResourceLoader::cancel(WebCore::ApplicationCacheResourceLoader::Error) + 145
1   com.apple.WebCore             	0x000000010e2887b4 WebCore::CachedRawResource::responseReceived(WebCore::ResourceResponse const&) + 308
2   com.apple.WebCore             	0x000000010e28825a WebCore::SubresourceLoader::didReceiveResponse(WebCore::ResourceResponse const&) + 1130
3   com.apple.WebCore             	0x000000010f1ca7a9 WebCore::ResourceLoader::didReceiveResponseAsync(WebCore::ResourceHandle*, WebCore::ResourceResponse&&, WTF::CompletionHandler<void ()>&&) + 57
4   com.apple.WebCore             	0x000000010f40e91a WebCore::ResourceHandle::didReceiveResponse(WebCore::ResourceResponse&&, WTF::CompletionHandler<void ()>&&) + 346
5   com.apple.WebCore             	0x000000010e8340f1 WTF::Function<void ()>::CallableWrapper<-[WebCoreResourceHandleAsOperationQueueDelegate connection:didReceiveResponse:]::$_4>::call() + 449
6   com.apple.JavaScriptCore      	0x000000010ae2dd00 WTF::dispatchFunctionsFromMainThread() + 176
7   com.apple.Foundation          	0x00007fff31ed7e75 __NSThreadPerformPerform + 334
8   com.apple.CoreFoundation      	0x00007fff2fcf5611 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
9   com.apple.CoreFoundation      	0x00007fff2fdacecc __CFRunLoopDoSource0 + 108
10  com.apple.CoreFoundation      	0x00007fff2fcd86a0 __CFRunLoopDoSources0 + 208
11  com.apple.CoreFoundation      	0x00007fff2fcd7b1d __CFRunLoopRun + 1293
12  com.apple.CoreFoundation      	0x00007fff2fcd7377 CFRunLoopRunSpecific + 487
13  DumpRenderTree                	0x000000010ac541b2 0x10ac41000 + 78258
14  DumpRenderTree                	0x000000010ac53508 0x10ac41000 + 75016
15  DumpRenderTree                	0x000000010ac54c8c 0x10ac41000 + 81036
16  libdyld.dylib                 	0x00007fff58581015 start + 1

[youenn fablet]

Ok, so I probably need to protect 'this' in cancel.
And there is apparently no guarantee that an ApplicationCacheGroup stays alive after didFailLoadingManifest/didFinishLoadingManifest, making the assertions I added bad.

[Chris Dumez]

(In reply to youenn fablet from comment #20)
> Ok, so I probably need to protect 'this' in cancel.
> And there is apparently no guarantee that an ApplicationCacheGroup stays
> alive after didFailLoadingManifest/didFinishLoadingManifest, making the
> assertions I added bad.

You can validate by running tests in the http/tests/appcache folder with -g.

[youenn fablet]

Created attachment 334387 [details]
Changes to fix guard malloc crashes

[youenn fablet]

Created attachment 334388 [details]
Fixing guard malloc crashes

[WebKit Commit Bot]

Comment on attachment 334388 [details]
Fixing guard malloc crashes

Rejecting attachment 334388 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 334388, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/6611674

[youenn fablet]

Created attachment 334397 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 334397 [details]
Patch for landing

Clearing flags on attachment: 334397

Committed r228892: <https://trac.webkit.org/changeset/228892>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.