1099092 - Navigation Timing has incorrect values when page is load via link with target=_blank attribute

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Peter]

Attachment: Timing bug demonstration page

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/38.0.2125.104 Safari/537.36

Steps to reproduce:

Click on any link wich has target="_blank" attribute and check values of performance.timing object.


Actual results:

Absolutely all values in perforamce.timing are zero or equal to navigationStart value.


Expected results:

Most of values (especially DOM events timings) should not be zero or should be at least greater then navigationStart value.

Comment 1

[Peter]

Attachment: Update timing bug demo page

Comment 2

[Boris Zbarsky [:bzbarsky]]

Valentin, do you have time to take a look here?

Comment 3

[(PTO until Jan 6) Valentin Gosu [:valentin] (he/him)]

Sure, I'll take a look.

Comment 4

[(PTO until Jan 6) Valentin Gosu [:valentin] (he/him)]

I tracked it down to this https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e6614d8d85f9&tochange=7f81be7db528

The most likely culprit seems to be https://hg.mozilla.org/mozilla-central/rev/d1b960226e0b

Basically, what happens is that a DOMNavigationTiming is created for performance.timing, but when that gets nulled out and a new one is created, only the new one gets the updated timestamps and events.

I'm guessing that if we call mTiming->Clear() on the object, instead of creating a new one, that would fix the issue. I'll look into it.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Hmm.  The nulling out was to preserve preexisting behavior.

When we create the new mTiming, shouldn't we also associate it with a new or existing performance.timing?  It's not clear to me what the spec says here....

Comment 6

[(PTO until Jan 6) Valentin Gosu [:valentin] (he/him)]

Attachment: Navigation Timing has incorrect values when page is load via link with target=_blank attribute

This seems to work, but I'm not entirely sure this is the correct way to set the timing object on the window.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8534728 [details] [diff] [review]
Navigation Timing has incorrect values when page is load via link with target=_blank attribute

mBlankTiming never gets set to false.  I don't see how that can possibly be correct...

Need more tests, clearly.  :(

Comment 8

[(PTO until Jan 6) Valentin Gosu [:valentin] (he/him)]

Attachment: Add tests for navigation timing

Comment 9

[(PTO until Jan 6) Valentin Gosu [:valentin] (he/him)]

Attachment: Navigation Timing has incorrect values when page is load via link with target=_blank attribute

You are right. Also, that patch had bad values for unloadStart/End because I was creating a new timing for the window. I also found another error regarding redirectStart/End which was my fault (from the resource timing bug).
The new approach seems a little better to me, and I added a mochitest based on the testcase, which verifies the simple load, _blank and _self cases.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8535332 [details] [diff] [review]
Navigation Timing has incorrect values when page is load via link with target=_blank attribute

So if we land in MaybeInitTiming when mBlankTiming but !mScriptGlobal... won't we end up crashing on the null-deref when we do mTiming->NotifyNavigationStart()?

Comment 11

[(PTO until Jan 6) Valentin Gosu [:valentin] (he/him)]

That's correct. Is that a common case? I haven't seen any crashes on try.

How about 
> if (!mBlankTiming || !mScriptGlobal) {
>   mTiming = new nsDOMNavigationTiming();

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8535330 [details] [diff] [review]
Add tests for navigation timing

>+  for (var i in timingParams) {
>+    var name = timingParams[i];

  for (var name of timingParams) {

(both places).

>+function test_blank() {

Why is the setTimeout call here needed? Document.  And explicitly pass 0 as the timeout value?

r=me

>+function test_self() {

Same thing here.

>+function test_load() {

And here.

Comment 13

[(PTO until Jan 6) Valentin Gosu [:valentin] (he/him)]

Attachment: Remove expected-fails from WP navigation-timing tests

Comment 14

[(PTO until Jan 6) Valentin Gosu [:valentin] (he/him)]

Attachment: Navigation Timing has incorrect values when page is load via link with target=_blank attribute

Also a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8847ef9dd008

Comment 15

[(PTO until Jan 6) Valentin Gosu [:valentin] (he/him)]

Attachment: Add tests for navigation timing

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8538465 [details] [diff] [review]
Navigation Timing has incorrect values when page is load via link with target=_blank attribute

>+    bool                            mBlankTiming;

Needs a comment describing what it means.

Past that, seems reasonable, I think.  I assume you've tested both cases that reuse the inner window and ones that don't?

Comment 17

[(PTO until Jan 6) Valentin Gosu [:valentin] (he/him)]

Attachment: Navigation Timing has incorrect values when page is load via link with target=_blank attribute

Comment 18

[(PTO until Jan 6) Valentin Gosu [:valentin] (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #16)
> Past that, seems reasonable, I think.  I assume you've tested both cases
> that reuse the inner window and ones that don't?

I did a manual test, and it seems to behave correctly.

Comment 19

[Boris Zbarsky [:bzbarsky]]

What were the manual steps you used?

Comment 20

[(PTO until Jan 6) Valentin Gosu [:valentin] (he/him)]

I opened a page and checked window.performance.timing
Clicked on a link (same domain) and checked that the timing info is different
Clicked the Back button, and checked that the timing info is the same as the first one.
Refreshed, checked that the timing info has changed.
Clicked on a link (other domain) and checked that the timing info has changed (and unloadEvent timings are 0)

Opening links with _blank and _self is tested in the mochitest.

Comment 21

[Boris Zbarsky [:bzbarsky]]

OK.  Here's what I meant in comment 16.

1)  Page opens a window with window.open() but no url, then later sets location of that window to a same-origin URL.  Want to make sure that the timing is sane both before the location set and after.

2)  Page opens a window with window.open() but no url, then later sets location of that window to a cross-origin URL.  Want to make sure that the timing is sane both before the location set and after.

3)  A link with target=_blank, pointing to a same-origin URL; check timing after page finishes loading.

4)  A link with target=_blank, pointing to a cross-origin URL; check timing after page finishes loading.

Comment 22

[(PTO until Jan 6) Valentin Gosu [:valentin] (he/him)]

I checked these cases as well, and everything seems fine.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Great, thank you!

Comment 24

[(PTO until Jan 6) Valentin Gosu [:valentin] (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2d5cd863b14
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0341b791d11
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a4d06a8f2ca

Comment 25

[(PTO until Jan 6) Valentin Gosu [:valentin] (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f15e6350d86b

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f2d5cd863b14
https://hg.mozilla.org/mozilla-central/rev/f0341b791d11
https://hg.mozilla.org/mozilla-central/rev/7a4d06a8f2ca
https://hg.mozilla.org/mozilla-central/rev/f15e6350d86b