Add nested-context resource-timing tests #13823

yoavweiss · 2018-10-31T15:08:02Z

No description provided.

resource-timing/embed-refresh.sub.html

resource-timing/embed-reload-back.html

resource-timing/embed-refresh.sub.html

resource-timing/embed-reload.sub.html

bzbarsky · 2018-11-02T14:26:49Z

resource-timing/embed-reload.sub.html

+            if (entry.name == "{{location[scheme]}}://{{host]}}:{{ports[http][0]}}/resource-timing/resources/iframe-that-reloads.html") {
+                found_first_iframe = true;
+            }
+            assert_not_equals(entry.name, "{{location[scheme]}}://{{host]}}:{{ports[http][0]}}/resource-timing/resources/reloaded_iframe.html", "Reloaded iframe should not be observable");


This will always pass, because there is no "reloaded_iframe.html" anywhere in these tests. Similar things with "refreshed_iframe.html" in some of the other tests. So this is not testing what it thinks it's testing.

I wonder whether it's feasible to have a complete whitelist of resource timing names that should be present, and fail the test if they're not all present or if something else is present. That would avoid issues like this...

I'm going to hold off on the rest of this review until the above issues are addressed, because they will significantly affect the readability of the tests, and possibly the logic flow...

Indeed! Good catch

I wonder whether it's feasible to have a complete whitelist of resource timing names that should be present, and fail the test if they're not all present or if something else is present. That would avoid issues like this...

There seem to be differences between browsers on that front (which is not great...), so better not go the whitelist route.

Hmm. What are the differences? Could we have a list of must-haves and a list of may-haves, maybe? And file bugs on specs and/or browsers as needed to eliminate the may-haves?

One difference ATM is failed fetches (See them in Firefox, but not Chrome), which is currently a MAY in the spec.
Once we'll define a stricter processing model, I think we can also align the implementations on a stricter behavior, where the entries are predictable in all engines.

Are there follow-up issues for this?

w3c/resource-timing#39 is the umbrella issue for that. w3c/resource-timing#12 was the more specific one that covered failed fetches

yoavweiss · 2018-11-06T08:24:17Z

@bzbarsky - PTAL?

yoavweiss · 2018-12-05T16:06:50Z

@bzbarsky - friendly ping :)

yoavweiss · 2019-01-04T12:41:17Z

@bzbarsky - post holidays ping :)

annevk

It also seems like a lot of the JavaScript could be abstracted somehow as the files look very similar. That would also help in reviewing them (and presumably when adding more tests at some point).

I hope this high-level review is useful.

I haven't run these yet, but if there's browser failures, are there bugs filed on those?

resource-timing/embed-navigate-back.html

lint.whitelist

resource-timing/embed-navigate-back.html

yoavweiss · 2019-03-12T15:58:40Z

It also seems like a lot of the JavaScript could be abstracted somehow as the files look very similar. That would also help in reviewing them (and presumably when adding more tests at some point).

I hope this high-level review is useful.

I haven't run these yet, but if there's browser failures, are there bugs filed on those?

Thanks for reviewing! :)

I tidied up and coalesced the JS.

Haven't filed bugs yet, but I will.

annevk

A thing I noticed is that there's no cross-origin tests anymore. Is that intentional? /common/get-host-info.sub.js should make those relatively easy to add to add.

Without cross-origin it's not really clear to me why you'd even need the URL object here.

resource-timing/embed-navigate-back.html

resource-timing/embed-navigate.html

resource-timing/embed-refresh.html

annevk · 2019-03-13T09:16:36Z

Firefox appears to pass all these tests, FWIW.

yoavweiss · 2019-03-13T10:07:30Z

A thing I noticed is that there's no cross-origin tests anymore. Is that intentional? /common/get-host-info.sub.js should make those relatively easy to add to add.

I didn't have them initially, but will add just to be on the safe side.

Without cross-origin it's not really clear to me why you'd even need the URL object here.

