1588119 - [Fission] The address bar displays the previously accessed webpage's address instead of the current's webpage url after session restore

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect, P2)

Description

[Gabi Cheta [:Gabi] Desktop QA]

Attachment: aboutpage.png

• [Affected versions]:
  Fx 71.0a1

• [Affected platforms]:
  macOS 10.14
  Windows 10x64
  Ubuntu 16.4 x64

• [Steps to reproduce]:

1. Launch Firefox
2. Navigate to about:config and set pref fission.autostart to true
3. Restart the browser (CTRL+ALT+R in the browser console)
4. Go to any web page
5. From web page in the same tab, go to cnn.com
6. Restart the browser again
7. Go to the open ccn.com tab

• [Expected result]:
  cnn.com page is displayed properly

• [Actual result]:
  The address bar displays the previously accessed wepage's address instead of the current's webpage url after browser restart

Comment 1

[Neha Kochar [:neha]]

This may also be related to the frameloader rebuilding which is always enabled for fission windows.

Comment 2

[u608768]

I can reproduce this. Flipping fission.rebuild_frameloaders_on_remoteness_change doesn't appear to help. The location object on the restored document is correct (i.e., it's still the location that was loaded before we restarted), so it looks like we're only incorrectly setting the URI that is displayed (which is set here).

Comment 3

[u608768]

