Bug 175090 – Resource Load Statistics: Report user interaction immediately, but only when needed

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[John Wilander]

User interaction is an important signal for websites that use it to get out of partitioning. It should be reported immediately to the UI process to not risk the throttling delay. We should also keep track of the domains we have already reported and not send messages about those until needed.

[Radar WebKit Bug Importer]

<rdar://problem/33685546>

[John Wilander]

Created attachment 316999 [details]
Patch

[Chris Dumez]

Comment on attachment 316999 [details]
Patch

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

> Source/WebCore/loader/ResourceLoadObserver.cpp:273
> +    if (lastReportedUserInteraction && newTime == lastReportedUserInteraction)

Do we really need this "lastReportedUserInteraction &&" check? newTime cannot be 0 so the second part of the check should be enough, no?

> Source/WebCore/loader/ResourceLoadObserver.cpp:292
> +void ResourceLoadObserver::immediateUserInteractionNotification(const String& primaryDomain, const WallTime& time)

Maybe notifyImmediately()?

> Source/WebCore/loader/ResourceLoadObserver.cpp:294
> +    if (!m_notificationTimer.isActive()) {

Because of this check, you may still wait 5 seconds I believe.

> Source/WebCore/loader/ResourceLoadObserver.cpp:295
> +        m_lastReportedUserInteractionMap.set(primaryDomain, time);

May be nicer at call site, where you already check the map.

> Source/WebCore/loader/ResourceLoadObserver.cpp:296
> +        m_notificationTimer.startOneShot(0_s);

Why not synchronously?

[Build Bot]

Comment on attachment 316999 [details]
Patch

Attachment 316999 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4242469

New failing tests:
http/tests/security/contentSecurityPolicy/audio-redirect-allowed.html

[Build Bot]

Created attachment 317033 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[John Wilander]

Created attachment 317038 [details]
Patch

[John Wilander]

Thanks, Chris! I addressed all your comments which simplified the code. Also came up with a test case that covers repeated user interaction not being reported.

[Build Bot]

Comment on attachment 317038 [details]
Patch

Attachment 317038 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4243522

New failing tests:
http/tests/loading/resourceLoadStatistics/user-interaction-only-reported-once-within-short-period-of-time.html

[Build Bot]

Created attachment 317074 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Chris Dumez]

Comment on attachment 317038 [details]
Patch

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

r=me with comments. Also, please take a look at the iOS failure.

> Source/WebCore/loader/ResourceLoadObserver.cpp:284
> +    notificationTimerFired();

I think this would look better if we called it notifyObserver(). Maybe the call to m_notificationTimer.stop() could be moved inside notifyObserver() too.

> Source/WebCore/loader/ResourceLoadObserver.cpp:337
> +    m_lastReportedUserInteractionMap.clear();

I think it would be nice to stop m_notificationTimer too.

> LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html:28
>      if (internals) {

Should drop the curly brackets.

> LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-only-reported-once-within-short-period-of-time.html:57
> +        if (internals) {

Should drop the curly brackets.

[John Wilander]

Filed this follow-up bug on the iOS test: https://bugs.webkit.org/show_bug.cgi?id=175170

[John Wilander]

Created attachment 317195 [details]
Patch

[John Wilander]

Comment on attachment 317195 [details]
Patch

Only running this patch on the bots before landing.

[Chris Dumez]

Comment on attachment 317195 [details]
Patch

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

> LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-only-reported-once-within-short-period-of-time.html:17
> +        UIHelper.activateAt(centerX, centerY);

This returns a promise. The tap is async on iOS. This may be your issue.

[John Wilander]

Comment on attachment 317195 [details]
Patch

I discussed the promise with Wenson. It might be it but since I wait for ITP’s callback the interaction should have been caught anyway. Will check in the other bug.

Thanks for the review!

[WebKit Commit Bot]

Comment on attachment 317195 [details]
Patch

Clearing flags on attachment: 317195

Committed r220268: <http://trac.webkit.org/changeset/220268>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

(In reply to WebKit Commit Bot from comment #16)
> Comment on attachment 317195 [details]
> Patch
> 
> Clearing flags on attachment: 317195
> 
> Committed r220268: <http://trac.webkit.org/changeset/220268>
This change caused assertion failures on macOS and iOS Debug WK2 bots:
https://build.webkit.org/builders/Apple%20Sierra%20Debug%20WK2%20(Tests)/builds/2361

ASSERTION FAILED: !statistics.isEmpty()
/Volumes/Data/slave/sierra-debug/build/Source/WebKit/WebProcess/WebProcess.cpp(201) : auto WebKit::WebProcess::WebProcess()::(anonymous class)::operator()(Vector<WebCore::ResourceLoadStatistics> &&) const
1   0x1087b255d WTFCrash
2   0x1028c8055 WebKit::WebProcess::WebProcess()::$_1::operator()(WTF::Vector<WebCore::ResourceLoadStatistics, 0ul, WTF::CrashOnOverflow, 16ul>&&) const
3   0x1028c7f54 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   0x10f5dccc1 WTF::Function<void (WTF::Vector<WebCore::ResourceLoadStatistics, 0ul, WTF::CrashOnOverflow, 16ul>&&)>::operator()(WTF::Vector<WebCore::ResourceLoadStatistics, 0ul, WTF::CrashOnOverflow, 16ul>&&) const
5   0x10f5db98b WebCore::ResourceLoadObserver::notifyObserver()
6   0x10f5de95d WTF::Function<void ()>::CallableWrapper<std::__1::__bind<void (WebCore::ResourceLoadObserver::*&)(), WebCore::ResourceLoadObserver*> >::call()
7   0x10d0c56ee WTF::Function<void ()>::operator()() const
8   0x10d0c5579 WebCore::Timer::fired()
9   0x10fb55160 WebCore::ThreadTimers::sharedTimerFiredInternal()
10  0x10fb558c1 WebCore::ThreadTimers::setSharedTimer(WebCore::SharedTimer*)::$_0::operator()() const
11  0x10fb55879 WTF::Function<void ()>::CallableWrapper<WebCore::ThreadTimers::setSharedTimer(WebCore::SharedTimer*)::$_0>::call()
12  0x10d0c56ee WTF::Function<void ()>::operator()() const
13  0x10ee321c5 WebCore::MainThreadSharedTimer::fired()
14  0x10ee32559 WebCore::timerFired(__CFRunLoopTimer*, void*)
15  0x7fffafbf4e04 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__
16  0x7fffafbf4a93 __CFRunLoopDoTimer
17  0x7fffafbf45ea __CFRunLoopDoTimers
18  0x7fffafbebfc1 __CFRunLoopRun
19  0x7fffafbeb544 CFRunLoopRunSpecific
20  0x7fffaf14bebc RunCurrentEventLoopInMode
21  0x7fffaf14bcf1 ReceiveNextEventCommon
22  0x7fffaf14bb26 _BlockUntilNextEventMatchingListInModeWithFilter
23  0x7fffad6e4a54 _DPSNextEvent
24  0x7fffade607ee -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:]
25  0x7fffad6d93db -[NSApplication run]
26  0x7fffad6a3e0e NSApplicationMain
27  0x7fffc5b7c8c7 _xpc_objc_main
28  0x7fffc5b7b2e4 xpc_main
29  0x101de9105 main
30  0x7fffc5923235 start
31  0x1

[Ryan Haddad]

Reverted r220268 for reason:

This change caused assertion failures on macOS and iOS Debug WK2.

Committed r220274: <http://trac.webkit.org/changeset/220274>

[John Wilander]

Created attachment 317253 [details]
Patch

[John Wilander]

Comment on attachment 317253 [details]
Patch

Moved the check for whether statistics are empty to the ResourceLoadObserver::notifyObserver() function to hopefully avoid the assert fail.

[Chris Dumez]

Comment on attachment 317253 [details]
Patch

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

> Source/WebCore/loader/ResourceLoadObserver.cpp:304
> +    if (m_resourceStatisticsMap.isEmpty())

I don't like this proposal. I believe the issue is that you forgot to stop the timer in some place. We should fix that issue instead of dealing with having no data when the timer fires.

> Source/WebCore/loader/ResourceLoadObserver.cpp:334
> +    m_lastReportedUserInteractionMap.clear();

In my previous review feedback, I asked to stop the timer here. I see this is not done here. Why didn't you address this review comment? Also note that this could be the reason why we hit the assertion.

[John Wilander]

(In reply to Chris Dumez from comment #22)
> Comment on attachment 317253 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=317253&action=review
> 
> > Source/WebCore/loader/ResourceLoadObserver.cpp:304
> > +    if (m_resourceStatisticsMap.isEmpty())
> 
> I don't like this proposal. I believe the issue is that you forgot to stop
> the timer in some place. We should fix that issue instead of dealing with
> having no data when the timer fires.

Got it.

> > Source/WebCore/loader/ResourceLoadObserver.cpp:334
> > +    m_lastReportedUserInteractionMap.clear();
> 
> In my previous review feedback, I asked to stop the timer here. I see this
> is not done here. Why didn't you address this review comment? Also note that
> this could be the reason why we hit the assertion.

Sorry, that was an oversight. I'll try this approach. Thanks!

[John Wilander]

Created attachment 317273 [details]
Patch

[Build Bot]

Comment on attachment 317273 [details]
Patch

Attachment 317273 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4254850

New failing tests:
http/tests/loading/resourceLoadStatistics/user-interaction-only-reported-once-within-short-period-of-time.html

[Build Bot]

Created attachment 317281 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[John Wilander]

Window focus shenanigans. Characters from me trying to exit VI got into the test case HTML. :/ New upload coming.

[John Wilander]

Created attachment 317282 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 317282 [details]
Patch

Clearing flags on attachment: 317282

Committed r220302: <http://trac.webkit.org/changeset/220302>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.