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
Brady Eidson
2017-02-02 14:43:28 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.
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. A test is not possible at this time. Created attachment 300539 [details]
PFR
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 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. (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. Non-dangerous changes are welcome :) Created attachment 300548 [details]
PFR
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. |