We need the full URL for the PerformanceEntry.name comparison.

…o one top level HTML

resource-timing/resources/nested-contexts.js

Stale

bzbarsky

@yoavweiss @annevk Thank you! This is very readable now and quite clear in terms of what it's doing. My sincere apologies for being so slow on reviewing this... :(

@yoavweiss I really appreciate the work you put into this and you pushing this over the finish line in spite of me timing out on you. Thank you, again.

bzbarsky · 2019-03-13T15:30:46Z

resource-timing/resources/nested-contexts.js

+    }
+    window.addEventListener("message", e=>{
+        if (e.data == "navigated") {
+            if (sessionStorage.navigated) {


It might have been good to document somewhere the tight coupling between this and the exact test-serialization behavior we get from using promise_test for everything, thus ensuring that we never touch sessionStorage from two different tests in a racy way.

bzbarsky · 2019-03-13T15:44:53Z

resource-timing/resources/nested-contexts.js

+let destination = location;
+if (location.search == "?crossorigin") {
+    const host_info = get_host_info()
+    destination = location.protocol + "//" + host_info["REMOTE_HOST"] + ":" + location.port;


As a followup, it might be good to have not-samesite tests here too, just to test the cross-process case. hostinfo does not expose NOTSAMESITE_HOST at the moment, so that would need to get fixed too... I filed #15819 on that.

But this test is already cross-origin between localhost and some other host, or am I missing something?

IIRC, @bzbarsky was asking for cross-origin and cross-site tests (so using hosts that have different domains)

Right but this test is both cross-origin and cross-site if runs from localhost. Maybe what's missing is same-site cross-origin?

This test does not normally run "from localhost". It runs from one of the defined test hosts in the test configuration. Specifically, from whatever {{host}} expands to. Depends on the harness what that is, but it's generally not localhost, but something more like web-platform.test or wpt.live or whatnot, depending on the test runner configuration.

And for details, see the get_host_info function's implementation and ORIGINAL_HOST and REMOTE_HOST and NOTSAMESITE_HOST in there. It has provisions for what to do if running from localhost, but also provisions for what to do in other cases. So yes, when running from localhost the test is testing the cross-site case and can't test the same-site cross-origin case at all.... which is why you should not be running these tests from localhost: a bunch of them will fail due to the lack of an available same-site cross-origin host in that setup.

Gotcha. I wasn't aware of these details. Thanks!

web-platform-tests/wpt#13823 introduces a test which is flaky in Chromium, due to a Chromium issue. This CL will enable that test to be imported without flaking the bots. Bug: 941482 Change-Id: I7568bd00d55743ab140f428b807cf2ffc92a9fda Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1520601 Commit-Queue: Robert Ma <[email protected]> Reviewed-by: Robert Ma <[email protected]> Cr-Commit-Position: refs/heads/master@{#640459}

wpt-pr-bot added resource-timing wg-webperf labels Oct 31, 2018

wpt-pr-bot requested review from haoxli, igrigorik, plehegar, toddreifsteck and zqzhang October 31, 2018 15:08

wpt-pr-bot added the infra label Oct 31, 2018

wpt-pr-bot requested a review from jgraham October 31, 2018 15:32

yoavweiss mentioned this pull request Oct 31, 2018

Better define which entries are exposed for nested contexts w3c/resource-timing#177

Merged

yoavweiss requested review from bzbarsky and annevk October 31, 2018 20:25

bzbarsky previously requested changes Nov 2, 2018

View reviewed changes

jugglinmike mentioned this pull request Nov 7, 2018

Verify correctness of Taskcluster PR checks #13194

Closed

wpt-pr-bot assigned yoavweiss Dec 5, 2018

yoavweiss mentioned this pull request Jan 11, 2019

Transition Level 2 to CR w3c/resource-timing#71

Closed

10 tasks

yoavweiss force-pushed the resource-timing-nested branch from 5bf797d to dbdc1c2 Compare March 12, 2019 12:05

annevk reviewed Mar 12, 2019

View reviewed changes

resource-timing/embed-navigate-back.html Outdated Show resolved Hide resolved

lint.whitelist Outdated Show resolved Hide resolved

resource-timing/embed-navigate-back.html Outdated Show resolved Hide resolved

yoavweiss added 7 commits March 12, 2019 20:17

Add nested-context resource-timing tests

a9d38f6

lint

9c407d1

Almost empty commit

8670ca6

review comments

4f31657

review comments

760b9d3

s/reload/navigate/

2abb55d

Annevk review comments

06075fb

yoavweiss force-pushed the resource-timing-nested branch from 4bbc152 to 06075fb Compare March 12, 2019 19:19

annevk reviewed Mar 13, 2019

View reviewed changes

resource-timing/embed-navigate-back.html Outdated Show resolved Hide resolved

resource-timing/embed-navigate.html Outdated Show resolved Hide resolved

resource-timing/embed-refresh.html Outdated Show resolved Hide resolved

Follow advice from Anne for easier local tests

2c63ca2

yoavweiss added 3 commits March 13, 2019 13:47

Added crossorigin tests for the navigate-back tests

8937681

Added crossorigin tests for everything else. Consolidated all tests t…

8f0c173

…o one top level HTML

lint

69dd2aa

annevk reviewed Mar 13, 2019

View reviewed changes

resource-timing/resources/nested-contexts.js Outdated Show resolved Hide resolved

yoavweiss added 2 commits March 13, 2019 15:18

Long timeout to make the bots happy

7f004db

const

50b4cd2

annevk approved these changes Mar 13, 2019

View reviewed changes

annevk merged commit 75913b3 into web-platform-tests:master Mar 13, 2019

yoavweiss deleted the resource-timing-nested branch March 13, 2019 15:28

bzbarsky reviewed Mar 13, 2019

View reviewed changes

jugglinmike mentioned this pull request Jul 18, 2019

hostinfo should expose NOTSAMESITE_HOST #15819

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add nested-context resource-timing tests #13823

Add nested-context resource-timing tests #13823

yoavweiss commented Oct 31, 2018

bzbarsky Nov 2, 2018

yoavweiss Nov 2, 2018

bzbarsky Nov 3, 2018

yoavweiss Nov 6, 2018

annevk Mar 12, 2019

yoavweiss Mar 12, 2019 •

edited

Loading

yoavweiss commented Nov 6, 2018

yoavweiss commented Dec 5, 2018

yoavweiss commented Jan 4, 2019

annevk left a comment

yoavweiss commented Mar 12, 2019

annevk left a comment

annevk commented Mar 13, 2019

yoavweiss commented Mar 13, 2019

bzbarsky left a comment

bzbarsky Mar 13, 2019

bzbarsky Mar 13, 2019

noamr Feb 8, 2022

yoavweiss Feb 8, 2022

noamr Feb 8, 2022

bzbarsky Feb 8, 2022 •

edited

Loading

bzbarsky Feb 8, 2022 •

edited

Loading

noamr Feb 8, 2022

Add nested-context resource-timing tests #13823

Add nested-context resource-timing tests #13823

Conversation

yoavweiss commented Oct 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoavweiss Mar 12, 2019 • edited Loading

Choose a reason for hiding this comment

yoavweiss commented Nov 6, 2018

yoavweiss commented Dec 5, 2018

yoavweiss commented Jan 4, 2019

annevk left a comment

Choose a reason for hiding this comment

yoavweiss commented Mar 12, 2019

annevk left a comment

Choose a reason for hiding this comment

annevk commented Mar 13, 2019

yoavweiss commented Mar 13, 2019

bzbarsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzbarsky Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

bzbarsky Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoavweiss Mar 12, 2019 •

edited

Loading

bzbarsky Feb 8, 2022 •

edited

Loading

bzbarsky Feb 8, 2022 •

edited

Loading