PSON: http/tests/resourceLoadStatistics/classify-as-prevalent-based-on-top-frame-redirect-collusion.html ASSERTS with process swap enabled

• Component: WebKit2
• Status: RESOLVED FIXED

[Brady Eidson]

PSON: http/tests/resourceLoadStatistics/classify-as-prevalent-based-on-top-frame-redirect-collusion.html ASSERTS with process swap enabled

[Brady Eidson]

ASSERTION FAILED: !statistics.isEmpty()
/Volumes/Data/git/OpenSource/Source/WebKit/WebProcess/WebProcess.cpp(203) : auto WebKit::WebProcess::WebProcess()::(anonymous class)::operator()(Vector<WebCore::ResourceLoadStatistics> &&) const
1   0x63f992ad9 WTFCrash
2   0x1064acbf5 WebKit::WebProcess::WebProcess()::$_1::operator()(WTF::Vector<WebCore::ResourceLoadStatistics, 0ul, WTF::CrashOnOverflow, 16ul>&&) const
3   0x1064acaf4 WTF::Function<void (WTF::Vector<WebCore::ResourceLoadStatistics, 0ul, WTF::CrashOnOverflow, 16ul>&&)>::CallableWrapper<WebKit::WebProcess::WebProcess()::$_1>::call(WTF::Vector<WebCore::ResourceLoadStatistics, 0ul, WTF::CrashOnOverflow, 16ul>&&)
4   0x63263205e WTF::Function<void (WTF::Vector<WebCore::ResourceLoadStatistics, 0ul, WTF::CrashOnOverflow, 16ul>&&)>::operator()(WTF::Vector<WebCore::ResourceLoadStatistics, 0ul, WTF::CrashOnOverflow, 16ul>&&) const
5   0x63262fd6b WebCore::ResourceLoadObserver::notifyObserver()
6   0x10675f049 WKBundleResourceLoadStatisticsNotifyObserver
7   0x645f189e8 WTR::InjectedBundle::statisticsNotifyObserver()
8   0x645f4f399 WTR::TestRunner::statisticsNotifyObserver()
9   0x645f44347 WTR::JSTestRunner::statisticsNotifyObserver(OpaqueJSContext const*, OpaqueJSValue*, OpaqueJSValue*, unsigned long, OpaqueJSValue const* const*, OpaqueJSValue const**)
10  0x63fb05757 long long JSC::APICallbackFunction::call<JSC::JSCallbackFunction>(JSC::ExecState*)
11  0xdc233ba02d
12  0x63fa85146 llint_entry
13  0x63fa7cb32 vmEntryToJavaScript
14  0x6408d90ba JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
15  0x6408d8661 JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*, JSC::JSObject*)
16  0x640b8fb87 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
17  0x640b8fd10 JSC::profiledEvaluate(JSC::ExecState*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
18  0x63195b46b WebCore::JSMainThreadExecState::profiledEvaluate(JSC::ExecState*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
19  0x63195b1d6 WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&, WebCore::ExceptionDetails*)
20  0x63195b54d WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&, WebCore::ExceptionDetails*)
21  0x631f3ea27 WebCore::ScriptElement::executeClassicScript(WebCore::ScriptSourceCode const&)
22  0x631f3cf09 WebCore::ScriptElement::prepareScript(WTF::TextPosition const&, WebCore::ScriptElement::LegacyTypeSupport)
23  0x6323735b0 WebCore::HTMLScriptRunner::runScript(WebCore::ScriptElement&, WTF::TextPosition const&)
24  0x63237341f WebCore::HTMLScriptRunner::execute(WTF::Ref<WebCore::ScriptElement, WTF::DumbPtrTraits<WebCore::ScriptElement> >&&, WTF::TextPosition const&)
25  0x632356135 WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder()
26  0x6323565f3 WebCore::HTMLDocumentParser::pumpTokenizerLoop(WebCore::HTMLDocumentParser::SynchronousMode, bool, WebCore::PumpSession&)
27  0x632355338 WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode)
28  0x632354eab WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode)
29  0x632358049 WebCore::HTMLDocumentParser::resumeParsingAfterScriptExecution()
30  0x63235844e WebCore::HTMLDocumentParser::notifyFinished(WebCore::PendingScript&)
31  0x6323584ac non-virtual thunk to WebCore::HTMLDocumentParser::notifyFinished(WebCore::PendingScript&)
#CRASHED - com.apple.WebKit.WebContent.Development (pid 50455)

[Brady Eidson]

ResourceLoadStatistics is fundamentally broken in PSON

We either need to move a statistics map from the old process to the new one, or just host statistics in the UIProcess.

[Brady Eidson]