It looks like we send a "SessionStore:restoreTabContentStarted" after restarting. The userTypedValue stored on the browser is cleared during the load, so we fallback on the value stored in tabData (https://searchfox.org/mozilla-central/rev/efdf9bb55789ea782ae3a431bda6be74a87b041e/browser/components/sessionstore/SessionStore.jsm#1099). For some reason, the value in tabData is stale, and so we set the displayed URI back to what it was before the latest navigation.

Comment 4

[u608768]

Okay, so while loading, we start the "async navigate and restore process" [1]. This calls TabState.clone [2], which eventually calls TabStateInternal._collectBaseTabData, which attempts to populate tabData from the TabStateCache [3]. The userTypedValue of the cached tab data is the URL that was typed in step 4 of comment #0 (even though something was typed and submitted after this). The condition in [4] isn't met, and so we don't update the value in tabData with the new URL.

I'm a little confused because even with Fission disabled, the cached userTypedValue is the old URL (so where do we get the "latest" value in the non-Fission case?). I'm also unsure if we want to actually update TabStateInternal._collectBaseTabData to cache the new value instead, although this seems like the correct thing to do.

[1] https://searchfox.org/mozilla-central/rev/efdf9bb55789ea782ae3a431bda6be74a87b041e/browser/components/sessionstore/SessionStore.jsm#3694
[2] https://searchfox.org/mozilla-central/rev/efdf9bb55789ea782ae3a431bda6be74a87b041e/browser/components/sessionstore/SessionStore.jsm#3720
[3] https://searchfox.org/mozilla-central/rev/efdf9bb55789ea782ae3a431bda6be74a87b041e/browser/components/sessionstore/TabState.jsm#122
[4] https://searchfox.org/mozilla-central/rev/efdf9bb55789ea782ae3a431bda6be74a87b041e/browser/components/sessionstore/TabState.jsm#138

Comment 5

[u608768]

(In reply to :kashav from comment #4)

where do we get the "latest" value in the non-Fission case?

It looks like it's being set when handling the "SessionStore:restoreHistoryComplete" message (sent in [1]). Unless I'm mistaken, we aren't seeing these when Fission is enabled. This might be something that'll be solved by bug 1507287.

[1] https://searchfox.org/mozilla-central/rev/ea63a0888d406fae720cf24f4727d87569a8cab5/browser/components/sessionstore/ContentSessionStore.jsm#660,665

Comment 6

[u608768]

(In reply to :kashav from comment #5)

This might be something that'll be solved by bug 1507287.

This still happens with the patch in that bug (I'm unaware of how complete it is right now). I'm also seeing this happen intermittently with Fission disabled (in both Release and Nightly). This is most likely happening because we don't update the cached userTypedValue (do we want to update the value to be the new URL after a load is done, or just clear the old value and save the new URL later?).

Comment 7

[u608768]

:smaug, I was told you may be able to provide more context or a path forward for this.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

I'm not familiar with this stuff.
(especially if this happens without Fission too)

Comment 9

[Alphan Chen [:alchen]]

By the STR, I cannot reproduce the symptom with my local build based on today's codebase with the patch of bug 1507287.

(In reply to :kashav from comment #2)

I can reproduce this. Flipping fission.rebuild_frameloaders_on_remoteness_change doesn't appear to help. The location object on the restored document is correct (i.e., it's still the location that was loaded before we restarted), so it looks like we're only incorrectly setting the URI that is displayed (which is set here).

I also met one thing which is related to the perf "fission.rebuild_frameloaders_on_remoteness_change".
Last Friday, I found two extra mochitest failures when set the pref as true by default in Bug 1583614. But they are gone now.

[mochitest failures I met]
browser/components/sessionstore/test/browser_closed_objects_changed_notifications_tabs.js
FAIL The expected number of notifications was received. - Got 4, expected 5
browser/components/sessionstore/test/browser_tab_label_during_restore.js
FAIL observed tab label changes - Got ["about:robots","New Tab","Gort! Klaatu barada nikto!"], expected ["about:robots","Gort! Klaatu barada nikto!"]

Comment 10

[u608768]

(In reply to Alphan Chen [:alchen] from comment #9)

By the STR, I cannot reproduce the symptom with my local build based on today's codebase with the patch of bug 1507287.

I also can't reproduce this with the latest patch from bug 1507287. Kind of curious as to what changed to make this not happen anymore.

We should keep this open until that bug lands and then re-verify.

Comment 11

[u608768]

(In reply to :kashav from comment #10)

Kind of curious as to what changed to make this not happen anymore.

We spoke about this on IRC. The latest changes to the patch in bug 1507287 fixed a timing issue in browser_447951.js caused by an async call from the parent to child. Interestingly enough, bug 447951 was also for an address-bar-displays-previous-url-after-restore thing.

Comment 12

[Alphan Chen [:alchen]]

Update the finding.

(In reply to :kashav from comment #6)

(In reply to :kashav from comment #5)

This might be something that'll be solved by bug 1507287.

This still happens with the patch in that bug (I'm unaware of how complete it is right now).

The comment is not totally correct.
The patch can solve the problem if we pref on "fission.sessionHistoryInParent".
The problem is not solved by the latest change.

(In reply to :kashav from comment #10)

(In reply to Alphan Chen [:alchen] from comment #9)

By the STR, I cannot reproduce the symptom with my local build based on today's codebase with the patch of bug 1507287.

The reason why the patch of Dec-11 works when kashav tried is the patch also includes the pref enabled.

[Conclusion]
The problem can be reproduced consistently without the patch of bug 1507287.
With my patch, it can be fixed if we enable sessionHistory in the parent.
But if we disable sessionHistory in the parent, we will still meet the problem.

Comment 13

[Chris Peterson [:cpeterson]]

Tracking for Fission Nightly (M6)

Session restore is not being fixed in M5. Kashav said he also sees this bug without Fission.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Hi Gabi,

Are you still able to reproduce this? We think the patch for bug 1615302 might have fixed this.

Comment 15

[Gabi Cheta [:Gabi] Desktop QA]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #14)

Hi Gabi,

Are you still able to reproduce this? We think the patch for bug 1615302 might have fixed this.

Hello,

Verified with Nightly 76.0a1 (2020-03-25) on Windows 10 x64 and the issue is still reproducing.
On Firefox Beta 75.0b8 the preference 'fission.autostart' is set to "false" and locked so I'm not able to reproduce the issue.

Comment 16

[Chris Peterson [:cpeterson]]

M6c. This is a UI issue, so not urgent but would be nice-to-have for Fission Nightly.

Moving to Session Restore component because this is a session store bug, not a DOM: Navigation bug.

Comment 17

[Neha Kochar [:neha]]

(In reply to Chris Peterson [:cpeterson] from comment #16)

M6c. This is a UI issue, so not urgent but would be nice-to-have for Fission Nightly.

Correction: this is not just a UI issue, it's an actual session restore issue.

Comment 18

[u608768]

(In reply to :kashav from comment #6)

This is most likely happening because we don't update the cached userTypedValue (do we want to update the value to be the new URL after a load is done, or just clear the old value and save the new URL later?).

We ended up doing the latter for bug 1627420. It appears to have fixed this bug as well.

Gabi, can you still reproduce?

Comment 19

[Neha Kochar [:neha]]

I'm not able to reproduce this anymore.

Comment 20

[Neha Kochar [:neha]]

Doesn't reproduce anymore. Probably fixed as part of bug 1627420.