Bug 216041 – Move NetworkProcessProxy ownership from WebProcessPool to WebsiteDataStore

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Move NetworkProcessProxy ownership from WebProcessPool to WebsiteDataStore

[Alex Christensen]

Created attachment 407690 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Alex Christensen]

I'm separating this approach's attempts so the EWS bubbles load faster and to keep the two approaches separate from each other so I can see my progress.
This first patch is just to see the EWS results.  It is not done yet.

[Aakash Jain]

Seems like this breaks api tests on macOS.

https://ews-build.webkit.org/#/builders/3/builds/31500
https://ews-build.webkit.org/#/builders/3/builds/31493
https://ews-build.webkit.org/#/builders/3/builds/31485

[Radar WebKit Bug Importer]

<rdar://problem/68514363>

[Alex Christensen]

Created attachment 408733 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Alex Christensen]

Not done yet.  Uploading WIP to get a checkpoint on which tests still need work.

[Alex Christensen]

Created attachment 408853 [details]
Patch

[Alex Christensen]

Created attachment 409211 [details]
Patch

[Alex Christensen]

Created attachment 409214 [details]
Patch

[Alex Christensen]

Created attachment 409300 [details]
Patch

[Alex Christensen]

Created attachment 409323 [details]
Patch

[Alex Christensen]

Created attachment 409327 [details]
Patch

[Alex Christensen]

Created attachment 409341 [details]
Patch

[Alex Christensen]

Created attachment 409342 [details]
Patch

[Alex Christensen]

Created attachment 409345 [details]
Patch

[Alex Christensen]

Created attachment 409349 [details]
Patch

[Alex Christensen]

Created attachment 409350 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 409350 [details]
Patch

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

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:195
> +        if (m_sessionID.isEphemeral() || m_sessionID == PAL::SessionID::defaultSessionID())
> +            m_networkProcess = NetworkProcessProxy::defaultNetworkProcess();

This is actually making the network process a singleton for the default session and the reason why tests are failing in GTK. I'll investigate more to see why they fail when reusing the same process.

[Carlos Garcia Campos]

Comment on attachment 409350 [details]
Patch

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

>> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:195
>> +            m_networkProcess = NetworkProcessProxy::defaultNetworkProcess();
> 
> This is actually making the network process a singleton for the default session and the reason why tests are failing in GTK. I'll investigate more to see why they fail when reusing the same process.

Every test case uses a new web context (process pool) to ensure a clean env. What happens now is that the first test case creates a website data store for the default session id, when the next test starts a new websitedata store is created for the default session id again, and the previous one is destroyed, since destroy happens after the create, we end up removing the newly created one, since we use the session id. I don't think we can share the network process for GLib API, it's ok to move the ownership to the data manager, but the process should be created and destroyed with the data store. I think we could share a process for ephemeral sessions, though.

[Alex Christensen]

Created attachment 409367 [details]
Patch

[Alex Christensen]

Created attachment 409378 [details]
Patch

[Chris Dumez]

Comment on attachment 409378 [details]
Patch

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

> Source/WebKit/ChangeLog:3
> +        Move NetworkProcessProxy ownership from WebProcessPool to WebsiteDataStore

Could the changelog explain why we're doing this? Why this is better than the current state? It is non-obvious to me at least.

[Alex Christensen]

Created attachment 409451 [details]
Patch

[Alex Christensen]

Created attachment 409452 [details]
Patch

[Alex Christensen]

Created attachment 409484 [details]
Patch

[Alex Christensen]

*** Bug 203547 has been marked as a duplicate of this bug. ***

[Carlos Garcia Campos]

Comment on attachment 409484 [details]
Patch

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

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:183
> -    if (m_sessionID != PAL::SessionID::defaultSessionID()) {
> -        auto result = nonDefaultDataStores().add(m_sessionID, this);
> -        ASSERT_UNUSED(result, result.isNewEntry);
> +    auto result = allDataStores().add(m_sessionID, this);
> +    ASSERT_UNUSED(result, result.isNewEntry);

This is a problem for GLib API too. In our API persistent data stores are always created with the default session ID, and are expected to be asociated with a web context (a process pool). So, you could have two default data stores each associated to a different context. Both are the default one, but the default for its network process. With this patch, creating a second data store will assert in debug. Note that we don't expose session IDs in our API on purpose. A possible solution would be to remove the concept of default session, and simply use a different session ID for each data store. After this patch the default session is not used that much, for soup we would just need to figure out how to implement the DNS resolver.

[Carlos Garcia Campos]

Created attachment 409568 [details]
Rebased patch

This is the patch rebased on top of current trunk and patch attached to bug #216927.

[Carlos Garcia Campos]

Created attachment 409569 [details]
GLib API changes

This patch contains the changes I added on top of the rebased patch just to make it easier to see them

[Carlos Garcia Campos]

Comment on attachment 409569 [details]
GLib API changes

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

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:163
> +#if PLATFORM(GTK) || PLATFORM(WPE)
> +    RELEASE_ASSERT_NOT_REACHED();
> +#endif

I added this to catch code using the default data store with the GLib API. I think the inspector uses it and probably some other places, so a few tests might still fail. I'll check those.

[Carlos Garcia Campos]

Ah, there's also the initial accept cookie policy, but I think we no longer need that, I'll check that too, but I think bug #216927 is the only one blocking this from the GLib API now.

[Alex Christensen]

Wonderful!  Thanks, Carlos!

[Alex Christensen]

Created attachment 409598 [details]
Patch

[Alex Christensen]

Created attachment 409600 [details]
Patch

[Alex Christensen]

Created attachment 409603 [details]
Patch

[Alex Christensen]

Created attachment 409604 [details]
Patch

[Alex Christensen]

Created attachment 409607 [details]
Patch

[Alex Christensen]

Created attachment 409631 [details]
Patch

[Brady Eidson]

Comment on attachment 409631 [details]
Patch

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

Too big to critically analyze every single change.

Overall, seems fine.

Yay tests.

One specific review comment.

> Source/WebKit/NetworkProcess/NetworkProcessCreationParameters.h:82
> -    bool shouldEnableITPDatabase { false };
> +    bool shouldEnableITPDatabase { true };

Why this change?

[Alex Christensen]

Comment on attachment 409631 [details]
Patch

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

>> Source/WebKit/NetworkProcess/NetworkProcessCreationParameters.h:82
>> +    bool shouldEnableITPDatabase { true };
> 
> Why this change?

This changed under me during my many rebases.  I don't think this did any change because it is always set from elsewhere, but I'll not make this change.
I'll also double check everything before landing to find any more there may be.

[Alex Christensen]

http://trac.webkit.org/r267608

[Alex Christensen]

This caused some API tests to fail on the bots.  Why they're so different than EWS, I may never know.

{
    "Failed": [
        {
            "output": "\n/Volumes/Data/slave/catalina-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:1543\nValue of: isRegistered\n  Actual: false\nExpected: true\n\n", 
            "name": "TestWebKitAPI.ResourceLoadStatistics.MigrateDataFromMissingTopFrameUniqueRedirectSameSiteStrictTableSchema"
        }, 
        {
            "output": "\n/Volumes/Data/slave/catalina-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:1582\nExpected equality of these values:\n  static_cast<int>([thirdPartyData count])\n    Which is: 0\n  1\n\n\n/Volumes/Data/slave/catalina-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:1586\nExpected equality of these values:\n  thirdParty.thirdPartyDomain\n    Which is: \"\"\n  @\"webkit.org\"\n    Which is: \"webkit.org\"\n\n\n/Volumes/Data/slave/catalina-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:1591\nExpected equality of these values:\n  firstParty.firstPartyDomain\n    Which is: \"\"\n  @\"apple.com\"\n    Which is: \"apple.com\"\n\n", 
            "name": "TestWebKitAPI.ResourceLoadStatistics.CanAccessDataSummaryWithNoProcessPool"
        }, 
        {
            "output": "\n/Volumes/Data/slave/catalina-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:1453\nValue of: isRegistered\n  Actual: false\nExpected: true\n\n", 
            "name": "TestWebKitAPI.ResourceLoadStatistics.MigrateDataFromIncorrectCreateTableSchema"
        }
    ], 
    "Timedout": [
        {
            "output": "", 
            "name": "TestWebKitAPI.ResourceLoadStatistics.ShouldNotGrandfatherOnStartup"
        }, 
        {
            "output": "", 
            "name": "TestWebKitAPI.ResourceLoadStatistics.GrandfatherCallback"
        }, 
        {
            "output": "", 
            "name": "TestWebKitAPI.ResourceLoadStatistics.ChildProcessesNotLaunched"
        }, 
        {
            "output": "", 
            "name": "TestWebKitAPI.ResourceLoadStatistics.GrandfatherCallbackDatabase"
        }, 
        {
            "output": "", 
            "name": "TestWebKitAPI.Fullscreen.WKViewDelegate"
        }
    ], 
    "Skipped": [], 
    "Crashed": []
}

[Alex Christensen]

Reverted r267608 for reason:

Caused API test failures

Committed r267618: <https://trac.webkit.org/changeset/267618>

[Carlos Garcia Campos]

Please, give me some time to check the layout test failures too before landing this again. See bug #217006

[Carlos Garcia Campos]

Comment on attachment 409631 [details]
Patch

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

> Tools/WebKitTestRunner/TestController.cpp:-3173
> -    WKContextSetPrimaryWebsiteDataStore(context, defaultWebsiteDataStore());

This is the problem of layout tests for GTK and WPE. The WTR default data store is not set as web view data store either now, so when the web page is created the default data store s created and used for the page. I'll submit a rebased patch with the fix.

[Carlos Garcia Campos]

Created attachment 409981 [details]
Rebased patch

Patch rebased and updated to fix GTK and WPE layout tests

[Alex Christensen]

Created attachment 410019 [details]
Patch

[Alex Christensen]

Created attachment 410023 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r267763

[Alex Christensen]

This was rdar://problem/69175060

[Alex Christensen]

Comment on attachment 410023 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:-350
> -    addIndexedDatabaseSession(sessionID, parameters.defaultDataStoreParameters.indexedDatabaseDirectory, parameters.defaultDataStoreParameters.indexedDatabaseDirectoryExtensionHandle);

This is still needed.  See https://bugs.webkit.org/show_bug.cgi?id=220666