(In reply to Brady Eidson from comment #2)
> ResourceLoadStatistics is fundamentally broken in PSON
> 
> We either need to move a statistics map from the old process to the new one,
> or just host statistics in the UIProcess.

Chris tells me that RLS are hosted in the UI process, but driven by data pushed out from WebProcesses. This test failure a case of a new WebProcess not having what is expected of it!

[Chris Dumez]

(In reply to Brady Eidson from comment #3)
> (In reply to Brady Eidson from comment #2)
> > ResourceLoadStatistics is fundamentally broken in PSON
> > 
> > We either need to move a statistics map from the old process to the new one,
> > or just host statistics in the UIProcess.
> 
> Chris tells me that RLS are hosted in the UI process, but driven by data
> pushed out from WebProcesses. This test failure a case of a new WebProcess
> not having what is expected of it!

The test is relying on a test runner API to force the WebProcess to send its RLS to the UIProcess:
6   0x10675f049 WKBundleResourceLoadStatisticsNotifyObserver
7   0x645f189e8 WTR::InjectedBundle::statisticsNotifyObserver()
8   0x645f4f399 WTR::TestRunner::statisticsNotifyObserver()

Because of process swap, it is asking to new WebProcess instead of the old one, we hit an assertion because we're not supposed to notify the client of new statistics with no statistics.

[Brady Eidson]

(In reply to Chris Dumez from comment #4)
> (In reply to Brady Eidson from comment #3)
> > (In reply to Brady Eidson from comment #2)
> > > ResourceLoadStatistics is fundamentally broken in PSON
> > > 
> > > We either need to move a statistics map from the old process to the new one,
> > > or just host statistics in the UIProcess.
> > 
> > Chris tells me that RLS are hosted in the UI process, but driven by data
> > pushed out from WebProcesses. This test failure a case of a new WebProcess
> > not having what is expected of it!
> 
> The test is relying on a test runner API to force the WebProcess to send its
> RLS to the UIProcess:
> 6   0x10675f049 WKBundleResourceLoadStatisticsNotifyObserver
> 7   0x645f189e8 WTR::InjectedBundle::statisticsNotifyObserver()
> 8   0x645f4f399 WTR::TestRunner::statisticsNotifyObserver()
> 
> Because of process swap, it is asking to new WebProcess instead of the old
> one, we hit an assertion because we're not supposed to notify the client of
> new statistics with no statistics.

Yup, and when I simply make the ASSERT a no-op instead the test passes.

/me shrugs

[Chris Dumez]

(In reply to Brady Eidson from comment #5)
> (In reply to Chris Dumez from comment #4)
> > (In reply to Brady Eidson from comment #3)
> > > (In reply to Brady Eidson from comment #2)
> > > > ResourceLoadStatistics is fundamentally broken in PSON
> > > > 
> > > > We either need to move a statistics map from the old process to the new one,
> > > > or just host statistics in the UIProcess.
> > > 
> > > Chris tells me that RLS are hosted in the UI process, but driven by data
> > > pushed out from WebProcesses. This test failure a case of a new WebProcess
> > > not having what is expected of it!
> > 
> > The test is relying on a test runner API to force the WebProcess to send its
> > RLS to the UIProcess:
> > 6   0x10675f049 WKBundleResourceLoadStatisticsNotifyObserver
> > 7   0x645f189e8 WTR::InjectedBundle::statisticsNotifyObserver()
> > 8   0x645f4f399 WTR::TestRunner::statisticsNotifyObserver()
> > 
> > Because of process swap, it is asking to new WebProcess instead of the old
> > one, we hit an assertion because we're not supposed to notify the client of
> > new statistics with no statistics.
> 
> Yup, and when I simply make the ASSERT a no-op instead the test passes.
> 
> /me shrugs

The test is marked as flaky, which is why it may have passed from you locally. However, I do think we would break the test here. We need the previous WebProcess do send its RLS to the UIProcess.

We may want to update the testRunner API to ask all WebProcesses to send their data to the UIProcess, instead of merely the current one.

[Chris Dumez]

(In reply to Chris Dumez from comment #6)
> (In reply to Brady Eidson from comment #5)
> > (In reply to Chris Dumez from comment #4)
> > > (In reply to Brady Eidson from comment #3)
> > > > (In reply to Brady Eidson from comment #2)
> > > > > ResourceLoadStatistics is fundamentally broken in PSON
> > > > > 
> > > > > We either need to move a statistics map from the old process to the new one,
> > > > > or just host statistics in the UIProcess.
> > > > 
> > > > Chris tells me that RLS are hosted in the UI process, but driven by data
> > > > pushed out from WebProcesses. This test failure a case of a new WebProcess
> > > > not having what is expected of it!
> > > 
> > > The test is relying on a test runner API to force the WebProcess to send its
> > > RLS to the UIProcess:
> > > 6   0x10675f049 WKBundleResourceLoadStatisticsNotifyObserver
> > > 7   0x645f189e8 WTR::InjectedBundle::statisticsNotifyObserver()
> > > 8   0x645f4f399 WTR::TestRunner::statisticsNotifyObserver()
> > > 
> > > Because of process swap, it is asking to new WebProcess instead of the old
> > > one, we hit an assertion because we're not supposed to notify the client of
> > > new statistics with no statistics.
> > 
> > Yup, and when I simply make the ASSERT a no-op instead the test passes.
> > 
> > /me shrugs
> 
> The test is marked as flaky, which is why it may have passed from you
> locally. However, I do think we would break the test here. We need the
> previous WebProcess do send its RLS to the UIProcess.
> 
> We may want to update the testRunner API to ask all WebProcesses to send
> their data to the UIProcess, instead of merely the current one.

Personally, I have this failure if I enable JSON and unskip the test:
--- /Volumes/Data/WebKit/OpenSource/WebKitBuild/Release/layout-test-results/http/tests/resourceLoadStatistics/classify-as-prevalent-based-on-top-frame-redirect-collusion-expected.txt
+++ /Volumes/Data/WebKit/OpenSource/WebKitBuild/Release/layout-test-results/http/tests/resourceLoadStatistics/classify-as-prevalent-based-on-top-frame-redirect-collusion-actual.txt
@@ -7,7 +7,6 @@
 PASS Colluding host 2 got set as prevalent resource.
 PASS Colluding host 3 got set as prevalent resource.
 PASS Colluding host 4 got set as prevalent resource.
-PASS Colluding localhost got set as prevalent resource after actual navigational redirect.
 PASS successfullyParsed is true

[Chris Dumez]

Updating testRunner.statisticsNotifyObserver() to sync all WebProcess instead of merely  the current one is not sufficient. When running with PSON, none of the 2 WebProcesses have any statistics to sync..

[Chris Dumez]

OK, plan B: Have ITP log frame navigations on the UIProcess instead of the WebProcess.

[Chris Dumez]

Created attachment 342596 [details]
WIP Patch

[Chris Dumez]

Created attachment 342605 [details]
Patch

[John Wilander]

Comment on attachment 342605 [details]
Patch

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

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:341
> +    cancelPendingStatisticsProcessingRequest();

We should augment the comment above to explain that we cancel any pending statistics processing because we are about to call processStatisticsAndDataRecords() directly here.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:564
> +    m_statisticsQueue->dispatchAfter(minimumStatisticsProcessingInterval, [this, protectedThis = makeRef(*this), statisticsProcessingRequestIdentifier = *m_pendingStatisticsProcessingRequestIdentifier] {

We are already on the worker thread here. Does that matter when dispatching again?

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:582
> +void WebResourceLoadStatisticsStore::logFrameNavigation(const WebFrameProxy& frame, const URL& pageURL, const WebCore::ResourceRequest& request, const WebCore::URL& redirectURL)

Does this function handle both top frame and sub frame navigations? The logic indicates that it is the case. I'm asking since top frame navigations are what the PSON changes deal with.

> LayoutTests/platform/wk2/TestExpectations:708
> +http/tests/resourceLoadStatistics/classify-as-prevalent-based-on-top-frame-redirect-collusion.html [ Pass ]

Nice!

[Chris Dumez]

Comment on attachment 342605 [details]
Patch

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

>> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:341
>> +    cancelPendingStatisticsProcessingRequest();
> 
> We should augment the comment above to explain that we cancel any pending statistics processing because we are about to call processStatisticsAndDataRecords() directly here.

Ok, can do this before landing.

>> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:564
>> +    m_statisticsQueue->dispatchAfter(minimumStatisticsProcessingInterval, [this, protectedThis = makeRef(*this), statisticsProcessingRequestIdentifier = *m_pendingStatisticsProcessingRequestIdentifier] {
> 
> We are already on the worker thread here. Does that matter when dispatching again?

Yes we are, I still would think we would not want to call updateCookiePartitioning / processStatisticsAndDataRecords too often, right? I applied the same rate limiting as what was used for the WebProcess to send its statistics.

>> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:582
>> +void WebResourceLoadStatisticsStore::logFrameNavigation(const WebFrameProxy& frame, const URL& pageURL, const WebCore::ResourceRequest& request, const WebCore::URL& redirectURL)
> 
> Does this function handle both top frame and sub frame navigations? The logic indicates that it is the case. I'm asking since top frame navigations are what the PSON changes deal with.

Yes, for all frames. This is called from decidePolicyForNavigationAction, which we have to call for every navigation, in every frame. It is true that PSON only applies to main frame but it does not impact logFrameNavigation().

[Brady Eidson]

Comment on attachment 342605 [details]
Patch

Fine with me.

[Chris Dumez]

Created attachment 342693 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 342693 [details]
Patch

Clearing flags on attachment: 342693

Committed r232814: <https://trac.webkit.org/changeset/232814>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/41103929>