Bug 193556 - Implement message handlers for NetworkProcess-based ResourceLoadStatistics
Summary: Implement message handlers for NetworkProcess-based ResourceLoadStatistics
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on: 194054
Blocks: 193199 193303 193659 193660 193723
  Show dependency treegraph
 
Reported: 2019-01-17 16:16 PST by Brent Fulgham
Modified: 2019-01-30 15:38 PST (History)
12 users (show)

See Also:


Attachments
Patch (288.51 KB, patch)
2019-01-17 17:04 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (288.96 KB, patch)
2019-01-17 19:03 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (3.00 MB, application/zip)
2019-01-17 20:25 PST, Build Bot
no flags Details
Patch (289.59 KB, patch)
2019-01-17 20:38 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (292.29 KB, patch)
2019-01-17 22:09 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (9.03 MB, application/zip)
2019-01-18 00:14 PST, Build Bot
no flags Details
Patch (286.56 KB, patch)
2019-01-18 08:38 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (9.07 MB, application/zip)
2019-01-18 10:54 PST, Build Bot
no flags Details
Patch (286.60 KB, patch)
2019-01-18 11:08 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (289.86 KB, patch)
2019-01-18 12:17 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (320.51 KB, patch)
2019-01-18 14:54 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (320.44 KB, patch)
2019-01-18 15:27 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (9.38 MB, application/zip)
2019-01-18 17:56 PST, Build Bot
no flags Details
Patch for landing (323.85 KB, patch)
2019-01-21 13:51 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2019-01-17 16:16:22 PST
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.
Comment 1 Brent Fulgham 2019-01-17 16:17:18 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).
Comment 2 Radar WebKit Bug Importer 2019-01-17 16:18:00 PST
<rdar://problem/47368501>
Comment 3 Brent Fulgham 2019-01-17 17:04:59 PST
Created attachment 359429 [details]
Patch
Comment 4 Brent Fulgham 2019-01-17 19:03:38 PST
Created attachment 359440 [details]
Patch
Comment 5 Build Bot 2019-01-17 20:25:13 PST
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
Comment 6 Build Bot 2019-01-17 20:25:15 PST
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
Comment 7 Brent Fulgham 2019-01-17 20:38:44 PST
Created attachment 359446 [details]
Patch
Comment 8 Brent Fulgham 2019-01-17 22:09:08 PST
Created attachment 359450 [details]
Patch
Comment 9 Build Bot 2019-01-18 00:14:05 PST
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
Comment 10 Build Bot 2019-01-18 00:14:07 PST
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
Comment 11 Brent Fulgham 2019-01-18 08:38:25 PST
Created attachment 359493 [details]
Patch
Comment 12 Build Bot 2019-01-18 10:54:18 PST
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
Comment 13 Build Bot 2019-01-18 10:54:20 PST
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
Comment 14 Brent Fulgham 2019-01-18 11:08:22 PST
Created attachment 359512 [details]
Patch
Comment 15 Brent Fulgham 2019-01-18 12:17:35 PST
Created attachment 359521 [details]
Patch
Comment 16 Brent Fulgham 2019-01-18 14:54:15 PST
Created attachment 359543 [details]
Patch
Comment 17 Brent Fulgham 2019-01-18 15:27:52 PST
Created attachment 359548 [details]
Patch
Comment 18 Alex Christensen 2019-01-18 16:28:02 PST
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 19 Build Bot 2019-01-18 17:56:32 PST
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
Comment 20 Build Bot 2019-01-18 17:56:34 PST
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
Comment 21 Brent Fulgham 2019-01-21 13:51:05 PST
Created attachment 359708 [details]
Patch for landing
Comment 22 WebKit Commit Bot 2019-01-21 14:28:11 PST
Comment on attachment 359708 [details]
Patch for landing

Clearing flags on attachment: 359708

Committed r240243: <https://trac.webkit.org/changeset/240243>
Comment 23 WebKit Commit Bot 2019-01-21 14:28:13 PST
All reviewed patches have been landed.  Closing bug.
Comment 24 Brent Fulgham 2019-01-21 16:12:15 PST
This change introduced an API test failure. See Bug 193660.
Comment 25 Michael Catanzaro 2019-01-22 12:09:01 PST
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.
Comment 26 Michael Catanzaro 2019-01-22 12:16:33 PST
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...
Comment 27 Brent Fulgham 2019-01-22 12:16:33 PST
(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.
Comment 28 Brent Fulgham 2019-01-22 12:20:42 PST
(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?
Comment 29 Brent Fulgham 2019-01-22 12:21:39 PST
Maybe in WKBundleResourceLoadStatisticsNotifyObserver(WKBundleRef)?
Comment 30 Michael Catanzaro 2019-01-22 12:41:53 PST
I'll try, thanks.
Comment 31 Michael Catanzaro 2019-01-22 13:36:09 PST
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 32 Chris Dumez 2019-01-30 15:29:27 PST
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.
Comment 33 Chris Dumez 2019-01-30 15:38:26 PST
(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.