Bug 182532 - REGRESSION(r227758): Webpage fails to load due to crash in com.apple.WebKit: WebKit::WebFrameLoaderClient::dispatchDecidePolicyForResponse + 267
Summary: REGRESSION(r227758): Webpage fails to load due to crash in com.apple.WebKit: ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-02-06 03:18 PST by Antti Koivisto
Modified: 2018-02-07 19:30 PST (History)
6 users (show)

See Also:


Attachments
patch (4.03 KB, patch)
2018-02-06 03:27 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (4.04 KB, patch)
2018-02-06 03:30 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
Patch (3.10 KB, patch)
2018-02-07 17:10 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2018-02-06 03:18:08 PST
boom

(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xbbadbeef)
    frame #0: 0x000000047098cfe4 JavaScriptCore`::WTFCrash() at Assertions.cpp:272
    frame #1: 0x0000000103d7d381 WebKit`WTF::Function<void (WebCore::PolicyAction)>::operator(this=0x00007ffeec018890, in=Ignore)(WebCore::PolicyAction) const at Function.h:55
  * frame #2: 0x000000010450d2c4 WebKit`WebKit::WebFrameLoaderClient::dispatchDecidePolicyForResponse(this=0x00007fd3cd427640, response=0x00000004767f7b18, request=0x00000004767f7a08, function=0x00007ffeec018890)>&&) at WebFrameLoaderClient.cpp:753
    frame #3: 0x00000004624dfe91 WebCore`WebCore::FrameLoader::checkContentPolicy(this=0x00007fd3cd41c240, response=0x00000004767f7b18, function=0x00007ffeec018890)>&&) at FrameLoader.cpp:360
    frame #4: 0x0000000462497ba0 WebCore`WebCore::DocumentLoader::responseReceived(this=0x00000004767f7400, response=0x0000000475774910) at DocumentLoader.cpp:786
    frame #5: 0x000000046249955e WebCore`WebCore::DocumentLoader::responseReceived(this=0x00000004767f7400, resource=0x0000000475774700, response=0x0000000475774910) at DocumentLoader.cpp:699
    frame #6: 0x00000004624995a4 WebCore`non-virtual thunk to WebCore::DocumentLoader::responseReceived(this=0x00000004767f7400, resource=0x0000000475774700, response=0x0000000475774910) at DocumentLoader.cpp:0
    frame #7: 0x00000004625b7751 WebCore`WebCore::CachedRawResource::responseReceived(this=0x0000000475774700, response=0x00007ffeec0193d8) at CachedRawResource.cpp:201
    frame #8: 0x0000000462554665 WebCore`WebCore::SubresourceLoader::didReceiveResponse(this=0x0000000475770800, response=0x00007ffeec0193d8) at SubresourceLoader.cpp:353
    frame #9: 0x0000000104960755 WebKit`WebKit::WebResourceLoader::didReceiveResponse(this=0x00000004757f20e0, response=0x00007ffeec0193d8, needsContinueDidReceiveResponseMessage=true) at WebResourceLoader.cpp:115
Comment 1 Antti Koivisto 2018-02-06 03:18:31 PST
<rdar://problem/36414017>
Comment 2 Antti Koivisto 2018-02-06 03:27:20 PST
Created attachment 333168 [details]
patch
Comment 3 Antti Koivisto 2018-02-06 03:30:16 PST
Created attachment 333169 [details]
patch
Comment 4 Chris Dumez 2018-02-06 09:19:36 PST
Comment on attachment 333169 [details]
patch

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

> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:749
>      if (!coreFrame)

Couldn't we move those null checks *betore* setting up the policy listener on the WebFrame earlier? This way, we can keep using the function as is, and we do not unnecessarily set up a policy listener.
Comment 5 Antti Koivisto 2018-02-06 11:03:11 PST
> Couldn't we move those null checks *betore* setting up the policy listener
> on the WebFrame earlier? This way, we can keep using the function as is, and
> we do not unnecessarily set up a policy listener.

I assumed there was a reason why they are after. Specifically setUpPolicyListener may call arbitrary code (m_willSubmitFormCompletionHandlers in invalidatePolicyListener) and if those null coreFrame/provisionalDocumentLoader we would crash.

Alex needs to answer whether that was the reason the checks are after setUpPolicyListener.
Comment 6 Antti Koivisto 2018-02-06 11:07:17 PST
> I assumed there was a reason why they are after. Specifically
> setUpPolicyListener may call arbitrary code
> (m_willSubmitFormCompletionHandlers in invalidatePolicyListener) and if
> those null coreFrame/provisionalDocumentLoader we would crash.

Also it calls any existing m_policyFunction.
Comment 7 Alexey Proskuryakov 2018-02-06 11:20:25 PST
Comment on attachment 333169 [details]
patch

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

> Source/WebKit/ChangeLog:9
> +        No test case, don't know how to make one. The repro involves multipart HTTP streaming and details are hazy.

We have some tests for multipart, those are actually not hard to write. Is there some particular detail that makes this unreproducible?
Comment 8 youenn fablet 2018-02-07 17:10:22 PST
Created attachment 333341 [details]
Patch
Comment 9 Chris Dumez 2018-02-07 18:43:36 PST
Comment on attachment 333341 [details]
Patch

r=me
Comment 10 WebKit Commit Bot 2018-02-07 19:30:38 PST
Comment on attachment 333341 [details]
Patch

Clearing flags on attachment: 333341

Committed r228257: <https://trac.webkit.org/changeset/228257>
Comment 11 WebKit Commit Bot 2018-02-07 19:30:40 PST
All reviewed patches have been landed.  Closing bug.