Bug 213700 - Crash in NetworkProcessProxy::getNetworkProcessConnection() lambda due to missing Optional<> value check
Summary: Crash in NetworkProcessProxy::getNetworkProcessConnection() lambda due to mis...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-06-28 08:59 PDT by David Kilzer (:ddkilzer)
Modified: 2020-06-29 14:39 PDT (History)
4 users (show)

See Also:


Attachments
Patch v1 (6.00 KB, patch)
2020-06-28 09:05 PDT, David Kilzer (:ddkilzer)
darin: review+
Details | Formatted Diff | Diff
Patch for landing (5.66 KB, patch)
2020-06-29 11:36 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2020-06-28 08:59:48 PDT
Crash in NetworkProcessProxy::getNetworkProcessConnection() lambda due to missing Optional<> value check.

NetworkProcess::createNetworkConnectionToWebProcess() can call its completion handler with an empty Optional<> argument:

    auto ipcConnection = createIPCConnectionPair();
    if (!ipcConnection) {
        completionHandler({ }, HTTPCookieAcceptPolicy::Never);
        return;
    }

The completion handler defined in NetworkProcessProxy::getNetworkProcessConnection() must be able to handle this.

Note that GPUProcess::createGPUConnectionToWebProcess() and GPUProcessProxy::getGPUProcessConnection() have the same issue.

<rdar://problem/64852903>
Comment 1 David Kilzer (:ddkilzer) 2020-06-28 09:05:26 PDT
Created attachment 402999 [details]
Patch v1
Comment 2 Darin Adler 2020-06-28 13:20:16 PDT
Comment on attachment 402999 [details]
Patch v1

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

> Source/WebKit/UIProcess/GPU/GPUProcessProxy.cpp:186
> +    sendWithAsyncReply(Messages::GPUProcess::CreateGPUConnectionToWebProcess { webProcessProxy.coreProcessIdentifier(), webProcessProxy.sessionID() }, [this, weakThis = makeWeakPtr(*this), reply = WTFMove(reply)](auto&& optionalConnectionIdentifier) mutable {

I don’t think the renaming is helpful. The code already has * and -> in it every place this is used, so it’s not like we were overlooking that this was optional. We were just overlooking that it could actually be null. In fact, I’d probably rename it to just one word "identifier".

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:136
> +    sendWithAsyncReply(Messages::NetworkProcess::CreateNetworkConnectionToWebProcess { webProcessProxy.coreProcessIdentifier(), webProcessProxy.sessionID() }, [this, weakThis = makeWeakPtr(*this), reply = WTFMove(reply)](auto&& optionalConnectionIdentifier, auto cookieAcceptPolicy) mutable {

Ditto.
Comment 3 David Kilzer (:ddkilzer) 2020-06-29 11:23:58 PDT
Comment on attachment 402999 [details]
Patch v1

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

>> Source/WebKit/UIProcess/GPU/GPUProcessProxy.cpp:186
>> +    sendWithAsyncReply(Messages::GPUProcess::CreateGPUConnectionToWebProcess { webProcessProxy.coreProcessIdentifier(), webProcessProxy.sessionID() }, [this, weakThis = makeWeakPtr(*this), reply = WTFMove(reply)](auto&& optionalConnectionIdentifier) mutable {
> 
> I don’t think the renaming is helpful. The code already has * and -> in it every place this is used, so it’s not like we were overlooking that this was optional. We were just overlooking that it could actually be null. In fact, I’d probably rename it to just one word "identifier".

I'll rename this to "identifier".

IMO, the type of every `auto` should be guessable on the line where it is declared, but I don't have the capacity or the desire to debate this.  An IDE that would tell me the type of an `auto` variable would be amazing, though.
Comment 4 David Kilzer (:ddkilzer) 2020-06-29 11:36:55 PDT
Created attachment 403090 [details]
Patch for landing
Comment 5 David Kilzer (:ddkilzer) 2020-06-29 14:01:19 PDT
Comment on attachment 403090 [details]
Patch for landing

Marking cq+ since there were no code changes from "Patch v1" and ios-wk2 is now passing.
Comment 6 EWS 2020-06-29 14:39:27 PDT
Committed r263695: <https://trac.webkit.org/changeset/263695>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403090 [details].