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.
<rdar://problem/33685546>
Created attachment 316999 [details] Patch
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?
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
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
Created attachment 317038 [details] Patch
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.
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
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
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.
Filed this follow-up bug on the iOS test: https://bugs.webkit.org/show_bug.cgi?id=175170
Created attachment 317195 [details] Patch
Comment on attachment 317195 [details] Patch Only running this patch on the bots before landing.
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.
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!
Comment on attachment 317195 [details] Patch Clearing flags on attachment: 317195 Committed r220268: <http://trac.webkit.org/changeset/220268>
All reviewed patches have been landed. Closing bug.
(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
Reverted r220268 for reason: This change caused assertion failures on macOS and iOS Debug WK2. Committed r220274: <http://trac.webkit.org/changeset/220274>
Created attachment 317253 [details] Patch
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.
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.
(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!
Created attachment 317273 [details] Patch
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
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
Window focus shenanigans. Characters from me trying to exit VI got into the test case HTML. :/ New upload coming.
Created attachment 317282 [details] Patch
Comment on attachment 317282 [details] Patch Clearing flags on attachment: 317282 Committed r220302: <http://trac.webkit.org/changeset/220302>