Bug 218626 – Fix crashes around NetworkStorageSession::registerCookieChangeListenersIfNecessary

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Fix crashes around NetworkStorageSession::registerCookieChangeListenersIfNecessary

[Alex Christensen]

Created attachment 413326 [details]
Patch

[Chris Dumez]

Comment on attachment 413326 [details]
Patch

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

> Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:671
> +    [nsCookieStorage() _setCookiesChangedHandler:^(NSArray<NSHTTPCookie *> *, NSString *) { } onQueue:dispatch_get_main_queue()];

Why these changes?

> Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:674
> +    [nsCookieStorage() _setSubscribedDomainsForCookieChanges:[NSSet set]];

ditto.

[Alex Christensen]

(In reply to Chris Dumez from comment #2)
> Comment on attachment 413326 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=413326&action=review
> 
> > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:671
> > +    [nsCookieStorage() _setCookiesChangedHandler:^(NSArray<NSHTTPCookie *> *, NSString *) { } onQueue:dispatch_get_main_queue()];
> 
> Why these changes?
CFNetwork calls the block without checking if it is nil.  This will cause it to call a block that does nothing instead of dereferencing null and crashing.
> 
> > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:674
> > +    [nsCookieStorage() _setSubscribedDomainsForCookieChanges:[NSSet set]];
> 
> ditto.
This one is probably not necessary, but I thought it wouldn't hurt to go along with the non-null theme of the other changes.

[Chris Dumez]

Jiten tells me this is very related to rdar://64024242. Do we still need a WebKit change or is that fix at rdar://64024242 sufficient?

[Alex Christensen]

Comment on attachment 413326 [details]
Patch

He definitely fixed at least part of the problem, but the weakThis check might still be necessary.  There's no reason to not take this change to be as safe as possible.

[Alex Christensen]

Created attachment 413380 [details]
patch

[EWS]

Committed r269512: <https://trac.webkit.org/changeset/269512>

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

[Radar WebKit Bug Importer]

<rdar://problem/71119360>