RESOLVED FIXED 175090
Resource Load Statistics: Report user interaction immediately, but only when needed
https://bugs.webkit.org/show_bug.cgi?id=175090
Summary Resource Load Statistics: Report user interaction immediately, but only when ...
John Wilander
Reported 2017-08-02 14:33:25 PDT
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.
Attachments
Patch (8.78 KB, patch)
2017-08-02 14:44 PDT, John Wilander
no flags
Archive of layout-test-results from ews115 for mac-elcapitan (1.83 MB, application/zip)
2017-08-02 17:01 PDT, Build Bot
no flags
Patch (18.38 KB, patch)
2017-08-02 17:17 PDT, John Wilander
no flags
Archive of layout-test-results from ews126 for ios-simulator-wk2 (9.85 MB, application/zip)
2017-08-02 20:11 PDT, Build Bot
no flags
Patch (19.40 KB, patch)
2017-08-03 18:42 PDT, John Wilander
no flags
Patch (19.88 KB, patch)
2017-08-04 10:08 PDT, John Wilander
no flags
Patch (19.04 KB, patch)
2017-08-04 13:16 PDT, John Wilander
no flags
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.27 MB, application/zip)
2017-08-04 14:02 PDT, Build Bot
no flags
Patch (19.03 KB, patch)
2017-08-04 14:05 PDT, John Wilander
no flags
Radar WebKit Bug Importer
Comment 1 2017-08-02 14:33:55 PDT
John Wilander
Comment 2 2017-08-02 14:44:41 PDT
Chris Dumez
Comment 3 2017-08-02 15:03:32 PDT
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 4 2017-08-02 17:01:28 PDT
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
Comment 5 2017-08-02 17:01:29 PDT
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
Comment 6 2017-08-02 17:17:08 PDT
John Wilander
Comment 7 2017-08-02 17:18:40 PDT
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 8 2017-08-02 20:11:23 PDT
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
Comment 9 2017-08-02 20:11:25 PDT
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 10 2017-08-02 21:20:18 PDT
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
Comment 11 2017-08-03 18:29:18 PDT
Filed this follow-up bug on the iOS test: https://bugs.webkit.org/show_bug.cgi?id=175170
John Wilander
Comment 12 2017-08-03 18:42:06 PDT
John Wilander
Comment 13 2017-08-03 18:42:37 PDT
Comment on attachment 317195 [details] Patch Only running this patch on the bots before landing.
Chris Dumez
Comment 14 2017-08-03 18:46:59 PDT
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 15 2017-08-04 06:50:20 PDT
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 16 2017-08-04 07:20:05 PDT
Comment on attachment 317195 [details] Patch Clearing flags on attachment: 317195 Committed r220268: <http://trac.webkit.org/changeset/220268>
WebKit Commit Bot
Comment 17 2017-08-04 07:20:07 PDT
All reviewed patches have been landed. Closing bug.
Ryan Haddad
Comment 18 2017-08-04 09:13:42 PDT
(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
Comment 19 2017-08-04 09:18:03 PDT
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
Comment 20 2017-08-04 10:08:26 PDT
John Wilander
Comment 21 2017-08-04 10:09:32 PDT
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 22 2017-08-04 10:25:46 PDT
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
Comment 23 2017-08-04 11:33:39 PDT
(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
Comment 24 2017-08-04 13:16:25 PDT
Build Bot
Comment 25 2017-08-04 14:02:19 PDT
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
Comment 26 2017-08-04 14:02:20 PDT
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
Comment 27 2017-08-04 14:05:11 PDT
Window focus shenanigans. Characters from me trying to exit VI got into the test case HTML. :/ New upload coming.
John Wilander
Comment 28 2017-08-04 14:05:53 PDT
WebKit Commit Bot
Comment 29 2017-08-04 15:35:00 PDT
Comment on attachment 317282 [details] Patch Clearing flags on attachment: 317282 Committed r220302: <http://trac.webkit.org/changeset/220302>
WebKit Commit Bot
Comment 30 2017-08-04 15:35:02 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.