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

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Beth Rennie [:beth] (she/her)]

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.

Comment 1

[:Gijs (out for now; he/him)]

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

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 3

[Rares Doghi, Desktop QA]

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.

Comment 4

[:Gijs (out for now; he/him)]

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?

Comment 5

[:Gijs (out for now; he/him)]

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.

Comment 6

[Brendan Dahl [:bdahl]]

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.

Comment 7

[Brian Grinstead [:bgrins]]

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.

Comment 8

[Brian Grinstead [:bgrins]]

A couple more notes:

• if I move from aboutSessionRestore.xhtml -> aboutSessionRestore.html (along with updating redirector and changing the markup to adapt) the button does remain active after refreshing as you'd expect since we move back off of prototype cache parser, etc.
• There's some direct querying for this input node in session restore files like https://searchfox.org/mozilla-central/rev/325c1a707819602feff736f129cb36055ba6d94f/toolkit/components/sessionstore/SessionStoreUtils.cpp#480-483.

Comment 9

[:Gijs (out for now; he/him)]

[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.

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Brendan is this something you can take on?

Comment 11

[Brendan Dahl [:bdahl]]

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

Brendan is this something you can take on?

Yeah, looking into it now.

Comment 12

[Brendan Dahl [:bdahl]]

Attachment: Bug 1575620 - Fix refreshing session restore when using prototype cache. r=smaug

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.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5e9663384022
Fix refreshing session restore when using prototype cache. r=smaug

Comment 14

[Stefan Hindli [:stefan_hindli]]

Backed out changeset 5e9663384022 (Bug 1575620) for causing bustages in builds/worker/workspace/build/src/obj-firefox/dist/include/nsIContent.h CLOSED TREE

https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&fromchange=2e47d97d7b2ee934c55f2140addee6cc4f40a831&selectedJob=265407311

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=265407311&repo=autoland&lineNumber=13462

Comment 15

[Pulsebot]

Backout by [e-mail]:
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

Comment 16

[Brendan Dahl [:bdahl]]

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

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e2dc5bc7d6d1
Fix refreshing session restore when using prototype cache. r=smaug

Comment 18

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/e2dc5bc7d6d1

Comment 19

[Julien Cristau [:jcristau]]

Should we uplift this to beta70?

Comment 20

[Brendan Dahl [: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

2. open the devtools console
3. run $("#sessionData").value = SessionStore.getBrowserState() in devtools
4. reload the page
5. 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:

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 22

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/releases/mozilla-beta/rev/97141a6413d8

Comment 23

[Alexandru Trif, Desktop Test Engineering [:atrif]]

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.