Bug 193816 – [Curl] Fix DRT crash related to private browsing.

• Component: Platform
• Status: RESOLVED FIXED

[Basuke Suzuki]

Many tests under storage storage/indexeddb/ have started crash after fetching storageSession from NetworkingContext.

[Takashi Komori]

Created attachment 360574 [details]
Patch

[Takashi Komori]

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.

[Ross Kirsling]

Seems good from my perspective, but I'd want Alex or Michael to give the r+ here.

[Michael Catanzaro]

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. :)

[Alex Christensen]

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.

[Takashi Komori]

Created attachment 360700 [details]
Patch

[Takashi Komori]

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

[Takashi Komori]

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

[WebKit Commit Bot]

Comment on attachment 360700 [details]
Patch

Clearing flags on attachment: 360700

Committed r240791: <https://trac.webkit.org/changeset/240791>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/47704080>