Bug 198797 – Ensure ITP state is relayed to Network Process on restart

• Component: WebKit2
• Status: RESOLVED FIXED

[Brent Fulgham]

Now that the ITP state is maintained in the Network Process, we have to make sure that we update the process with current ITP state if the Network Process crashes and is relaunched.

This wasn't a problem in earlier releases because we tracked all ITP state in the UIProcess.

[Brent Fulgham]

<rdar://problem/51646944>

[Brent Fulgham]

Created attachment 372004 [details]
Patch

[EWS Watchlist]

Comment on attachment 372004 [details]
Patch

Attachment 372004 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12461360

New failing tests:
imported/blink/fast/canvas/canvas-clip-stack-persistence.html
fast/shadow-dom/svg-thref-href-change-in-shadow-tree.html

[EWS Watchlist]

Created attachment 372023 [details]
Archive of layout-test-results from ews211 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews211  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Sam Weinig]

Comment on attachment 372004 [details]
Patch

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

> Source/WebKit/UIProcess/WebProcessPool.cpp:637
> +            m_networkProcess->send(Messages::NetworkProcess::SetResourceLoadStatisticsEnabled(true), 0);

To avoid the unnecessary extra IPC, can this be added to NetworkProcessCreationParameters? What is the purpose of predicating this on !withWebsiteDataStore && !m_websiteDataStore?

> Source/WebKit/UIProcess/WebProcessPool.cpp:682
> +    // Make sure the newly-spawned NetworkProcess is in the right ITP state.
> +    if (m_itpIsEnabled && !m_websiteDataStore)
> +        newNetworkProcess.send(Messages::NetworkProcess::SetResourceLoadStatisticsEnabled(true), 0);

This seems like duplicate work given you also call this in ensureNetworkProcess().

> Source/WebKit/UIProcess/WebProcessPool.h:730
> +    bool m_itpIsEnabled { false };

It seems like this might make more sense as a property of the WebSiteDataStore, rather than the network process itself.

[Brent Fulgham]

Created attachment 372071 [details]
Patch

[Brent Fulgham]

Comment on attachment 372004 [details]
Patch

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

>> Source/WebKit/UIProcess/WebProcessPool.cpp:637
>> +            m_networkProcess->send(Messages::NetworkProcess::SetResourceLoadStatisticsEnabled(true), 0);
> 
> To avoid the unnecessary extra IPC, can this be added to NetworkProcessCreationParameters? What is the purpose of predicating this on !withWebsiteDataStore && !m_websiteDataStore?

Yes -- fixed!

>> Source/WebKit/UIProcess/WebProcessPool.cpp:682
>> +        newNetworkProcess.send(Messages::NetworkProcess::SetResourceLoadStatisticsEnabled(true), 0);
> 
> This seems like duplicate work given you also call this in ensureNetworkProcess().

You are right -- I'll remove.

>> Source/WebKit/UIProcess/WebProcessPool.h:730
>> +    bool m_itpIsEnabled { false };
> 
> It seems like this might make more sense as a property of the WebSiteDataStore, rather than the network process itself.

Unfortunately, they are several WKWebView use cases that initialize the NetworkProcess before a WebsiteDataStore has been established (Mobile Safari, for example).

[Sam Weinig]

