Bug 193816

Summary: [Curl] Fix DRT crash related to private browsing.
Product: WebKit Reporter: Basuke Suzuki <Basuke.Suzuki>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, commit-queue, don.olmstead, Hironori.Fujii, mcatanzaro, ross.kirsling, takashi.komori, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=194141
Attachments:
Description Flags
Patch
none
Patch none

Description Basuke Suzuki 2019-01-24 23:56:17 PST
Many tests under storage storage/indexeddb/ have started crash after fetching storageSession from NetworkingContext.
Comment 1 Takashi Komori 2019-01-30 06:03:27 PST
Created attachment 360574 [details]
Patch
Comment 2 Takashi Komori 2019-01-30 06:20:32 PST
Curl port crashes when DRT tests private browsing mode.
This is because lack of inmplementation of WebFrameNetworkingContext::ensurePrivateBrowsingSession() for Curl port.

Attached patch fixes it.
Comment 3 Ross Kirsling 2019-01-30 09:48:53 PST
Seems good from my perspective, but I'd want Alex or Michael to give the r+ here.
Comment 4 Michael Catanzaro 2019-01-30 09:57:21 PST
Comment on attachment 360574 [details]
Patch

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

> Source/WebKitLegacy/WebCoreSupport/NetworkStorageSessionMap.cpp:115
> +    addResult.iterator->value = std::make_unique<WebCore::NetworkStorageSession>(sessionID, nullptr);

Unrelated: I think you should remove the second parameter from your NetworkStorageSession constructor. It's a curl-specific constructor, and I believe you have to always pass nullptr for it to not crash. :)
Comment 5 Alex Christensen 2019-01-30 10:45:20 PST
Comment on attachment 360574 [details]
Patch

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

Looks good to me.

> Source/WebKitLegacy/WebCoreSupport/NetworkStorageSessionMap.cpp:111
> +    auto addResult = globalSessionMap().add(sessionID, nullptr);

You could use HashMap::ensure here.

> Source/WebKitLegacy/win/WebCoreSupport/WebFrameNetworkingContext.cpp:91
> +    if (auto privateSession = NetworkStorageSessionMap::storageSession(PAL::SessionID::legacyPrivateSessionID()))
> +        return *privateSession;

This seems unnecessary.
Comment 6 Takashi Komori 2019-01-30 23:52:56 PST
Created attachment 360700 [details]
Patch
Comment 7 Takashi Komori 2019-01-30 23:57:57 PST
(In reply to Michael Catanzaro from comment #4)
> Unrelated: I think you should remove the second parameter from your
> NetworkStorageSession constructor. It's a curl-specific constructor, and I
> believe you have to always pass nullptr for it to not crash. :)

I agree.
I will open another ticket for removing the parameter.
Comment 8 Takashi Komori 2019-01-30 23:58:26 PST
(In reply to Alex Christensen from comment #5)
> Comment on attachment 360574 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=360574&action=review
> 
> Looks good to me.
> 
> > Source/WebKitLegacy/WebCoreSupport/NetworkStorageSessionMap.cpp:111
> > +    auto addResult = globalSessionMap().add(sessionID, nullptr);
> 
> You could use HashMap::ensure here.
> 
> > Source/WebKitLegacy/win/WebCoreSupport/WebFrameNetworkingContext.cpp:91
> > +    if (auto privateSession = NetworkStorageSessionMap::storageSession(PAL::SessionID::legacyPrivateSessionID()))
> > +        return *privateSession;
> 
> This seems unnecessary.

Fixed.
Comment 9 WebKit Commit Bot 2019-01-31 09:04:40 PST
Comment on attachment 360700 [details]
Patch

Clearing flags on attachment: 360700

Committed r240791: <https://trac.webkit.org/changeset/240791>
Comment 10 WebKit Commit Bot 2019-01-31 09:04:41 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2019-01-31 09:05:29 PST
<rdar://problem/47704080>