Bug 187055 – Split memory store logic out of WebResourceLoadStatisticsStore to clarify threading model

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

[Chris Dumez]

Split memory store logic out of WebResourceLoadStatisticsStore to clarify threading model, similarly to what was already done for the persistent store.

[Chris Dumez]

Created attachment 343632 [details]
Patch

[EWS Watchlist]

Comment on attachment 343632 [details]
Patch

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

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html

[EWS Watchlist]

Created attachment 343646 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[John Wilander]

Comment on attachment 343632 [details]
Patch

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

This is an important improvement to this code. Thanks for doing this work, Chris.

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:363
> +std::optional<bool> ResourceLoadStatisticsMemoryStore::hasStorageAccess(const String& subFramePrimaryDomain)

This is confusing now that it's broken out and only handles the part about domains *not being able* to request access (blocked) and domains *not needing* to request access (neither blocked nor partitioned). I suggest the name mayRequestStorageAccess() and a return StorageAccessStatus::CannotRequestAccess for blocked, StorageAccessStatus::HasAccess for neither blocked nor partitioned, and StorageAccessStatus::RequiresUserPrompt for the remaining partitioned case (currently std::nullopt).

The last of those, returning StorageAccessStatus::RequiresUserPrompt, might still be confusing since this is a call to hasStorageAccess(), not requestStorageAccess(), and the user may have already been prompted. Two other options would be different enum for hasStorageAccess() or an additional value in the existing enum such as StorageAccessStatus::MayHaveStorageAccess. Adding StorageAccessStatus::MayHaveStorageAccess makes me worried that we'll mess things up in requestStorageAccess() where that value doesn't really make sense.

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:1069
> +void ResourceLoadStatisticsMemoryStore::processStatistics(const WTF::Function<void(const ResourceLoadStatistics&)>& processFunction) const

Let's take the opportunity to get rid of WTF:: here.

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.h:65
> +    void processStatistics(const WTF::Function<void(const WebCore::ResourceLoadStatistics&)>&) const;

Let's take the opportunity to get rid of WTF:: here.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:210
> +                completionHandler(*hasStorageAccess);

This code is confusing (see naming comment above) and wrong as far as I can tell.

First, it looks like it's asking the memory store whether the subFramePrimaryDomain has been granted storage access but that cannot happen. Instead the memory store just knows if a) the domain is unable to request access or b) doesn't need to request access. I suggest mayRequestStorageAccess().

Second, out of the three current return values, both 'true' and 'false' should return early and skip the call to the network process. Only the std::nullopt case, meaning the domain *may have requested* storage access, should trigger a call to the network process. The network process will of course respond with the right value, so no test will fail. It's just unnecessary.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:159
>      Ref<WTF::WorkQueue> m_statisticsQueue;

Let's take the opportunity to get rid of WTF:: here.

[Chris Dumez]

Comment on attachment 343632 [details]
Patch

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

As discussed offline, I will refactor hasStorageAccess() to be more consistent with the rest and not use an std::optional.

>> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:210
>> +                completionHandler(*hasStorageAccess);
> 
> This code is confusing (see naming comment above) and wrong as far as I can tell.
> 
> First, it looks like it's asking the memory store whether the subFramePrimaryDomain has been granted storage access but that cannot happen. Instead the memory store just knows if a) the domain is unable to request access or b) doesn't need to request access. I suggest mayRequestStorageAccess().
> 
> Second, out of the three current return values, both 'true' and 'false' should return early and skip the call to the network process. Only the std::nullopt case, meaning the domain *may have requested* storage access, should trigger a call to the network process. The network process will of course respond with the right value, so no test will fail. It's just unnecessary.

hasStorageAccess is actually a std::optional

[Chris Dumez]

Created attachment 343672 [details]
Patch

[Chris Dumez]

Took John's feedback into consideration. All layout tests / API tests pass in a debug build.

[Sam Weinig]

Comment on attachment 343672 [details]
Patch

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

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:58
> +    if (!isFirstItem)

Is isFirstItem ever not equal to builder.isEmpty()?

[Chris Dumez]

