Closed Bug 1575620 Opened 5 years ago Closed 5 years ago

Refreshing session restore page clears session data from the page, preventing session restore

Categories

(Firefox :: Session Restore, defect)

70 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 71
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox68 --- unaffected
firefox69 --- unaffected
firefox70 + verified
firefox71 --- verified

People

(Reporter: barret, Assigned: bdahl)

References

(Regression)

Details

(Keywords: dataloss, regression)

Attachments

(1 file)

STR

  1. Trigger session restore (e.g., by force killing the browser).
  2. Refresh the session restore page

AR
The "Restore Session" button becomes un-clickable. Restarting the browser takes you to about:newtab. The "Restore" option in the history menu is also disabled.

ER
I should be able to restore my session.

This occurs on Nightly but not Release. I don't have an installation of Beta or Dev Edition to compare to.

Based on this not happening on release, this must have regressed at some point...

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.

Hi guys, I was unable to get a mozregression because of the continuous restarts i needed to trigger the restore session but I did test manually a few builds and I think this is as close as we can get :

First known bad -

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1da5a37de0fd1695e96a9d5e7f59fbd84d8759b2&tochange=922be4adb708aee5ab59602b38fbb19f37c2de53

Using the builds from the archive I managed to narrow it down a bit to these nightly builds:

70.0a1 (2019-07-24) - last known good build
70.0a1 (2019-07-25) - first known bad build

I hope this helps and maybe theres a way to do a mozregression where you can restart and crash a build without triggering the updates or starting from different temp folders.
anyways hope this helps.

I found simpler steps to reproduce:

  1. open about:sessionrestore
  2. open the devtools console
  3. get the sessionData element by ID, and assign it a valid session restore string as a value (you can use the SessionStore API in the browser console to get one, or just crash a browser a few times yourself properly first, and then copy it from the element's value)
  4. refresh

In good builds, after the refresh the value is still present and the default "restore" button becomes enabled.

In bad builds, it is not.

Regression window:

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=0052568cf7101c0856d1a48f5532f40b5ba63739&tochange=4f0e19e164f101f5af183785b2dfcfe5efe82fff

Brendan, can you please take a look?

Has Regression Range: --- → yes
Has STR: --- → yes
Flags: needinfo?(bdahl)
Regressed by: 1550801
Summary: Refreshing session restore page prevents session restore → Refreshing session restore page clears session data from the page, preventing session restore

YMMV, but in case it helps, it looks to me like we would not have used the document prototype cache for about:sessionrestore prior to the changes, because the channel's original URI would have been an about: URI, and IsChromeURI would return false, but it looks like now we will use the proto cache, because the page has system principal.

I'll be out of the office next week and won't be able to get to this today. Maybe Brian can take a look for me when he's back monday.

Flags: needinfo?(bdahl) → needinfo?(bgrinstead)

I can confirm the STR from Comment 4 with: $("#sessionData").value = SessionStore.getBrowserState() in the web console for about:sessionrestore.

It seems like the .value on that input used to be persisted from the reload and isn't anymore, which gets us into the early return at https://searchfox.org/mozilla-central/rev/325c1a707819602feff736f129cb36055ba6d94f/browser/components/sessionstore/content/aboutSessionRestore.js#67-73.

A couple more notes:

[Tracking Requested - why for this release]:
This was tripped by a subtle behaviour change in gecko and this might therefore affect other things than just sessionstore - but refreshing about:sessionrestore and losing data that way is bad enough to get this tracked, I suspect.

Brendan is this something you can take on?

Flags: needinfo?(bdahl)

(In reply to Liz Henry (:lizzard) from comment #10)

Brendan is this something you can take on?

Yeah, looking into it now.

Assignee: nobody → bdahl
Flags: needinfo?(bdahl)

The session restore page keeps its restore list within a text input field
so that the values are persisted even if the page is refreshed. When form
elements were loaded with the prototype cache we didn't call
DoneCreatingElement after creating the element, which means the form values
weren't restored.

The list of elements that require DoneCreatingElement and DoneAddingChildren
to be called was in three (now four) different places, so I moved them to
a central spot in nsIContent to share in all locations. This also highlighted
that the check for <output> nodes is missing from the XML content sink.

Pushed by bdahl@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5e9663384022 Fix refreshing session restore when using prototype cache. r=smaug
Backout by shindli@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0bf660cc1a0f Backed out changeset 5e9663384022 for causing bustages in builds/worker/workspace/build/src/obj-firefox/dist/include/nsIContent.h CLOSED TREE

Pushed up a bad ctrl+z. Will fix here...

Flags: needinfo?(bdahl)
Pushed by bdahl@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e2dc5bc7d6d1 Fix refreshing session restore when using prototype cache. r=smaug
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 71
Flags: needinfo?(bgrinstead)

Should we uplift this to beta70?

Flags: needinfo?(bdahl)

Comment on attachment 9090770 [details]
Bug 1575620 - Fix refreshing session restore when using prototype cache. r=smaug

Beta/Release Uplift Approval Request

  • User impact if declined: Potential to lose the option to restore tabs if the user reloads the session restore page.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: 1) open about:sessionrestore
  1. open the devtools console
  2. run $("#sessionData").value = SessionStore.getBrowserState() in devtools
  3. reload the page
  4. the restore button should still be active
  • List of other uplifts needed: None
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): This patch changes (fixes) how some elements were being handled in XHTML, which could have unknown side affects. I don't think this is likely though, since it now moves XHTML handling more inline with HTML handling.
  • String changes made/needed:
Flags: needinfo?(bdahl)
Attachment #9090770 - Flags: approval-mozilla-beta?
Flags: qe-verify+

Comment on attachment 9090770 [details]
Bug 1575620 - Fix refreshing session restore when using prototype cache. r=smaug

Avoid a new session store regression in 70.
OK for uplift for beta 7.

Attachment #9090770 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Hello,
Reproduced the issue with Firefox 70.0a1 (20190821215524) on Windows 10x64.
The issue is verified fixed with Firefox 71.0a1 (20190915214245) and Firefox 70.0b7 (20190915235853) on Windows 10x64, macOS 10.14 and Ubuntu 18.04.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: