Make sure we never end up with NetworkDataTaskCocoa TaskIdentifier conflicts

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Make sure we never end up with NetworkDataTaskCocoa TaskIdentifier conflicts.

[Chris Dumez]

Created attachment 382930 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 382930 [details]
Patch

Clearing flags on attachment: 382930

Committed r252141: <https://trac.webkit.org/changeset/252141>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56953662>

[Alex Christensen]

Comment on attachment 382930 [details]
Patch

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

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1247
> +    RELEASE_ASSERT(!map.contains(taskIdentifier));

This will be hit in practice.  I think we should just roll this out and fix it the right way in https://bugs.webkit.org/show_bug.cgi?id=203934

[Chris Dumez]

(In reply to Alex Christensen from comment #5)
> Comment on attachment 382930 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=382930&action=review
> 
> > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1247
> > +    RELEASE_ASSERT(!map.contains(taskIdentifier));
> 
> This will be hit in practice.  I think we should just roll this out and fix
> it the right way in https://bugs.webkit.org/show_bug.cgi?id=203934

I did not add any new assertion, so I am not sure I understand why.

Please do NOT revert.

[Chris Dumez]

Comment on attachment 382930 [details]
Patch

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

>>> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1247
>>> +    RELEASE_ASSERT(!map.contains(taskIdentifier));
>> 
>> This will be hit in practice.  I think we should just roll this out and fix it the right way in https://bugs.webkit.org/show_bug.cgi?id=203934
> 
> I did not add any new assertion, so I am not sure I understand why.
> 
> Please do NOT revert.

I don't think we generally revert patches without proof that something is wrong. You claim that this assertion will be hit in practice but have not explained at all why it would be. Also, note that I merely turned this debug assertion into a release one. If it really gets hit, I'd argue a crash is better than a security bug.

My patch is simply refactoring the code to have it in a single place and strengthening the assertions. Therefore, I am not sure what you mean by fixing the right way. https://bugs.webkit.org/show_bug.cgi?id=203934 re-enables isolated sessions, it does not simply fix things.