Bug 183791

Summary: QuickLook.NavigationDelegate API test is failing on iOS with async policy delegates
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebCore Misc.Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, aestes, beidson, commit-queue, darin, dbates, ews-watchlist, japhet, webkit-bug-importer, wenson_hsieh, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 180568    
Attachments:
Description Flags
Patch none

Description Chris Dumez 2018-03-20 12:41:54 PDT
QuickLook.NavigationDelegate API test is failing on iOS with async policy delegates.
Comment 1 Chris Dumez 2018-03-20 12:53:29 PDT
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.
Comment 2 Chris Dumez 2018-03-20 12:57:25 PDT
_sendDidReceiveResponseIfNecessary in PreviewLoader.mm calls _resourceLoader->didReceiveResponse(response, nullptr); and does not pass in a completion handler.
Comment 3 Chris Dumez 2018-03-20 14:29:25 PDT
Created attachment 336150 [details]
Patch
Comment 4 Alex Christensen 2018-03-20 14:35:50 PDT
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 5 Chris Dumez 2018-03-20 14:38:53 PDT
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?
Comment 6 Alex Christensen 2018-03-20 14:41:43 PDT
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.
Comment 7 Chris Dumez 2018-03-20 14:42:38 PDT
(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 8 Alex Christensen 2018-03-20 14:44:29 PDT
Comment on attachment 336150 [details]
Patch

:|
You're right.  r=me
Comment 9 Darin Adler 2018-03-20 14:53:53 PDT
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 10 Chris Dumez 2018-03-20 15:28:38 PDT
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 11 WebKit Commit Bot 2018-03-20 15:54:25 PDT
Comment on attachment 336150 [details]
Patch

Clearing flags on attachment: 336150

Committed r229776: <https://trac.webkit.org/changeset/229776>
Comment 12 WebKit Commit Bot 2018-03-20 15:54:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2018-03-20 15:55:21 PDT
<rdar://problem/38683883>
Comment 14 Alex Christensen 2018-03-20 17:48:27 PDT
(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.