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

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Kate Cheney]

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

[Kate Cheney]

<rdar://problem/69216768>

[Kate Cheney]

Created attachment 409593 [details]
Patch

[Alex Christensen]

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

[Kate Cheney]

Created attachment 409599 [details]
Patch for landing

[Kate Cheney]

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

[EWS]

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

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

[Darin Adler]

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.

[Kate Cheney]

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