Bug 183791 – QuickLook.NavigationDelegate API test is failing on iOS with async policy delegates

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

[Chris Dumez]

QuickLook.NavigationDelegate API test is failing on iOS with async policy delegates.

[Chris Dumez]

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.

[Chris Dumez]

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

[Chris Dumez]

Created attachment 336150 [details]
Patch

[Alex Christensen]

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.

[Chris Dumez]

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?

[Alex Christensen]

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.

[Chris Dumez]

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

[Alex Christensen]

Comment on attachment 336150 [details]
Patch

:|
You're right.  r=me

[Darin Adler]

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 *);

[Chris Dumez]

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.

[WebKit Commit Bot]

Comment on attachment 336150 [details]
Patch

Clearing flags on attachment: 336150

Committed r229776: <https://trac.webkit.org/changeset/229776>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/38683883>

[Alex Christensen]

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