1459711 - window.performance.navigation.type Doesn't Return 2 When Reaching a Page via Back or Forward via bfcache

Status: RESOLVED FIXED

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

Description

[Andy Mercer]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20180427210249

Steps to reproduce:

Navigate to https://stackexchange.com/
Open Inspector Tool
Run in the console: performance.navigation.type
Refresh the page.
Run in the console: performance.navigation.type
Click on "Tour" link, then navigate back one page.
Run in the console: performance.navigation.type


Actual results:

Console shows: 0, then 1, then 1.


Expected results:

Console should show: 0, then 1, then 2.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Presumably we need to update performance.navigation.type when doing a load from bfcache.  The spec defines this attribute as returning information about the browsing context, not about the page involved, so the .type can change, per spec, even if the page data structures have not changed.

In particular, the .type should get updated on anchor scrolls as well (to TYPE_NAVIGATE), which we also don't do right now.

Comment 2

[Boris Zbarsky [:bzbarsky]]

And in particular, we should probably just fetch the type from the docshell, since that's the spec model.

Andrea, do you have time to look at this?  If not, I can do it.

Comment 3

[Andrea Marchesini [:baku]]

> Andrea, do you have time to look at this?  If not, I can do it.

Should it be enough to expose ConvertLoadTypeToNavigationType(mLoadType) from the docShell each time PerformanceNavigation.type is called? If yes, it's trivial enough, I can take it.

Comment 4

[Boris Zbarsky [:bzbarsky]]

That seems pretty plausible, yes.

Comment 5

[Andy Mercer]

Hi there. Just checking in to see if this is going to be fixed any time soon. I'm still having to use event.persisted for Firefox and performance.navigation.type == 2 for Chrome.

Comment 6

[Will Hawkins]

I am investigating this, but in Nightly I am having a difficult time replicating the problem. I will keep everyone posted.

Will

Update: I am able to reproduce this in Nightly, but not on a consistent basis. I will continue to investigate.

Comment 7

[Andrea Marchesini [:baku]]

Yesterday, I debugged this issue a bit. We actually update the nsDOMNavigationTiming correctly, set its type to TYPE_BACK_FORWARD. But it seems we are using a different object to retrieve the type from. I'll debug it a bit more today.

Comment 8

[Will Hawkins]

I am glad that we are seeing the same thing. I, too, see that we are setting the type correctly and then show that a different value is being generated by the call. To me, it looked like performance.navigation.type is returning

0: If the page has only been loaded once (ie, never refreshed)
1: If the page has ever been refreshed (and does not have contain anything that would invalidate it from the bfcache)

I'm glad that we are seeing the same behavior. I, too, believe that we are retrieving from the wrong object and will continue to debug.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Ah, I bet we get into nsDocShell::InternalLoad for the history navigation, call nsDocShell::MaybeInitTiming and create a new timing instance.

Then under nsDocShell::OnStateChange (called from nsDocShell::BeginRestore when it adds things to the loadgroup) we call nsDOMNavigationTiming::NotifyFetchStart on it, which sets its navigation type to TYPE_BACK_FORWARD.

But then I expect we just pull out our old page, do not create a new PerformanceMainThread, and don't propagate our new timing instance to the existing PerformanceMainThread object. At least I see nothing that sets PerformanceMainThread::mDOMTiming other than the PerformanceMainThread constructor, right?

So per spec, what should happen here in the bfcache case? Say we load page A, then page B, then restore A from bfcache. According to https://w3c.github.io/navigation-timing/#sec-PerformanceNavigationTiming most of the attributes refer to the "current document" state, so I would expect them to show the same values after the restore as they did before we started loading B. But type is explicitly tied to browsing context state, not the document. That's definitely not what any browser implements; I filed https://github.com/w3c/navigation-timing/issues/114 on that... It might be worth following up on the spec end of this before making changes.

If we do want to make changes, then I expect just updating the type value in RestoreFromHistory sometime after we do the mContentViewer.swap(viewer) it (that is, once the page being restored becomes the "current" page) would make sense.

Comment 10

[Will Hawkins]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #9)

Ah, I bet we get into nsDocShell::InternalLoad for the history navigation, call nsDocShell::MaybeInitTiming and create a new timing instance.

Then under nsDocShell::OnStateChange (called from nsDocShell::BeginRestore when it adds things to the loadgroup) we call nsDOMNavigationTiming::NotifyFetchStart on it, which sets its navigation type to TYPE_BACK_FORWARD.

But then I expect we just pull out our old page, do not create a new PerformanceMainThread, and don't propagate our new timing instance to the existing PerformanceMainThread object. At least I see nothing that sets PerformanceMainThread::mDOMTiming other than the PerformanceMainThread constructor, right?

So per spec, what should happen here in the bfcache case? Say we load page A, then page B, then restore A from bfcache. According to https://w3c.github.io/navigation-timing/#sec-PerformanceNavigationTiming most of the attributes refer to the "current document" state, so I would expect them to show the same values after the restore as they did before we started loading B. But type is explicitly tied to browsing context state, not the document. That's definitely not what any browser implements; I filed https://github.com/w3c/navigation-timing/issues/114 on that... It might be worth following up on the spec end of this before making changes.

If we do want to make changes, then I expect just updating the type value in RestoreFromHistory sometime after we do the mContentViewer.swap(viewer) it (that is, once the page being restored becomes the "current" page) would make sense.

I have a pending fix that I am testing that follows this paradigm. I will test further later tonight (EDT) and post as soon as I think it is ready.

Comment 11

[Will Hawkins]

Notwithstanding the ongoing discussion around the interpretation of the spec, here is the reason for the incorrect behavior:

The performance object is per (inner) window. When a document is restored from session history, the window is restored which means that the performance object is restored, too. Therefore, when a user invokes performance.navigation.type after navigating back/forward to a document that is restored from the session history, the result is the value of that attribute as it existed before the user navigated away from that page originally.

I have a fix that I am cleaning up and will post tomorrow morning. It is tested and seems to do the right thing.

Comment 12

[Andrea Marchesini [:baku]]

I'm happy to review your code as soon as it's ready!

Comment 13

[Will Hawkins]

Attachment: Bug 1459711: Ensure that navigation.performance.type is 2 when restoring a document from the session history.

Comment 14

[Will Hawkins]

Thanks to Boris for reviewing and approving the change. I am landing this now!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/46208314f5af
Ensure that navigation.performance.type is 2 when restoring a document from the session history. r=bzbarsky

Comment 16

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/46208314f5af