Bug 238570 – Do not create network process in ~WebsiteDataStore destructor

• Component: WebKit2
• Status: RESOLVED FIXED

[Yury Semikhatsky]

Do not create network process in ~WebsiteDataStore destructor

[Yury Semikhatsky]

Created attachment 456166 [details]
Patch

[Darin Adler]

Comment on attachment 456166 [details]
Patch

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

Great fix! We really want this.

> Source/WebKit/ChangeLog:13
> +        No new tests.

Can we make a test? Normally the project requires a test for an bug fix.

[Yury Semikhatsky]

> Can we make a test? Normally the project requires a test for an bug fix.

The bug manifested itself as WPENetworkProcess running after corresponding WebsiteDataStore has been closed so I don't think there is a way to test it in a layout test. I tried to come up with a unit test but there is no API for getting currently running network processes. Do you have an advice on how that could be tested via WebKit API?

[Darin Adler]

In past in such cases we have typically added calls as needed to test such things. Like add something to count network processes.

[Darin Adler]

Chris, do you have suggestions on how to make a test? You’ve fixed many bugs like this one. Or maybe you know someone else we should ask.

[Chris Dumez]

Comment on attachment 456166 [details]
Patch

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

>> Source/WebKit/ChangeLog:13
>> +        No new tests.
> 
> Can we make a test? Normally the project requires a test for an bug fix.

I would look at the NetworkProcess.LaunchOnlyWhenNecessary for example. Not sure if this is practical in this particular case. I am in the middle of something but can look into it a bit later if needed.

[Chris Dumez]

