Bug 174120 – Resource Load Statistics: User interaction should always go to top document

• Component: WebKit2
• Status: RESOLVED FIXED

[John Wilander]

No matter where on a webpage a user interacts, the interaction should always be recorded for the top document. Users have no way of understanding what a cross-origin iframe is and they have no visual way to tell content from different origins apart.

[Radar WebKit Bug Importer]

<rdar://problem/33117899>

[John Wilander]

Created attachment 314552 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[John Wilander]

Created attachment 314607 [details]
Patch

[Chris Dumez]

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?

[Chris Dumez]

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.

[Chris Dumez]

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?

[John Wilander]

Thanks for the review comments, Chris! I'll get to them shortly. Just need to focus on two other patches first.

[Brent Fulgham]

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);

[Brent Fulgham]

Created attachment 314940 [details]
Patch (Rebased after Bug 174290)

[Brent Fulgham]

Comment on attachment 314940 [details]
Patch (Rebased after Bug 174290)

This should address Chris's comments, including getting rid of the 1 second timeout.

[Build Bot]

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.

[Build Bot]

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

[Chris Dumez]

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)

[Chris Dumez]

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

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 314952 [details]
Patch

[Chris Dumez]

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?

[Brent Fulgham]

Committed r219284: <http://trac.webkit.org/changeset/219284>

[Brent Fulgham]

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.

[Matt Lewis]

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