| Summary: | Fix crashes around NetworkStorageSession::registerCookieChangeListenersIfNecessary | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||
| Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | cdumez, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Alex Christensen
2020-11-05 11:02:19 PST
Created attachment 413326 [details]
Patch
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. (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. 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? 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.
Created attachment 413380 [details]
patch
Committed r269512: <https://trac.webkit.org/changeset/269512> All reviewed patches have been landed. Closing bug and clearing flags on attachment 413380 [details]. |