Bug 183791

The issue is that we commit the load before we've even validated the response: 0x122480000 - DocumentLoader::continueAfterContentPolicy(2) x-apple-ql-id://E98B3934-F318-4778-823F-AE27DE4B5EFB/x-apple-ql-magic/pages.pages 1 0x117212ec8 WebCore::DocumentLoader::continueAfterContentPolicy(WebCore::PolicyAction) 2 0x11721ef97 WTF::Function<void (WebCore::PolicyAction)>::CallableWrapper<WebCore::DocumentLoader::responseReceived(WebCore::ResourceResponse const&, WTF::CompletionHandler<void ()>&&)::$_7>::call(WebCore::PolicyAction) 3 0x10d6fc64c WebKit::WebFrame::invalidatePolicyListener() 4 0x11722ca0a WebCore::FrameLoader::stopLoading(WebCore::UnloadEventPolicy) 5 0x117234d3b WebCore::FrameLoader::transitionToCommitted(WebCore::CachedPage*) 6 0x11723452f WebCore::FrameLoader::commitProvisionalLoad() 7 0x117213991 WebCore::DocumentLoader::commitLoad(char const*, int) 8 0x117294242 WebCore::CachedRawResource::notifyClientsDataWasReceived(char const*, unsigned int) 9 0x117293ffb WebCore::CachedRawResource::updateBuffer(WebCore::SharedBuffer&) 10 0x11726771b WebCore::SubresourceLoader::didReceiveDataOrBuffer(char const*, int, WTF::RefPtr<WebCore::SharedBuffer, WTF::DumbPtrTraits<WebCore::SharedBuffer> >&&, long long, WebCore::DataPayloadType) 11 0x117267588 WebCore::SubresourceLoader::didReceiveData(char const*, unsigned int, long long, WebCore::DataPayloadType) 12 0x12384a161 -[QLThreadInvoker connectionDidReceiveDataLengthReceived:] 13 0x10cf84d5e __NSThreadPerformPerform 14 0x10ecebbb1 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ 15 0x10ecd04af __CFRunLoopDoSources0 16 0x10eccfa6f __CFRunLoopRun 17 0x10eccf30b CFRunLoopRunSpecific 18 0x10cf3fb4a -[NSRunLoop(NSRunLoop) runMode:beforeDate:] 19 0x10cf3fa25 -[NSRunLoop(NSRunLoop) run] 20 0x1108f08c9 _xpc_objc_main 21 0x1108f2d73 xpc_main 22 0x10ce93257 main 23 0x110539955 start So our Quicklook logic does not wait for the response policy to be complete because sending the data.

_sendDidReceiveResponseIfNecessary in PreviewLoader.mm calls _resourceLoader->didReceiveResponse(response, nullptr); and does not pass in a completion handler.

Created attachment 336150 [details] Patch

Comment on attachment 336150 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336150&action=review > Source/WebCore/loader/ios/PreviewLoader.mm:53 > + long long _lengthReceived; Use SharedBuffer::size instead of saving the same information in a duplicate location.

Comment on attachment 336150 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336150&action=review >> Source/WebCore/loader/ios/PreviewLoader.mm:53 >> + long long _lengthReceived; > > Use SharedBuffer::size instead of saving the same information in a duplicate location. Is it really duplicate information? > Source/WebCore/loader/ios/PreviewLoader.mm:145 > + _resourceLoader->didReceiveData(bufferedData->data(), bufferedData->size(), _lengthReceived, DataPayloadBytes); Note here that we send both bufferedData->size() and _lengthReceived. Or is _lengthReceived the total amount of data we've received so far? while bufferedData->size() is only the size of the latest chunk of data we've received?

One of those is supposed to be the number of bytes received over the network and the other is the size of the data being pointed to. For example, gzip-encoded messages would have one be the encoded length and one the decoded length.

(In reply to Alex Christensen from comment #6) > One of those is supposed to be the number of bytes received over the network > and the other is the size of the data being pointed to. For example, > gzip-encoded messages would have one be the encoded length and one the > decoded length. So then, it is not duplicate information, is it? The buffer size may differ from lengthReceived no?

Comment on attachment 336150 [details] Patch :| You're right. r=me

Comment on attachment 336150 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336150&action=review > Source/WebCore/platform/SharedBuffer.h:69 > + void append(NSData *); Instead of append should we have a combination of both create and append? static void append(RefPtr<SharedBuffer>&, NSData *);

Comment on attachment 336150 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336150&action=review >> Source/WebCore/platform/SharedBuffer.h:69 >> + void append(NSData *); > > Instead of append should we have a combination of both create and append? > > static void append(RefPtr<SharedBuffer>&, NSData *); I merely wanted to match the API for CF types below: static Ref<SharedBuffer> create(CFDataRef); void append(CFDataRef); It looked weird in my patch that I had to cast for append() but not create(). I am not sure it is worth introducing a new method here.

Comment on attachment 336150 [details] Patch Clearing flags on attachment: 336150 Committed r229776: <https://trac.webkit.org/changeset/229776>

All reviewed patches have been landed. Closing bug.

<rdar://problem/38683883>

(In reply to Darin Adler from comment #9) > Comment on attachment 336150 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=336150&action=review > > > Source/WebCore/platform/SharedBuffer.h:69 > > + void append(NSData *); We already had non-static appends that take a CFDataRef, a const SharedBuffer&, a const char*/size_t, and a Vector<char>&&. We also have a create that takes an NSData * and just calls the CF version. I think this fits quite nicely.