Bug 167759

Summary: Regression (iOS 10.3 beta): 3rd party apps can crash when custom protocols cause loads to be synchronously cancelled
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebCore Misc.Assignee: Brady Eidson <beidson>
Status: ASSIGNED ---    
Severity: Normal CC: achristensen, ap
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Incomplete patch
none
PFR
ap: review-
PFR none

Description Brady Eidson 2017-02-02 14:43:28 PST
Regression (iOS 10.3 beta): 3rd party apps can crash when custom protocols cause loads to be synchronous cancelled

When a custom protocol tries to redirect back to a normal protocol (e.g. file://), and they do so synchronously when the NSURLConnection is started, WebKit will crash.

Custom protocols are not supposed to do that synchronously, so WebCore::ResourceHandle doesn't handle it.

Putting the [NSURLConnection start] on a 0-delay fixes this.

I'm also working on a test, but it may not be possible.
Comment 1 Brady Eidson 2017-02-02 14:44:03 PST
<rdar://problem/30129966>
Comment 2 Brady Eidson 2017-02-02 14:45:37 PST
Created attachment 300454 [details]
Incomplete patch

Not yet for review as I'm working on a test, but the code change is this simple.
Comment 3 Brady Eidson 2017-02-02 19:54:24 PST
Still have hope that I can reproduce this in a testing environment, but if it takes longer than tomorrow morning, I'll just prep the patch for review anyways.
Comment 4 Brady Eidson 2017-02-03 09:39:00 PST
A test is not possible at this time.
Comment 5 Brady Eidson 2017-02-03 09:42:29 PST
Created attachment 300539 [details]
PFR
Comment 6 Brady Eidson 2017-02-03 09:43:23 PST
I'll attach the test I have that is *99%* of the way there, but because TestWebKitAPI grants universal access, and reproducing this relies on *not* having universal access, we can't quite reproduce.
Comment 7 Alexey Proskuryakov 2017-02-03 09:46:47 PST
Comment on attachment 300539 [details]
PFR

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

> Source/WebCore/platform/network/mac/ResourceHandleMac.mm:292
> +        RunLoop::current().dispatch([protectedThis = makeRef(*this), this]() {

This looks very dangerous to me, as it introduces races with stopping. I don't think that we should make this change.
Comment 8 Brady Eidson 2017-02-03 09:49:59 PST
(In reply to comment #7)
> Comment on attachment 300539 [details]
> PFR
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=300539&action=review
> 
> > Source/WebCore/platform/network/mac/ResourceHandleMac.mm:292
> > +        RunLoop::current().dispatch([protectedThis = makeRef(*this), this]() {
> 
> This looks very dangerous to me, as it introduces races with stopping. I
> don't think that we should make this change.

That's a good point.

Instead of sharing your viewpoint that the change is automatically terrible, I'm instead going to modify the change to gracefully handle that case.
Comment 9 Alexey Proskuryakov 2017-02-03 10:06:28 PST
Non-dangerous changes are welcome :)
Comment 10 Brady Eidson 2017-02-03 10:29:31 PST
Created attachment 300548 [details]
PFR
Comment 11 Alexey Proskuryakov 2017-02-03 12:44:05 PST
Comment on attachment 300548 [details]
PFR

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

> Source/WebCore/platform/network/mac/ResourceHandleMac.mm:292
> +        d->m_connectionStartTimer.startOneShot(0);

Will the timer fire for synchronous XHR? We spin the run loop in WebCoreSynchronousLoaderRunLoopMode only, so I expect it to not work.

We probably need the fix in ResourceHandle::schedule too, as we call -start there as well. Also, not sure about ResourceHandle::platformLoadResourceSynchronously - can we crash there for the same reason?

> Source/WebCore/platform/network/mac/ResourceHandleMac.mm:312
> +    [connection() start];

We probably need BEGIN_BLOCK_OBJC_EXCEPTIONS/END_BLOCK_OBJC_EXCEPTIONS here too, given that we had it around the original location.