Bug 167759 – Regression (iOS 10.3 beta): 3rd party apps can crash when custom protocols cause loads to be synchronously cancelled

• Status: ASSIGNED
• Component: WebCore Misc.

[Brady Eidson]

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.

[Brady Eidson]

<rdar://problem/30129966>

[Brady Eidson]

Created attachment 300454 [details]
Incomplete patch

Not yet for review as I'm working on a test, but the code change is this simple.

[Brady Eidson]

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.

[Brady Eidson]

A test is not possible at this time.

[Brady Eidson]

Created attachment 300539 [details]
PFR

[Brady Eidson]

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.

[Alexey Proskuryakov]

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.

[Brady Eidson]

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

[Alexey Proskuryakov]

Non-dangerous changes are welcome :)

[Brady Eidson]

Created attachment 300548 [details]
PFR

[Alexey Proskuryakov]

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.