Do not create network process in ~WebsiteDataStore destructor
Created attachment 456166 [details] Patch
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.
> 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?
In past in such cases we have typically added calls as needed to test such things. Like add something to count network processes.
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.
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.
(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]); } ```
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)
(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.
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?
(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.
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?
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.
Created attachment 456330 [details] Patch
Darin, Chris, I've incorporated the test in the patch. Can you have another look?
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 :)
Created attachment 456710 [details] Patch
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.
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].
<rdar://problem/91304176>