Summary: | [iOS] Network process is crashing when launching TJMaxx app due to invalid NetworkProcess::DestroySession IPC message | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||
Component: | WebKit2 | Assignee: | Chris Dumez <cdumez> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | achristensen, darin, ddkilzer, ggaren, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Chris Dumez
2020-06-25 15:43:31 PDT
Created attachment 402822 [details]
Patch
Comment on attachment 402822 [details]
Patch
r=me
Committed r263545: <https://trac.webkit.org/changeset/263545> All reviewed patches have been landed. Closing bug and clearing flags on attachment 402822 [details]. Comment on attachment 402822 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402822&action=review > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:125 > + return [WKWebsiteDataStore defaultDataStore]; I think we need to retain the value we are returning here. And release or dealloc self. > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:127 > + return nil; I think we need to release or dealloc self here. Reopening based on Darin's comments. Reverted r263545 for reason: Patch will need refining Committed r263547: <https://trac.webkit.org/changeset/263547> Created attachment 402835 [details]
Patch
Comment on attachment 402835 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402835&action=review > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:124 > + if (!(self = [super init])) Why don't we just call _initWithConfiguration with a non-persistent configuration instead of all this mess? Comment on attachment 402835 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402835&action=review >> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:124 >> + if (!(self = [super init])) > > Why don't we just call _initWithConfiguration with a non-persistent configuration instead of all this mess? That sounds like a good idea. Let me give this a try: auto configuration = adoptNS([[_WKWebsiteDataStoreConfiguration alloc] initNonPersistentConfiguration]); return [WKWebsiteDataStore _initWithConfiguration:configuration.get()]; Comment on attachment 402835 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402835&action=review > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:127 > + _bypassWebsiteDataStoreDestructor = YES; Why is a boolean needed rather than a null check? > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:128 > + [self autorelease]; Why autorelease rather than release or dealloc? > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:131 > + // FIXME: We should eventually drop this and always return nil. Not sure we really need this comment. I think linkedOnOrAfter above makes the point about our long term goals. And I think we should eventually drop this and always *raise an exception*. Created attachment 402877 [details]
Patch
Comment on attachment 402877 [details]
Patch
ObjC doesn't really like calling another initializer from init, but this does the same thing.
(In reply to Alex Christensen from comment #12) > Created attachment 402877 [details] > Patch Ok, let me validate this patch on device to make sure the app is happy with it. (In reply to Chris Dumez from comment #14) > (In reply to Alex Christensen from comment #12) > > Created attachment 402877 [details] > > Patch > > Ok, let me validate this patch on device to make sure the app is happy with > it. Yes, this works fine. Committed r263570: <https://trac.webkit.org/changeset/263570> All reviewed patches have been landed. Closing bug and clearing flags on attachment 402877 [details]. |