(In reply to Sam Weinig from comment #8)
> Comment on attachment 343672 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343672&action=review
> 
> > Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:58
> > +    if (!isFirstItem)
> 
> Is isFirstItem ever not equal to builder.isEmpty()?

I did not write this code and am merely moving it but I believe you're right. Will fix.

[Chris Dumez]

Created attachment 343687 [details]
Patch

[Chris Dumez]

Created attachment 343717 [details]
Patch

[Brent Fulgham]

Comment on attachment 343717 [details]
Patch

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

Looks good, and the tests all pass. Nice work on this huge refactoring job! r=me.

> Source/WebKit/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=187055

Radar please!

> Source/WebKit/ChangeLog:14
> +        objects merely proxies calls from WebKit to those persistent / memory stores and takes care of hoping back and

"... takes care of HOPPING back and"

> Source/WebKit/ChangeLog:18
> +        I fixed those in this patch now that the model is clearer.

Awesome!

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:468
> +    RunLoop::main().dispatch([subFramePrimaryDomain = subFramePrimaryDomain.isolatedCopy(), topFramePrimaryDomain = topFramePrimaryDomain.isolatedCopy(), frameID, pageID, store = makeRef(m_store), callback = WTFMove(callback)]() mutable {

I guess this is one of the cases where we were calling on the wrong thread in the original code!

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:129
> +        m_memoryStore->setPersistentStorage(*m_persistentStorage);

I wonder if the m_persistentStore constructor could establish this relationship?

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:183
> +// On background queue due to IPC.

Thank you for this comment! I thought this was a mistake at first! :-)

[Radar WebKit Bug Importer]

<rdar://problem/41584026>

[Chris Dumez]

Comment on attachment 343717 [details]
Patch

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

>> Source/WebKit/ChangeLog:4
>> +        https://bugs.webkit.org/show_bug.cgi?id=187055
> 
> Radar please!

Will add.

>> Source/WebKit/ChangeLog:14
>> +        objects merely proxies calls from WebKit to those persistent / memory stores and takes care of hoping back and
> 
> "... takes care of HOPPING back and"

:D

>> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:468
>> +    RunLoop::main().dispatch([subFramePrimaryDomain = subFramePrimaryDomain.isolatedCopy(), topFramePrimaryDomain = topFramePrimaryDomain.isolatedCopy(), frameID, pageID, store = makeRef(m_store), callback = WTFMove(callback)]() mutable {
> 
> I guess this is one of the cases where we were calling on the wrong thread in the original code!

Yes, the handlers were often called on the non-main thread.

>> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:129
>> +        m_memoryStore->setPersistentStorage(*m_persistentStorage);
> 
> I wonder if the m_persistentStore constructor could establish this relationship?

Oh, that'd be better indeed... Will do.

>> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:183
>> +// On background queue due to IPC.
> 
> Thank you for this comment! I thought this was a mistake at first! :-)

This is temporary, my next patch will make it so that ALL members are called on the main thread.

[Chris Dumez]

Created attachment 343811 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 343811 [details]
Patch

Clearing flags on attachment: 343811

Committed r233310: <https://trac.webkit.org/changeset/233310>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

LayoutTests are exiting early on the bots due to one of the assertions added with this change:

ASSERTION FAILED: RunLoop::isMain()
/Volumes/Data/slave/highsierra-debug/build/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp(146) : void WebKit::WebResourceLoadStatisticsStore::flushAndDestroyPersistentStore()
1   0x10f445069 WTFCrash
2   0x114d04cc8 WebKit::WebResourceLoadStatisticsStore::flushAndDestroyPersistentStore()
3   0x114d04a03 WebKit::WebResourceLoadStatisticsStore::~WebResourceLoadStatisticsStore()
4   0x114d04e45 WebKit::WebResourceLoadStatisticsStore::~WebResourceLoadStatisticsStore()
5   0x114d04e69 WebKit::WebResourceLoadStatisticsStore::~WebResourceLoadStatisticsStore()

https://build.webkit.org/results/Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r233315%20(3879)/results.html

[Chris Dumez]

(In reply to Ryan Haddad from comment #18)
> LayoutTests are exiting early on the bots due to one of the assertions added
> with this change:
> 
> ASSERTION FAILED: RunLoop::isMain()
> /Volumes/Data/slave/highsierra-debug/build/Source/WebKit/UIProcess/
> WebResourceLoadStatisticsStore.cpp(146) : void
> WebKit::WebResourceLoadStatisticsStore::flushAndDestroyPersistentStore()
> 1   0x10f445069 WTFCrash
> 2   0x114d04cc8
> WebKit::WebResourceLoadStatisticsStore::flushAndDestroyPersistentStore()
> 3   0x114d04a03
> WebKit::WebResourceLoadStatisticsStore::~WebResourceLoadStatisticsStore()
> 4   0x114d04e45
> WebKit::WebResourceLoadStatisticsStore::~WebResourceLoadStatisticsStore()
> 5   0x114d04e69
> WebKit::WebResourceLoadStatisticsStore::~WebResourceLoadStatisticsStore()
> 
> https://build.webkit.org/results/
> Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r233315%20(3879)/results.html

Looking now.

[Chris Dumez]

(In reply to Chris Dumez from comment #19)
> (In reply to Ryan Haddad from comment #18)
> > LayoutTests are exiting early on the bots due to one of the assertions added
> > with this change:
> > 
> > ASSERTION FAILED: RunLoop::isMain()
> > /Volumes/Data/slave/highsierra-debug/build/Source/WebKit/UIProcess/
> > WebResourceLoadStatisticsStore.cpp(146) : void
> > WebKit::WebResourceLoadStatisticsStore::flushAndDestroyPersistentStore()
> > 1   0x10f445069 WTFCrash
> > 2   0x114d04cc8
> > WebKit::WebResourceLoadStatisticsStore::flushAndDestroyPersistentStore()
> > 3   0x114d04a03
> > WebKit::WebResourceLoadStatisticsStore::~WebResourceLoadStatisticsStore()
> > 4   0x114d04e45
> > WebKit::WebResourceLoadStatisticsStore::~WebResourceLoadStatisticsStore()
> > 5   0x114d04e69
> > WebKit::WebResourceLoadStatisticsStore::~WebResourceLoadStatisticsStore()
> > 
> > https://build.webkit.org/results/
> > Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r233315%20(3879)/results.html
> 
> Looking now.

This shows the WebResourceLoadStatisticsStore getting destroyed on the background queue, which I am fixing via https://bugs.webkit.org/show_bug.cgi?id=187143.

This is not a regression from this patch but the crash is caused by a new assertion I added. I will temporarily disable the assertion and re-land it as part of Bug 187143.

[Chris Dumez]

Comment on attachment 343811 [details]
Patch

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

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:146
> +    ASSERT(RunLoop::isMain());

This assertion.

[Chris Dumez]

Committed r233319: <https://trac.webkit.org/changeset/233319>

[Ross Kirsling]

This broke WinCairo, as was indicated by EWS:

> c:\webkit-ews\webkit\source\webkit\uiprocess\WebResourceLoadStatisticsStore.h(67): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int
> c:\webkit-ews\webkit\source\webkit\uiprocess\WebResourceLoadStatisticsStore.h(67): error C2143: syntax error: missing ',' before '&'
> c:\webkit-ews\webkit\source\webkit\uiprocess\WebResourceLoadStatisticsStore.h(67): error C3735: template or generic redefined
> c:\webkit-ews\webkit\source\webkit\uiprocess\WebResourceLoadStatisticsStore.h(67): error C3861: 'callback': identifier not found

[Chris Dumez]

(In reply to Ross Kirsling from comment #23)
> This broke WinCairo, as was indicated by EWS:
> 
> > c:\webkit-ews\webkit\source\webkit\uiprocess\WebResourceLoadStatisticsStore.h(67): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int
> > c:\webkit-ews\webkit\source\webkit\uiprocess\WebResourceLoadStatisticsStore.h(67): error C2143: syntax error: missing ',' before '&'
> > c:\webkit-ews\webkit\source\webkit\uiprocess\WebResourceLoadStatisticsStore.h(67): error C3735: template or generic redefined
> > c:\webkit-ews\webkit\source\webkit\uiprocess\WebResourceLoadStatisticsStore.h(67): error C3861: 'callback': identifier not found

I guess it is due to stripping the WTF::, which Per said would be OK on Windows iirc.