Bug 213625 – [iOS] Network process is crashing when launching TJMaxx app due to invalid NetworkProcess::DestroySession IPC message

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

Network process is crashing when launching TJMaxx app due to invalid NetworkProcess::DestroySession IPC message.

[Chris Dumez]

<rdar://problem/64737890>

[Chris Dumez]

Created attachment 402822 [details]
Patch

[Geoffrey Garen]

Comment on attachment 402822 [details]
Patch

r=me

[EWS]

Committed r263545: <https://trac.webkit.org/changeset/263545>

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

[Darin Adler]

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.

[Chris Dumez]

Reopening based on Darin's comments.

[Chris Dumez]

Reverted r263545 for reason:

Patch will need refining

Committed r263547: <https://trac.webkit.org/changeset/263547>

[Chris Dumez]

Created attachment 402835 [details]
Patch

[Alex Christensen]

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?

[Chris Dumez]

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()];

[Darin Adler]

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*.

[Alex Christensen]

Created attachment 402877 [details]
Patch

[Alex Christensen]

Comment on attachment 402877 [details]
Patch

ObjC doesn't really like calling another initializer from init, but this does the same thing.

[Chris Dumez]

(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.

[Chris Dumez]

(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.

[EWS]

Committed r263570: <https://trac.webkit.org/changeset/263570>

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