Summary: | Implement message handlers for NetworkProcess-based ResourceLoadStatistics | ||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brent Fulgham <bfulgham> | ||||||||||||||||||||||||||||||
Component: | WebKit Misc. | Assignee: | Brent Fulgham <bfulgham> | ||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | achristensen, bfulgham, cdumez, commit-queue, dbates, ews-watchlist, japhet, mcatanzaro, pgriffis, rniwa, webkit-bug-importer, wilander | ||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||
Bug Depends on: | 194054 | ||||||||||||||||||||||||||||||||
Bug Blocks: | 193199, 193303, 193659, 193660, 193723 | ||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Brent Fulgham
2019-01-17 16:16:22 PST
Note: This patch also adds some new messaging between the WebContent process and the NetworkProcess to track user interaction (mainly as generated by the test system). Created attachment 359429 [details]
Patch
Created attachment 359440 [details]
Patch
Comment on attachment 359440 [details] Patch Attachment 359440 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/10791334 New failing tests: http/tests/storageAccess/has-storage-access-from-prevalent-domain-with-user-interaction.html http/tests/storageAccess/request-storage-access-cross-origin-sandboxed-iframe-without-allow-token.html http/tests/storageAccess/request-and-grant-access-cross-origin-sandboxed-nested-iframe.html http/tests/storageAccess/request-storage-access-cross-origin-sandboxed-iframe-without-user-gesture.html http/tests/storageAccess/request-and-grant-access-then-navigate-cross-site-should-not-have-access.html Created attachment 359443 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Created attachment 359446 [details]
Patch
Created attachment 359450 [details]
Patch
Comment on attachment 359450 [details] Patch Attachment 359450 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/10792810 New failing tests: http/tests/resourceLoadStatistics/prevalent-resource-with-user-interaction-timeout.html Created attachment 359455 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Created attachment 359493 [details]
Patch
Comment on attachment 359493 [details] Patch Attachment 359493 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/10797893 New failing tests: http/tests/resourceLoadStatistics/prevalent-resource-with-user-interaction-timeout.html Created attachment 359509 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Created attachment 359512 [details]
Patch
Created attachment 359521 [details]
Patch
Created attachment 359543 [details]
Patch
Created attachment 359548 [details]
Patch
Comment on attachment 359548 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359548&action=review r=me. Good luck landing this. > Source/WebCore/loader/ResourceLoadObserver.cpp:72 > +void ResourceLoadObserver::setLogUserInteractionNotificationCallback(WTF::Function<void(PAL::SessionID, const String&)>&& callback) WTF:: is unnecessary. > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:107 > + postTaskReply([completionHandler = WTFMove(completionHandler)] () mutable { > + completionHandler(); > + }); postTaskReply(WTFMove(completionHandler)); > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:291 > - m_memoryStore->hasStorageAccess(subFramePrimaryDomain, topFramePrimaryDomain, frameID, pageID, [completionHandler = WTFMove(completionHandler)](bool hasStorageAccess) mutable { > + m_memoryStore->hasStorageAccess(subFramePrimaryDomain, topFramePrimaryDomain, frameID.value(), pageID, [completionHandler = WTFMove(completionHandler)](bool hasStorageAccess) mutable { This .value() shouldn't be necessary. Just make the callee take an Optional. > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1195 > + if (m_websiteDataStore) > + WebProcessProxy::topPrivatelyControlledDomainsWithWebsiteData(dataTypes, shouldNotifyPage, WTFMove(completionHandler)); > + > + if (m_networkSession) > + m_networkSession->topPrivatelyControlledDomainsWithWebsiteData(dataTypes, shouldNotifyPage, WTFMove(completionHandler)); Maybe we should early return if there's a websiteDataStore, and call the completion handler if there are neither websiteDataStore nor networkSession just to make sure it's even more impossible to misuse the completion handler. There are a few such things in this file. > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h:65 > +enum class ShouldGrandfather { This no longer has the scope of WebResourceLoadStatisticsStore, so it could use a better name. And it can use a bool for storage and be on one line. > Source/WebKit/NetworkProcess/NetworkProcess.cpp:-653 > - ASSERT(RunLoop::isMain()); Why are we removing this assertion? > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1370 > + // FIXME: No API to delete credentials by origin Should this have the same bugzilla number as the other FIXMEs in this patch? > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:250 > + while (!m_allStorageAccessEntriesCallbackMap.isEmpty()) > + m_allStorageAccessEntriesCallbackMap.take(m_allStorageAccessEntriesCallbackMap.begin()->key)({ }); Using sendWithAsyncReply would make all these autogenerated and reduce the boilerplate code in this patch significantly. Comment on attachment 359548 [details] Patch Attachment 359548 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/10803474 New failing tests: http/tests/resourceLoadStatistics/prevalent-resource-with-user-interaction-timeout.html Created attachment 359565 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Created attachment 359708 [details]
Patch for landing
Comment on attachment 359708 [details] Patch for landing Clearing flags on attachment: 359708 Committed r240243: <https://trac.webkit.org/changeset/240243> All reviewed patches have been landed. Closing bug. This change introduced an API test failure. See Bug 193660. This is causing our bots to exit early from crashes and timeouts. I'll try to help investigate soon but I'm going to do an emergency rollout for now, sorry. Good way to prevent or delay emergency rollouts: land a follow-up fix, so the automated rollout tool doesn't work. :D Problem is: ASSERTION FAILED: m_notificationCallback STDERR: ../../Source/WebCore/loader/ResourceLoadObserver.cpp(376) : void WebCore::ResourceLoadObserver::notifyObserver() Thread 1 (Thread 0x7fb142867ac0 (LWP 38216)): #0 0x00007fb15119f6ac in WTFCrash () at /home/slave/webkitgtk/gtk-linux-64-debug/build/Source/WTF/wtf/Assertions.cpp:255 #1 0x00007fb15ddcdde7 in (anonymous namespace)::ResourceLoadObserver::notifyObserver (this=0x7fb1648d5aa0 <_ZZN7WebCore20ResourceLoadObserver6sharedEvE20resourceLoadObserver>) at ../../Source/WebCore/loader/ResourceLoadObserver.cpp:376 #2 0x00007fb15c2114ed in WKBundleResourceLoadStatisticsNotifyObserver () at ../../Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundle.cpp:326 #3 0x00007fb0ebc2430d in WTR::InjectedBundle::statisticsNotifyObserver (this=0x55fdf8b93110) at /home/slave/webkitgtk/gtk-linux-64-debug/build/Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:1005 #4 0x00007fb0ebc3fcb5 in WTR::TestRunner::statisticsNotifyObserver (this=0x7fb141fe8540) at /home/slave/webkitgtk/gtk-linux-64-debug/build/Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:1896 #5 0x00007fb0ebc5fa3c in WTR::JSTestRunner::statisticsNotifyObserver (context=0x7fff4ab200d0, thisObject=0x7fb0eaff4030, argumentCount=0, arguments=0x7fff4ab1ff80, exception=0x7fff4ab1ff68) at /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/DerivedSources/InjectedBundle/JSTestRunner.cpp:2473 #6 0x00007fb14ff06876 in JSC::APICallbackFunction::call<JSC::JSCallbackFunction> (exec=0x7fff4ab200d0) at ../../Source/JavaScriptCore/API/APICallbackFunction.h:63 #7 0x00007fb0fa8a302d in ?? () #8 0x00007fff4ab20140 in ?? () #9 0x00007fb150c10f43 in llint_op_call () from /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18 #10 0x0000000000000000 in ?? () Investigating... (In reply to Michael Catanzaro from comment #25) > This is causing our bots to exit early from crashes and timeouts. I'll try > to help investigate soon but I'm going to do an emergency rollout for now, > sorry. Please do not do so. I've already landed other changes that depend on this. Let's figure out what's going on in the GTK code. (In reply to Michael Catanzaro from comment #26) > Problem is: > > ASSERTION FAILED: m_notificationCallback > STDERR: ../../Source/WebCore/loader/ResourceLoadObserver.cpp(376) : void > WebCore::ResourceLoadObserver::notifyObserver() This indicates the test code is trying to notify an observer with no notification callback registered. My guess is that some of the code I protected with ENABLE(RESOURCE_LOAD_STATISTICS) was setting this value for GTK (even though it wasn't being used). Perhaps the observer notifications should be protected with ENABLE(RESOURCE_LOAD_STATISTICS) to avoid the assertion? Maybe in WKBundleResourceLoadStatisticsNotifyObserver(WKBundleRef)? I'll try, thanks. I think that maybe fixed the crashes, but not the timeouts. I found bug #193320 and after some discussion there, decided to just skip the tests. This was an emergency because when the bots are so broken that they exit early, we lose a potentially large range of test results and then it becomes hard to identify which commit in that range is responsible for a regression. Anyway, thanks for the help. Committed r240293: <https://trac.webkit.org/changeset/240293> Comment on attachment 359708 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=359708&action=review > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1216 > + struct CallbackAggregator final : public RefCounted<CallbackAggregator> { This should be ThreadSafeRefCounted, see crashes on the bots. > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1298 > + postStorageTask(CrossThreadTask([this, sessionID, callbackAggregator = callbackAggregator.copyRef(), path = WTFMove(path), topPrivatelyControlledDomains]() mutable { For e.g., it is passed to a background thread there. (In reply to Chris Dumez from comment #32) > Comment on attachment 359708 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=359708&action=review > > > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1216 > > + struct CallbackAggregator final : public RefCounted<CallbackAggregator> { > > This should be ThreadSafeRefCounted, see crashes on the bots. > > > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1298 > > + postStorageTask(CrossThreadTask([this, sessionID, callbackAggregator = callbackAggregator.copyRef(), path = WTFMove(path), topPrivatelyControlledDomains]() mutable { > > For e.g., it is passed to a background thread there. Uploaded a patch at https://bugs.webkit.org/show_bug.cgi?id=194054. |