Bug 216934

Summary:

CrashTracer: com.apple.WebKit.Networking in NetworkSession::firstPartyHostCNAMEDomain() code

Product:

WebKit

Reporter:

Kate Cheney <katherine_cheney>

Component:

WebKit Misc.

Assignee:

Kate Cheney <katherine_cheney>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

achristensen, darin, ddkilzer, wilander

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch for landing	none

Description Kate Cheney 2020-09-24 10:10:47 PDT

This seems to be crashing at auto iterator = m_firstPartyHostCNAMEDomains.find(firstPartyHost);

Comment 1 Kate Cheney 2020-09-24 10:12:32 PDT

<rdar://problem/69216768>

Comment 2 Kate Cheney 2020-09-24 10:27:14 PDT

Created attachment 409593 [details]
Patch

Comment 3 Alex Christensen 2020-09-24 10:40:49 PDT

Comment on attachment 409593 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkSession.cpp:300
> +    if (firstPartyHost.isNull())

Alternatively you could use decltype(m_firstPartyHostCNAMEDomains)::isValidKey

Comment 4 Kate Cheney 2020-09-24 10:55:34 PDT

Created attachment 409599 [details]
Patch for landing

Comment 5 Kate Cheney 2020-09-24 10:55:56 PDT

(In reply to Alex Christensen from comment #3)
> Comment on attachment 409593 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=409593&action=review
> 
> > Source/WebKit/NetworkProcess/NetworkSession.cpp:300
> > +    if (firstPartyHost.isNull())
> 
> Alternatively you could use
> decltype(m_firstPartyHostCNAMEDomains)::isValidKey

fancy!!

Comment 6 EWS 2020-09-24 11:29:27 PDT

Committed r267540: <https://trac.webkit.org/changeset/267540>

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

Comment 7 Darin Adler 2020-09-24 12:19:09 PDT

Comment on attachment 409599 [details]
Patch for landing

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

> Source/WebKit/NetworkProcess/NetworkSession.cpp:303
> +    if (!decltype(m_firstPartyHostCNAMEDomains)::isValidKey(firstPartyHost))
> +        return WTF::nullopt;
> +
>      auto iterator = m_firstPartyHostCNAMEDomains.find(firstPartyHost);

I think our HashTable design is working against us here (and elsewhere) we should add a safeFind function that does this. Or rename cpntains/find/get/take/remove to fastContains/fastFind/fastGet/fastTake/fastRemove and make this check a part of the slower ones. Not sure about "fast" and "safe" in the naming, but I think there are many cases we’d be wiling to spend the extra branches checking for reserved hash table key values.

It’s a trickier design question if we could do this for add/set/ensure, since we’d have to define failure behavior.

Comment 8 Kate Cheney 2020-09-24 13:16:42 PDT

(In reply to Darin Adler from comment #7)
> Comment on attachment 409599 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=409599&action=review
> 
> > Source/WebKit/NetworkProcess/NetworkSession.cpp:303
> > +    if (!decltype(m_firstPartyHostCNAMEDomains)::isValidKey(firstPartyHost))
> > +        return WTF::nullopt;
> > +
> >      auto iterator = m_firstPartyHostCNAMEDomains.find(firstPartyHost);
> 
> I think our HashTable design is working against us here (and elsewhere) we
> should add a safeFind function that does this. Or rename
> cpntains/find/get/take/remove to
> fastContains/fastFind/fastGet/fastTake/fastRemove and make this check a part
> of the slower ones. Not sure about "fast" and "safe" in the naming, but I
> think there are many cases we’d be wiling to spend the extra branches
> checking for reserved hash table key values.
> 
> It’s a trickier design question if we could do this for add/set/ensure,
> since we’d have to define failure behavior.

Good idea, we can at least do the find() case to start. Tracking in rdar://problem/69521312.