Bug 193556 – Implement message handlers for NetworkProcess-based ResourceLoadStatistics

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

This patch adds implementations for message handling to support driving WebKitTestRunner tests using a NetworkProcess-based WebResourceLoadStatisticsStore object.

This patch does not activate these new code paths or change test results.

[Brent Fulgham]

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).

[Radar WebKit Bug Importer]

<rdar://problem/47368501>

[Brent Fulgham]

Created attachment 359429 [details]
Patch

[Brent Fulgham]

Created attachment 359440 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Brent Fulgham]

Created attachment 359446 [details]
Patch

[Brent Fulgham]

Created attachment 359450 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Brent Fulgham]

Created attachment 359493 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Brent Fulgham]

Created attachment 359512 [details]
Patch

[Brent Fulgham]

Created attachment 359521 [details]
Patch

[Brent Fulgham]

Created attachment 359543 [details]
Patch

[Brent Fulgham]

Created attachment 359548 [details]
Patch

[Alex Christensen]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Brent Fulgham]

Created attachment 359708 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 359708 [details]
Patch for landing

Clearing flags on attachment: 359708

Committed r240243: <https://trac.webkit.org/changeset/240243>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brent Fulgham]

This change introduced an API test failure. See Bug 193660.

[Michael Catanzaro]

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.

[Michael Catanzaro]

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...

[Brent Fulgham]

(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.

[Brent Fulgham]

(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?

[Brent Fulgham]

Maybe in WKBundleResourceLoadStatisticsNotifyObserver(WKBundleRef)?

[Michael Catanzaro]

I'll try, thanks.

[Michael Catanzaro]

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>

[Chris Dumez]

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.

[Chris Dumez]

(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.