| Summary: | Resource Load Statistics: User interaction should always go to top document | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | John Wilander <wilander> | ||||||||||||||
| Component: | WebKit2 | Assignee: | John Wilander <wilander> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | bfulgham, buildbot, cdumez, dbates, esprehn+autocc, japhet, jlewis3, kangil.han, webkit-bug-importer, wilander | ||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| Bug Depends on: | 174194 | ||||||||||||||||
| Bug Blocks: | |||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
John Wilander
2017-07-03 21:14:25 PDT
Created attachment 314552 [details]
Patch
Comment on attachment 314552 [details] Patch Attachment 314552 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4048889 New failing tests: http/tests/loading/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html Created attachment 314553 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Created attachment 314607 [details]
Patch
Comment on attachment 314607 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314607&action=review > Source/WebCore/testing/Internals.cpp:3897 > +void Internals::setResourceLoadStatisticsThrottledObserverNotifications(bool enable) Why do we need this Internals API, can't we just disable throttling all the time for WebKitTestRunner? Comment on attachment 314607 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314607&action=review > Source/WebCore/loader/ResourceLoadObserver.cpp:50 > +static ResourceLoadObserver::NotificationType notificationBehavior { ResourceLoadObserver::NotificationType::Throttled }; static bool shouldThrottleNotifications { false }; ? > Source/WebCore/loader/ResourceLoadObserver.cpp:58 > +void ResourceLoadObserver::setThrottledObserverNotifications(bool shouldThrottle) setShouldThrottleObserverNotifications(bool) > Source/WebCore/loader/ResourceLoadObserver.cpp:62 > + notificationBehavior = NotificationType::Throttled; Not convinced with need this enumeration. Maybe using the boolean is enough. > Source/WebCore/loader/ResourceLoadObserver.cpp:171 > + m_notificationCallback(notificationBehavior); Can we do the throttling at ResourceLoadObserver level so we don't have to expose this concept to the WebProcess code? > Source/WebKit2/WebProcess/WebProcess.cpp:202 > + ResourceLoadObserver::shared().setNotificationCallback([this] (ResourceLoadObserver::NotificationType notificationType) { I think throttling should be done on ResourceLoadObserver side. Also, it'd be nice if this callback would take a Vector<ResourceLoadStatistics>&& in parameter. We would then not need a takeStatistics() API on the observer either. Comment on attachment 314607 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314607&action=review > LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html:52 > + setTimeout(finishTest, 1000); Does this test *really* have to wait for 1 full second? Cannot we wait for some events instead? Thanks for the review comments, Chris! I'll get to them shortly. Just need to focus on two other patches first. Comment on attachment 314607 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314607&action=review >> Source/WebCore/loader/ResourceLoadObserver.cpp:50 >> +static ResourceLoadObserver::NotificationType notificationBehavior { ResourceLoadObserver::NotificationType::Throttled }; > > static bool shouldThrottleNotifications { false }; ? I think 'true'? > Source/WebKit2/WebProcess/WebProcess.cpp:214 > + } Yeah, it seem like maybe this would be enough: m_notificationTimer.startOneShot(shouldThrottleNotifications ? minimumNotificationInterval : 0_s); Created attachment 314940 [details] Patch (Rebased after Bug 174290) Comment on attachment 314940 [details] Patch (Rebased after Bug 174290) This should address Chris's comments, including getting rid of the 1 second timeout. Comment on attachment 314940 [details] Patch (Rebased after Bug 174290) Attachment 314940 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4082349 Number of test failures exceeded the failure limit. Created attachment 314942 [details]
Archive of layout-test-results from ews117 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Failures on EWS look real: Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.JavaScriptCore 0x0000000105eaab97 WTFCrash + 39 (Assertions.cpp:293) 1 com.apple.WebCore 0x000000010f8e1ae2 WebCore::ResourceLoadObserver::scheduleNotificationIfNeeded() + 82 (ResourceLoadObserver.cpp:239) 2 com.apple.WebCore 0x000000010f8e1a7b WebCore::ResourceLoadObserver::setShouldThrottleObserverNotifications(bool) + 59 (ResourceLoadObserver.cpp:66) Comment on attachment 314940 [details] Patch (Rebased after Bug 174290) View in context: https://bugs.webkit.org/attachment.cgi?id=314940&action=review > Source/WebCore/loader/ResourceLoadObserver.cpp:50 > +static bool shouldThrottleNotifications { true }; Could be a data member. > Source/WebCore/loader/ResourceLoadObserver.cpp:64 > + ResourceLoadObserver::shared().m_notificationTimer.stop(); This looks a bit ugly. Would look much nicer if this function were not static. > Source/WebCore/loader/ResourceLoadObserver.cpp:65 > + ResourceLoadObserver::shared().scheduleNotificationIfNeeded(); Won't this schedule a notification even if there was previously no notification scheduled? > Source/WebCore/loader/ResourceLoadObserver.h:53 > + WEBCORE_EXPORT static void setShouldThrottleObserverNotifications(bool); I don't think this needs to be static since ResourceLoadObserver is a singleton. > Source/WebCore/testing/Internals.cpp:3897 > +void Internals::setResourceLoadStatisticsThrottledObserverNotifications(bool enable) I think this should be called: setResourceLoadStatisticsShouldThrottleObserverNotifications > LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html:4 > +<script src="../../resources/js-test-pre.js"></script> Could use js-test.js > LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html:25 > + eventSender.mouseDown(); If this used resources/ui-helper.js and activateAt(x, y), this test would work on iOS as well. > LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html:29 > + resultInteractionTopFrame = testRunner.isStatisticsHasHadUserInteraction(topFrameOrigin); We don't really need this variable, we could inline the testRunner call in the shouldBeTrue / shouldBeFalse. > LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html:30 > + resultInteractionSubFrame = testRunner.isStatisticsHasHadUserInteraction(subFrameOrigin); We don't really need this variable, we could inline the testRunner call in the shouldBeTrue / shouldBeFalse. > LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html:31 > + shouldBe("resultInteractionTopFrame", "true"); shouldBeTrue() > LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html:32 > + shouldBe("resultInteractionSubFrame", "false"); shouldBeFalse() > LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html:48 > + currentTopFrameOrigin = document.location.origin; This variable does not seem needed. > LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html:52 > + resultInteractionTopFrame = testRunner.isStatisticsHasHadUserInteraction(topFrameOrigin); We don't really need this variable, we could inline the testRunner call in the shouldBeTrue / shouldBeFalse. > LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html:53 > + resultInteractionSubFrame = testRunner.isStatisticsHasHadUserInteraction(subFrameOrigin); We don't really need this variable, we could inline the testRunner call in the shouldBeTrue / shouldBeFalse. > LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html:54 > + shouldBe("resultInteractionTopFrame", "false"); shouldBeFalse(). > LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html:55 > + shouldBe("resultInteractionSubFrame", "false"); shouldBeFalse(). > LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html:61 > +<script src="../../resources/js-test-post.js"></script> Not needed Comment on attachment 314940 [details] Patch (Rebased after Bug 174290) View in context: https://bugs.webkit.org/attachment.cgi?id=314940&action=review >> Source/WebCore/loader/ResourceLoadObserver.cpp:50 >> +static bool shouldThrottleNotifications { true }; > > Could be a data member. Agreed. >> Source/WebCore/loader/ResourceLoadObserver.cpp:64 >> + ResourceLoadObserver::shared().m_notificationTimer.stop(); > > This looks a bit ugly. Would look much nicer if this function were not static. Yes -- sounds good, >> Source/WebCore/loader/ResourceLoadObserver.cpp:65 >> + ResourceLoadObserver::shared().scheduleNotificationIfNeeded(); > > Won't this schedule a notification even if there was previously no notification scheduled? Good point. I'll fix that. That's also the cause of the assertions in the debug build. >> Source/WebCore/loader/ResourceLoadObserver.h:53 >> + WEBCORE_EXPORT static void setShouldThrottleObserverNotifications(bool); > > I don't think this needs to be static since ResourceLoadObserver is a singleton. Will fix. >> Source/WebCore/testing/Internals.cpp:3897 >> +void Internals::setResourceLoadStatisticsThrottledObserverNotifications(bool enable) > > I think this should be called: > setResourceLoadStatisticsShouldThrottleObserverNotifications No objections -- I'll change to that. >> LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html:4 >> +<script src="../../resources/js-test-pre.js"></script> > > Could use js-test.js Ok. >> LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html:25 >> + eventSender.mouseDown(); > > If this used resources/ui-helper.js and activateAt(x, y), this test would work on iOS as well. Well let's do it then! :-) >> LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html:29 >> + resultInteractionTopFrame = testRunner.isStatisticsHasHadUserInteraction(topFrameOrigin); > > We don't really need this variable, we could inline the testRunner call in the shouldBeTrue / shouldBeFalse. That's true. It does make the test output a little more cluttered: PASS testRunner.isStatisticsHasHadUserInteraction(topFrameOrigin) is true >> LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html:30 >> + resultInteractionSubFrame = testRunner.isStatisticsHasHadUserInteraction(subFrameOrigin); > > We don't really need this variable, we could inline the testRunner call in the shouldBeTrue / shouldBeFalse. Will do. >> LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html:31 >> + shouldBe("resultInteractionTopFrame", "true"); > > shouldBeTrue() Ditto. >> LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html:32 >> + shouldBe("resultInteractionSubFrame", "false"); > > shouldBeFalse() Ditto. >> LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html:48 >> + currentTopFrameOrigin = document.location.origin; > > This variable does not seem needed. OK. >> LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html:52 >> + resultInteractionTopFrame = testRunner.isStatisticsHasHadUserInteraction(topFrameOrigin); > > We don't really need this variable, we could inline the testRunner call in the shouldBeTrue / shouldBeFalse. Will do! >> LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html:53 >> + resultInteractionSubFrame = testRunner.isStatisticsHasHadUserInteraction(subFrameOrigin); > > We don't really need this variable, we could inline the testRunner call in the shouldBeTrue / shouldBeFalse. Ditto. >> LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html:54 >> + shouldBe("resultInteractionTopFrame", "false"); > > shouldBeFalse(). Ditto. >> LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html:55 >> + shouldBe("resultInteractionSubFrame", "false"); > > shouldBeFalse(). Ditto. >> LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html:61 >> +<script src="../../resources/js-test-post.js"></script> > > Not needed Gone. Created attachment 314952 [details]
Patch
Comment on attachment 314952 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314952&action=review r=me with comments. > Source/WebCore/loader/ResourceLoadObserver.cpp:64 > + ASSERT(m_notificationCallback); I don't think this assertion is useful, it is already inside scheduleNotificationIfNeeded(); > LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html:48 > +<iframe id="testFrame" src="http://localhost:8000/loading/resourceLoadStatistics/resources/dummy.html"></iframe> I would use subFrameOrigin + "/loading/resourceLoadStatistics/resources/dummy.html" for clarity. > LayoutTests/platform/mac-wk2/TestExpectations:713 > +# Currently only tests with click, not tap. Why is this still skipped on iOS-wk2? Committed r219284: <http://trac.webkit.org/changeset/219284> Comment on attachment 314952 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314952&action=review >> Source/WebCore/loader/ResourceLoadObserver.cpp:64 >> + ASSERT(m_notificationCallback); > > I don't think this assertion is useful, it is already inside scheduleNotificationIfNeeded(); Okay. >> LayoutTests/http/tests/loading/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html:48 >> +<iframe id="testFrame" src="http://localhost:8000/loading/resourceLoadStatistics/resources/dummy.html"></iframe> > > I would use subFrameOrigin + "/loading/resourceLoadStatistics/resources/dummy.html" for clarity. WebKitTestRunner doesn't care for this syntax, and I couldn't find any examples of this kind of string composition in the other test files. I guess we could set 'src' manually using a composed string, but I don't think it's worth the effort for this case. >> LayoutTests/platform/mac-wk2/TestExpectations:713 >> +# Currently only tests with click, not tap. > > Why is this still skipped on iOS-wk2? Unskipped. After speaking with Brent, Skipping the test on iOS due to it causing consistent failures on iOS Simulator. https://trac.webkit.org/changeset/219299/webkit |