(In reply to Brent Fulgham from comment #7)
> >> Source/WebKit/UIProcess/WebProcessPool.h:730
> >> +    bool m_itpIsEnabled { false };
> > 
> > It seems like this might make more sense as a property of the WebSiteDataStore, rather than the network process itself.
> 
> Unfortunately, they are several WKWebView use cases that initialize the
> NetworkProcess before a WebsiteDataStore has been established (Mobile
> Safari, for example).

What can a network process do without a web site data store? Specifically, what does MobileSafari do with one? Can you test this configuration?

[Brent Fulgham]

(In reply to Sam Weinig from comment #8)
> (In reply to Brent Fulgham from comment #7)
> > >> Source/WebKit/UIProcess/WebProcessPool.h:730
> > >> +    bool m_itpIsEnabled { false };
> > > 
> > > It seems like this might make more sense as a property of the WebSiteDataStore, rather than the network process itself.
> > 
> > Unfortunately, they are several WKWebView use cases that initialize the
> > NetworkProcess before a WebsiteDataStore has been established (Mobile
> > Safari, for example).
> 
> What can a network process do without a web site data store? Specifically,
> what does MobileSafari do with one? Can you test this configuration?

For one thing, it can launch.

MobileSafari triggers the NetworkProcess to launch before adding the website data store.

The tests cases I wrote already act this way when running on iOS.

Whether that was really the intended design or not is a question for Geoff; all I can do at this point is work with the existing behavior.

Perhaps the WebsiteDataStore should not have been an optional part of the NetworkProcess launch state.

[Sam Weinig]

(In reply to Brent Fulgham from comment #9)
> (In reply to Sam Weinig from comment #8)
> > (In reply to Brent Fulgham from comment #7)
> > > >> Source/WebKit/UIProcess/WebProcessPool.h:730
> > > >> +    bool m_itpIsEnabled { false };
> > > > 
> > > > It seems like this might make more sense as a property of the WebSiteDataStore, rather than the network process itself.
> > > 
> > > Unfortunately, they are several WKWebView use cases that initialize the
> > > NetworkProcess before a WebsiteDataStore has been established (Mobile
> > > Safari, for example).
> > 
> > What can a network process do without a web site data store? Specifically,
> > what does MobileSafari do with one? Can you test this configuration?
> 
> For one thing, it can launch.
> 
> MobileSafari triggers the NetworkProcess to launch before adding the website
> data store.
> 
> The tests cases I wrote already act this way when running on iOS.
> 

I don't follow. How does your test case exercise a network process without a data store? 

configuration.get().websiteDataStore = dataStore;

[Brent Fulgham]

(In reply to Sam Weinig from comment #10)
> (In reply to Brent Fulgham from comment #9)
> > (In reply to Sam Weinig from comment #8)
> > > (In reply to Brent Fulgham from comment #7)
> > > > >> Source/WebKit/UIProcess/WebProcessPool.h:730
> > > > >> +    bool m_itpIsEnabled { false };
> > > > > 
> > > > > It seems like this might make more sense as a property of the WebSiteDataStore, rather than the network process itself.
> > > > 
> > > > Unfortunately, they are several WKWebView use cases that initialize the
> > > > NetworkProcess before a WebsiteDataStore has been established (Mobile
> > > > Safari, for example).
> > > 
> > > What can a network process do without a web site data store? Specifically,
> > > what does MobileSafari do with one? Can you test this configuration?
> > 
> > For one thing, it can launch.
> > 
> > MobileSafari triggers the NetworkProcess to launch before adding the website
> > data store.
> > 
> > The tests cases I wrote already act this way when running on iOS.
> > 
> 
> I don't follow. How does your test case exercise a network process without a
> data store? 
> 
> configuration.get().websiteDataStore = dataStore;

When you call this:

    [configuration setProcessPool: sharedProcessPool];

... it brings up a Network Process without a WebsiteDataStore. It doesn't matter that we assign one in the next bit of WebKit test code, the process is already running and has been initialized with the default network launch parameters (which does not have ITP turned on).

[youenn fablet]

Comment on attachment 372071 [details]
Patch

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

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp:249
> +        logTestingEvent("Storage Synced"_s);

Why do we need that?

> Source/WebKit/UIProcess/WebProcessPool.cpp:612
> +        enableResourceLoadStatistics = m_itpIsEnabled;

I guess this could be set at declaration time.
bool enableResourceLoadStatistics = m_itpIsEnabled.
For consistency, should we rename m_itpIsEnabled to m_defaultEnableResourceLoadStatistics?

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1876
>          processPool->sendToNetworkingProcess(Messages::NetworkProcess::SetResourceLoadStatisticsEnabled(false));

It seems strange to do that.
Instead, we should have NetworkProcess::SetResourceLoadStatisticsEnabled take a sessionID as well as the boolean.

If we want to keep a special value in WebProcessPool, it should be the one of the default session ID.
Maybe, this default value should be stored in NetworkProcessProxy.
On network process crash, WebProcessPool would keep it in m_itpIsEnabled.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:229
> +    // Tell ITP to process data so it will sync the new 'bad guy' to persistent storage:

I am not clear about this comment.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:322
> +    WKProcessPool *sharedProcessPool = [WKProcessPool _sharedProcessPool];

auto

[Sam Weinig]

(In reply to Brent Fulgham from comment #11)
> (In reply to Sam Weinig from comment #10)
> > (In reply to Brent Fulgham from comment #9)
> > > (In reply to Sam Weinig from comment #8)
> > > > (In reply to Brent Fulgham from comment #7)
> > > > > >> Source/WebKit/UIProcess/WebProcessPool.h:730
> > > > > >> +    bool m_itpIsEnabled { false };
> > > > > > 
> > > > > > It seems like this might make more sense as a property of the WebSiteDataStore, rather than the network process itself.
> > > > > 
> > > > > Unfortunately, they are several WKWebView use cases that initialize the
> > > > > NetworkProcess before a WebsiteDataStore has been established (Mobile
> > > > > Safari, for example).
> > > > 
> > > > What can a network process do without a web site data store? Specifically,
> > > > what does MobileSafari do with one? Can you test this configuration?
> > > 
> > > For one thing, it can launch.
> > > 
> > > MobileSafari triggers the NetworkProcess to launch before adding the website
> > > data store.
> > > 
> > > The tests cases I wrote already act this way when running on iOS.
> > > 
> > 
> > I don't follow. How does your test case exercise a network process without a
> > data store? 
> > 
> > configuration.get().websiteDataStore = dataStore;
> 
> When you call this:
> 
>     [configuration setProcessPool: sharedProcessPool];
> 
> ... it brings up a Network Process without a WebsiteDataStore. It doesn't
> matter that we assign one in the next bit of WebKit test code, the process
> is already running and has been initialized with the default network launch
> parameters (which does not have ITP turned on).

But why does it matter if ITP is turned on or off at that time? What behavior change would one see if you did not set it?

[youenn fablet]

> But why does it matter if ITP is turned on or off at that time? What
> behavior change would one see if you did not set it?

My understanding is that we enable ITP for all sessions, even though we should apply it to only the website data store session given the current API tied to WebsiteDataStore.

Then, on crash, we loose that boolean information since not all datastore might actually having their IPTEnabled boolean set to true, so we try to keep that state in UIProcess.

Given the current API, it makes sense to enable ITP per sessionID.
Maybe there is a specific handling needed for the default session.

[Brent Fulgham]

Comment on attachment 372071 [details]
Patch

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

>> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp:249
>> +        logTestingEvent("Storage Synced"_s);
> 
> Why do we need that?

We use these testing events to notify the UIProcess when interesting things happen. This is a low-frequency operation (once every 30 seconds or so, if the user found new information for ITP) so it doesn't get hit very often. We already do this for Grandfathering and other important milestones.

>> Source/WebKit/UIProcess/WebProcessPool.cpp:612
>> +        enableResourceLoadStatistics = m_itpIsEnabled;
> 
> I guess this could be set at declaration time.
> bool enableResourceLoadStatistics = m_itpIsEnabled.
> For consistency, should we rename m_itpIsEnabled to m_defaultEnableResourceLoadStatistics?

We are trying to move away from using 'resourceLoadStatistics" everywhere. Might as well use the proper naming from the start on this change.

>> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1876
>>          processPool->sendToNetworkingProcess(Messages::NetworkProcess::SetResourceLoadStatisticsEnabled(false));
> 
> It seems strange to do that.
> Instead, we should have NetworkProcess::SetResourceLoadStatisticsEnabled take a sessionID as well as the boolean.
> 
> If we want to keep a special value in WebProcessPool, it should be the one of the default session ID.
> Maybe, this default value should be stored in NetworkProcessProxy.
> On network process crash, WebProcessPool would keep it in m_itpIsEnabled.

Maybe, but I don't want to change how any of this works for the purpose of creating a test case. Could I file a separate bug to make "SetResourceLoadStatisticsEnabled" work on a sessionID?

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:229
>> +    // Tell ITP to process data so it will sync the new 'bad guy' to persistent storage:
> 
> I am not clear about this comment.

I want to make sure we sync the ITP state to disk. One of the times we do this is when an entry is added or removed from the ITP data. The "processStatisticsAndDataRecords" action syncs state to persistent storage at the end.

I should change the comment:

"// Trigger ITP to process its data to force a sync to persistent storage"

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:322
>> +    WKProcessPool *sharedProcessPool = [WKProcessPool _sharedProcessPool];
> 
> auto

Will fix!

[Brent Fulgham]

Created attachment 372086 [details]
Patch

[youenn fablet]

Comment on attachment 372086 [details]
Patch

OK, let's roll this in.
Let's also handle itp enabling per session as a follow-up.

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

> Source/WebKit/UIProcess/WebProcessPool.cpp:585
> +    bool enableResourceLoadStatistics = m_itpIsEnabled;

Since it only applies to the default session, I would rename it to something like m_itpIsEnabledForDefaultSession or maybe just m_iptEnabledForDefaultSessions

[Darin Adler]

Comment on attachment 372086 [details]
Patch

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

> Source/WebKit/UIProcess/WebProcessPool.h:730
> +    bool m_itpIsEnabled { false };

I always try to use tricks to make sure I don’t have to look at those lowercase acronyms like "itp". In this case I will take my cue from Youenn’s suggestion above, and the idiom in the surrounding code and suggest this name:

    m_shouldEnableITPForDefaultSessions

[Brent Fulgham]

Comment on attachment 372086 [details]
Patch

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

>> Source/WebKit/UIProcess/WebProcessPool.cpp:585
>> +    bool enableResourceLoadStatistics = m_itpIsEnabled;
> 
> Since it only applies to the default session, I would rename it to something like m_itpIsEnabledForDefaultSession or maybe just m_iptEnabledForDefaultSessions

I'll use "m_iptEnabledForDefaultSession"

>> Source/WebKit/UIProcess/WebProcessPool.h:730
>> +    bool m_itpIsEnabled { false };
> 
> I always try to use tricks to make sure I don’t have to look at those lowercase acronyms like "itp". In this case I will take my cue from Youenn’s suggestion above, and the idiom in the surrounding code and suggest this name:
> 
>     m_shouldEnableITPForDefaultSessions

Okay -- I'll use that!

[Brent Fulgham]

Committed r246500: <https://trac.webkit.org/changeset/246500>