(In reply to Chris Dumez from comment #6)
> Comment on attachment 456166 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=456166&action=review
> 
> >> Source/WebKit/ChangeLog:13
> >> +        No new tests.
> > 
> > Can we make a test? Normally the project requires a test for an bug fix.
> 
> I would look at the NetworkProcess.LaunchOnlyWhenNecessary for example. Not
> sure if this is practical in this particular case. I am in the middle of
> something but can look into it a bit later if needed.

I think something like this might do the trick (but please verify):
```
TEST(NetworkProcess, DoNotLaunchOnDataStoreDestruction)
{
    EXPECT_FALSE([WKWebsiteDataStore _defaultDataStoreExists]);
    @autoreleasepool {
        auto storeConfiguration = adoptNS([[_WKWebsiteDataStoreConfiguration alloc] initNonPersistentConfiguration]);
        auto websiteDataStore = adoptNS([[WKWebsiteDataStore alloc] _initWithConfiguration: storeConfiguration.get()]);
    }

    TestWebKitAPI::Util::spinRunLoop(10);
    EXPECT_FALSE([WKWebsiteDataStore _defaultDataStoreExists]);
}
```

[Sihui Liu]

Comment on attachment 456166 [details]
Patch

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

>>>> Source/WebKit/ChangeLog:13
>>>> +        No new tests.
>>> 
>>> Can we make a test? Normally the project requires a test for an bug fix.
>> 
>> I would look at the NetworkProcess.LaunchOnlyWhenNecessary for example. Not sure if this is practical in this particular case. I am in the middle of something but can look into it a bit later if needed.
> 
> I think something like this might do the trick (but please verify):
> ```
> TEST(NetworkProcess, DoNotLaunchOnDataStoreDestruction)
> {
>     EXPECT_FALSE([WKWebsiteDataStore _defaultDataStoreExists]);
>     @autoreleasepool {
>         auto storeConfiguration = adoptNS([[_WKWebsiteDataStoreConfiguration alloc] initNonPersistentConfiguration]);
>         auto websiteDataStore = adoptNS([[WKWebsiteDataStore alloc] _initWithConfiguration: storeConfiguration.get()]);
>     }
> 
>     TestWebKitAPI::Util::spinRunLoop(10);
>     EXPECT_FALSE([WKWebsiteDataStore _defaultDataStoreExists]);
> }
> ```

How about creating a custom persistent WKWebsiteDataStore, destroying it, and checking if default network process exists?
@autoreleasepool {
    auto websiteDataStoreConfiguration = adoptNS([[_WKWebsiteDataStoreConfiguration alloc] init]);
    auto websiteDataStore = adoptNS([[WKWebsiteDataStore alloc] _initWithConfiguration:websiteDataStoreConfiguration.get()]).get();
}
TestWebKitAPI::Util::spinRunLoop(10); // Is this needed?
EXPECT_FALSE([WKWebsiteDataStore _defaultNetworkProcessExists]);

(please check if the test fails without fix)

[Chris Dumez]

(In reply to Chris Dumez from comment #7)
> (In reply to Chris Dumez from comment #6)
> > Comment on attachment 456166 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=456166&action=review
> > 
> > >> Source/WebKit/ChangeLog:13
> > >> +        No new tests.
> > > 
> > > Can we make a test? Normally the project requires a test for an bug fix.
> > 
> > I would look at the NetworkProcess.LaunchOnlyWhenNecessary for example. Not
> > sure if this is practical in this particular case. I am in the middle of
> > something but can look into it a bit later if needed.
> 
> I think something like this might do the trick (but please verify):
> ```
> TEST(NetworkProcess, DoNotLaunchOnDataStoreDestruction)
> {
>     EXPECT_FALSE([WKWebsiteDataStore _defaultDataStoreExists]);
>     @autoreleasepool {
>         auto storeConfiguration = adoptNS([[_WKWebsiteDataStoreConfiguration
> alloc] initNonPersistentConfiguration]);
>         auto websiteDataStore = adoptNS([[WKWebsiteDataStore alloc]
> _initWithConfiguration: storeConfiguration.get()]);
>     }
> 
>     TestWebKitAPI::Util::spinRunLoop(10);
>     EXPECT_FALSE([WKWebsiteDataStore _defaultDataStoreExists]);
> }
> ```

Sihui pointed out I have a typo. I mean to check _defaultNetworkProcessExists above, not _defaultDataStoreExists.

[Chris Dumez]

Comment on attachment 456166 [details]
Patch

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

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:154
> +        m_networkProcess->removeSession(*this);

So the API test I wrote (and likely Sihui's) seems to pass without this fix. I believe the reason is that we launch the network process and then destroy it right away (unlike what the changelog claims).

We looked at it with Sihui and `networkProcess()` would indeed allocate a NetworkProcessProxy. However, the call to `removeSession()` would null out the default NetworkProcessProxy, which would cause the newly launched (likely still launching) process to exist right away.

Yury, I think it is good to fix either way but are you still the network process is indeed hanging around unnecessarily?

[Chris Dumez]

(In reply to Chris Dumez from comment #10)
> Comment on attachment 456166 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=456166&action=review
> 
> > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:154
> > +        m_networkProcess->removeSession(*this);
> 
> So the API test I wrote (and likely Sihui's) seems to pass without this fix.
> I believe the reason is that we launch the network process and then destroy
> it right away (unlike what the changelog claims).
> 
> We looked at it with Sihui and `networkProcess()` would indeed allocate a
> NetworkProcessProxy. However, the call to `removeSession()` would null out
> the default NetworkProcessProxy, which would cause the newly launched
> (likely still launching) process to exist right away.

typo again: exist -> exit.

[Chris Dumez]

Never mind, I was able to write an API test to cover this change:
```
TEST(NetworkProcess, DoNotLaunchOnDataStoreDestruction)
{
    auto storeConfiguration1 = adoptNS([[_WKWebsiteDataStoreConfiguration alloc] initNonPersistentConfiguration]);
    auto websiteDataStore1 = adoptNS([[WKWebsiteDataStore alloc] _initWithConfiguration: storeConfiguration1.get()]);

    EXPECT_FALSE([WKWebsiteDataStore _defaultNetworkProcessExists]);
    @autoreleasepool {
        auto storeConfiguration2 = adoptNS([[_WKWebsiteDataStoreConfiguration alloc] initNonPersistentConfiguration]);
        auto websiteDataStore2 = adoptNS([[WKWebsiteDataStore alloc] _initWithConfiguration: storeConfiguration2.get()]);
    }

    TestWebKitAPI::Util::spinRunLoop(10);
    EXPECT_FALSE([WKWebsiteDataStore _defaultNetworkProcessExists]);
}
```

Yury, do you mind incorporating it in your patch?

[Yury Semikhatsky]

Comment on attachment 456166 [details]
Patch

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

> Yury, do you mind incorporating it in your patch?

Neat! I was looking for a hook that would give access to those in the linux ports but didn't find any, didn't consider the default network process accessor. In any case, I've added the test to the patch.

> Yury, I think it is good to fix either way but are you still the network process is indeed hanging around unnecessarily?

Originally I discovered the problem on linux where a new network process is launched for each ephemeral data store. The problem in GTK/WPE is that NetworkProxyProxy launches a new process
and gets destroyed immediately (as the only reference from WebsiteDataStore is removed), but the process itself keeps running until the browser is closed. Admittedly the process is properly killed
if at least one page was created with that WebsiteDataStore instance. I'd expect a network process to be terminated if its NetworkProcessProxy has been destroyed. Perhaps it's worth adding some logic
that would terminate the network process once its NetworkProcessProxy is destroyed.

[Yury Semikhatsky]

Created attachment 456330 [details]
Patch

[Yury Semikhatsky]

Darin, Chris, I've incorporated the test in the patch. Can you have another look?

[Chris Dumez]

Comment on attachment 456330 [details]
Patch

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

r=me

> Source/WebKit/ChangeLog:13
> +        No new tests.

This is now a lie :)

[Yury Semikhatsky]

Created attachment 456710 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 456330 [details]
Patch

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

>> Source/WebKit/ChangeLog:13
>> +        No new tests.
> 
> This is now a lie :)

Oops, fixed.

[EWS]

Committed r292403 (249266@main): <https://commits.webkit.org/249266@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/91304176>