1462879 - PerformanceNavigationTiming must be notified correctly

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Andrea Marchesini [:baku]]

https://w3c.github.io/navigation-timing/#processing-model

The spec says that:

[...]
2. "Create a new PerformanceNavigationTiming object and add it to the performance entry buffer."
This makes the entry immediately available via getEntries()
[...]
After the load event ends:
28. "Queue the new PerformanceNavigationTiming object."

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - Notify

Comment 2

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8979001 [details] [diff] [review]
part 1 - Notify

Review of attachment 8979001 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 2 - getEntriesByName

Before, PerformanceNavigationTiming.name was always 'document'. Now it contains the URL of the document. This must be considered in getEntriesByName().

In addition, it's possible to have multiple entries with the same name.
This bug is needed to fix some WPTs.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 3 - Update the channel properties before notify

Comment 6

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8979008 [details] [diff] [review]
part 2 - getEntriesByName

Review of attachment 8979008 [details] [diff] [review]:
-----------------------------------------------------------------

r+, it you really did want me to review it, not bz :)

Comment 7

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8979009 [details] [diff] [review]
part 3 - Update the channel properties before notify

Review of attachment 8979009 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/tests/navigation-timing/nav2_test_attributes_values.html
@@ +93,5 @@
>                      // running this test.
>                      assert_true(entries[0].transferSize > entries[0].encodedBodySize,
>                          "Expected transferSize to be greater than encodedBodySize in uncached navigation.");
> +                    assert_equals(entries[0].encodedBodySize, 5272);
> +                    assert_equals(entries[0].decodedBodySize, 5272);

Do these tests pass in other browsers?

Comment 8

[Andrea Marchesini [:baku]]

> Do these tests pass in other browsers?

The size changed because I had to change the URL for the comparison with PerformanceNavigationTiming.name

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3058ee77146
PerformanceNavigationTiming must be notified correctly, r=valentin, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f2b2965aa89
Performance.getEntriesByName must consider the PerformanceNavigationTiming name, r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b3a818279f3
Update the channel properties before notify, r=valentin

Comment 10

[Noemi Erli[:noemi_erli]]

Backed out 3 changesets (bug 1462879) for wpt failures in nav2_test_document_open.html and mochitest failures in test_resource_timing.html on a CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1b3a818279f31a04e5934dd4139302afbdd779e3&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&selectedJob=180007103

Failure logs: https://treeherder.mozilla.org/logviewer.html#?job_id=180007103&repo=mozilla-inbound&lineNumber=20674
https://treeherder.mozilla.org/logviewer.html#?job_id=180004646&repo=mozilla-inbound

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/a89aed7f22d05d2f009e79daba44d825e8a7cc01

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e171db33014
PerformanceNavigationTiming must be notified correctly, r=valentin, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5ae7c2df4a4
Performance.getEntriesByName must consider the PerformanceNavigationTiming name, r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae1bf2d218bb
Update the channel properties before notify, r=valentin

Comment 12

[Cristina Coroiu [:ccoroiu]]

Backed out 3 changesets (bug 1462879) for failing mochitests on Android at dom/tests/mochitest/general/test_resource_timing.htm

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/6a7c05d39fdfb5082565fb076f051a4437f813aa

Failure push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ae1bf2d218bb2db3d1438cbf492a772e6eb5ec82

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=183242346&repo=mozilla-inbound&lineNumber=1671

[task 2018-06-15T00:06:08.789Z] 00:06:08     INFO -  350 INFO TEST-PASS | dom/tests/mochitest/general/test_resource_timing.html | http://mochi.test:8888/tests/image/test/mochitest/damon.jpg should be a valid entry name
[task 2018-06-15T00:06:08.789Z] 00:06:08     INFO -  Buffered messages finished
[task 2018-06-15T00:06:08.789Z] 00:06:08     INFO -  351 INFO TEST-UNEXPECTED-FAIL | dom/tests/mochitest/general/test_resource_timing.html | This iframe should contain itself only once
[task 2018-06-15T00:06:08.789Z] 00:06:08     INFO -      ok@dom/tests/mochitest/general/resource_timing_main_test.html:55:3
[task 2018-06-15T00:06:08.789Z] 00:06:08     INFO -      doTest@dom/tests/mochitest/general/resource_timing_iframe.html:32:3
[task 2018-06-15T00:06:08.789Z] 00:06:08     INFO -      onload@dom/tests/mochitest/general/resource_timing_iframe.html:1:1

Comment 13

[Andrea Marchesini [:baku]]

What blocks the landing of this patches is a failure on android. It seems that the nsIChannel doesn't have the correct url spec, in iframes, when onload is triggered. The name is taken here:

https://searchfox.org/mozilla-central/rev/285da1fd7dcf67448b9175741fa330158edcff73/dom/performance/PerformanceMainThread.cpp#18-39

but somehow, only on android, here we have 'document' instead of the correct iframe URL:

https://searchfox.org/mozilla-central/rev/285da1fd7dcf67448b9175741fa330158edcff73/dom/tests/mochitest/general/resource_timing_iframe.html#30-33

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d36901e8058e
PerformanceNavigationTiming must be notified correctly - part 1 - notify, r=smaug, r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4fae17b04a6
PerformanceNavigationTiming must be notified correctly - part 2 - getEntryByName, r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb557622b966
PerformanceNavigationTiming must be notified correctly - part 3 - Update the channel properties before notify, r=valentin

Comment 15

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/d36901e8058e
https://hg.mozilla.org/mozilla-central/rev/c4fae17b04a6
https://hg.mozilla.org/mozilla-central/rev/cb557622b966