Split memory store logic out of WebResourceLoadStatisticsStore to clarify threading model, similarly to what was already done for the persistent store.
Created attachment 343632 [details] Patch
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
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
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.
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
Created attachment 343672 [details] Patch
Took John's feedback into consideration. All layout tests / API tests pass in a debug build.
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()?
(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.
Created attachment 343687 [details] Patch
Created attachment 343717 [details] Patch
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! :-)
<rdar://problem/41584026>
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.
Created attachment 343811 [details] Patch
Comment on attachment 343811 [details] Patch Clearing flags on attachment: 343811 Committed r233310: <https://trac.webkit.org/changeset/233310>
All reviewed patches have been landed. Closing bug.
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
(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.
(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.
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.
Committed r233319: <https://trac.webkit.org/changeset/233319>
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
(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.