Bug 213625

Summary: [iOS] Network process is crashing when launching TJMaxx app due to invalid NetworkProcess::DestroySession IPC message
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: 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 Flags
Patch
none
Patch
none
Patch none

Description Chris Dumez 2020-06-25 15:43:31 PDT
Network process is crashing when launching TJMaxx app due to invalid NetworkProcess::DestroySession IPC message.
Comment 1 Chris Dumez 2020-06-25 15:43:43 PDT
<rdar://problem/64737890>
Comment 2 Chris Dumez 2020-06-25 15:58:28 PDT
Created attachment 402822 [details]
Patch
Comment 3 Geoffrey Garen 2020-06-25 16:02:14 PDT
Comment on attachment 402822 [details]
Patch

r=me
Comment 4 EWS 2020-06-25 16:43:43 PDT
Committed r263545: <https://trac.webkit.org/changeset/263545>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402822 [details].
Comment 5 Darin Adler 2020-06-25 17:02:10 PDT
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.
Comment 6 Chris Dumez 2020-06-25 17:08:16 PDT
Reopening based on Darin's comments.
Comment 7 Chris Dumez 2020-06-25 17:10:31 PDT
Reverted r263545 for reason:

Patch will need refining

Committed r263547: <https://trac.webkit.org/changeset/263547>
Comment 8 Chris Dumez 2020-06-25 18:03:05 PDT
Created attachment 402835 [details]
Patch
Comment 9 Alex Christensen 2020-06-26 09:42:32 PDT
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 10 Chris Dumez 2020-06-26 09:49:09 PDT
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 11 Darin Adler 2020-06-26 09:49:12 PDT
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*.
Comment 12 Alex Christensen 2020-06-26 09:54:16 PDT
Created attachment 402877 [details]
Patch
Comment 13 Alex Christensen 2020-06-26 09:56:12 PDT
Comment on attachment 402877 [details]
Patch

ObjC doesn't really like calling another initializer from init, but this does the same thing.
Comment 14 Chris Dumez 2020-06-26 09:56:57 PDT
(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.
Comment 15 Chris Dumez 2020-06-26 10:11:51 PDT
(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.
Comment 16 EWS 2020-06-26 10:37:30 PDT
Committed r263570: <https://trac.webkit.org/changeset/263570>

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