WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
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
Details
Patch
(18.38 KB, patch)
2017-08-02 17:17 PDT
,
John Wilander
no flags
Details
Formatted Diff
Diff
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
Details
Patch
(19.40 KB, patch)
2017-08-03 18:42 PDT
,
John Wilander
no flags
Details
Formatted Diff
Diff
Patch
(19.88 KB, patch)
2017-08-04 10:08 PDT
,
John Wilander
no flags
Details
Formatted Diff
Diff
Patch
(19.04 KB, patch)
2017-08-04 13:16 PDT
,
John Wilander
no flags
Details
Formatted Diff
Diff
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
Details
Patch
(19.03 KB, patch)
2017-08-04 14:05 PDT
,
John Wilander
no flags
Details
Formatted Diff
Diff
Show Obsolete
(8)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2017-08-02 14:33:55 PDT
<
rdar://problem/33685546
>
John Wilander
Comment 2
2017-08-02 14:44:41 PDT
Created
attachment 316999
[details]
Patch
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
Created
attachment 317038
[details]
Patch
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
Created
attachment 317195
[details]
Patch
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
Created
attachment 317253
[details]
Patch
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
Created
attachment 317273
[details]
Patch
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
Created
attachment 317282
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug