[e10s] Form data are lost when restore session

RESOLVED FIXED in Firefox 45



Session Restore
2 years ago
a year ago


(Reporter: Alice0775 White, Assigned: billm)


({dataloss, regression})

45 Branch
Firefox 46
dataloss, regression
Dependency tree / graph

Firefox Tracking Flags

(e10sm8+, firefox44 unaffected, firefox45+ verified, firefox46+ fixed)



(1 attachment)



2 years ago
[Tracking Requested - why for this release]: Session data lost due to regression

Steps to reproduce:

1. Open this bug page and logged in
2. Type text in Comment field
3. Exit Browser and restart
4. Click "Restore Previous Session"

Actual results:
Form data are not restored.

Expected results:
Form data should be restored.

Regression window:

Regressed by: Bug 1177310

Need info ( because Dev are in PTO )
Flags: needinfo?(wmccloskey)
Flags: needinfo?(mconley)
Flags: needinfo?(dteller)


2 years ago
See Also: → bug 1235368


2 years ago
tracking-e10s: --- → ?
Summary: Form data are lost when restore session → [e10s] Form data are lost when restore session
Mike's out, so I guess I'll take this.
Assignee: nobody → wmccloskey
Flags: needinfo?(wmccloskey)
Flags: needinfo?(mconley)
Flags: needinfo?(dteller)
This is pretty confusing. The problem seems to be here:
If I comment that line out, then everything seems to work as expected.

What's confusing is that the code seems to be working as intended. If we're shutting down and we're not restoring on startup, then we don't save data for HTTPS sites (according to the browser.sessionstore.privacy_level_deferred pref, which defaults to 1).

It's hard for me to tell if this "feature" ever worked as intended (before Mike's change, that is). The code was added in bug 588482, but there are no tests for it. I haven't tracked all the way through the history, and the state management here is really tricky. In any case, we now seem to be getting true for Services.startup.shuttingDown when we're collecting the data right before we write it out at shutdown. Before we weren't.

Tim, what do you think we should do here? I'd like to do some more investigation, but something is really wrong here. I'm thinking maybe we should just get rid of this deferred pref, but I still don't understand it well enough to be sure.
Flags: needinfo?(ttaubert)
tracking-firefox46: ? → +
Created attachment 8702712 [details] [diff] [review]
remove deferred pref

I looked through the code more and I don't think it ever worked. If you look here:
you can see that quit-application-requested does the final collection of session data. Only later, in quit-application-granted, do we set _loadState to STATE_QUITTING. And the _checkPrivacyLevel code (which is run when collecting data) checks for STATE_QUITTING:

I tested a Firefox 4 nightly from about a month after bug 588482 landed and it doesn't seem to be respecting the deferred pref. I wasn't able to build the code, so I couldn't debug any further.

So, as far as I can tell, the sessionstore.privacy_level_deferred pref has never been used until Mike's patch. I think we should remove it. That's what this patch does.
Flags: needinfo?(ttaubert)
Attachment #8702712 - Flags: review?(ttaubert)
Comment on attachment 8702712 [details] [diff] [review]
remove deferred pref

Review of attachment 8702712 [details] [diff] [review]:

Those code paths are complex indeed, if not terrible. I wish we had shutdown/restart test suites so we could have had tests.

Removing the pref seems fine, it gets rid of a lot of complexity and no one noticed since Firefox 4 it seems. This was meant to provide a higher level of privacy as we were now saving things to disk by default, instead of only when the user opted into restoring their session automatically. Although with the web moving to HTTPS by default trying to differentiate between sensitive and non-sensitive pages only by the protocol used to retrieve them doesn't seem timely anymore.

::: browser/components/sessionstore/PrivacyLevel.jsm
@@ +40,3 @@
>     * @return {bool} Whether we can save data for the specified site.
>     */
>    canSave: function ({isHttps, isPinned}) {

Forgot to remove isPinned here.
Attachment #8702712 - Flags: review?(ttaubert) → review+
tracking-e10s: ? → m8+

Comment 6

2 years ago
Last Resolved: 2 years ago
status-firefox46: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Alice0775, when you have a moment, can you confirm that this is fixed in tomorrow's Nightly?
Flags: needinfo?(alice0775)

Comment 8

2 years ago
I cannot reproduce on the latest Nightly.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0 ID:20160107030235
tracking-firefox45: ? → ---
Flags: needinfo?(alice0775)
[Tracking Requested - why for this release]:

Potential dataloss on application shutdown.

billm, do you feel like this is safe for requesting uplift?
tracking-firefox45: --- → ?
Flags: needinfo?(wmccloskey)
Comment on attachment 8702712 [details] [diff] [review]
remove deferred pref

Approval Request Comment
[Feature/regressing bug #]: bug 1177310
[User impact if declined]: form data is lost in session restore for HTTPS sites on shutdown
[Describe test coverage new/current, TreeHerder]: passes on m-c, but no new test coverage
[Risks and why]: We're disabling some behavior that we're pretty sure has been broken since Firefox 4. But there's a risk that it was somehow still being used. Seems very unlikely though.
[String/UUID change made/needed]: none
Flags: needinfo?(wmccloskey)
Attachment #8702712 - Flags: approval-mozilla-aurora?
tracking-firefox45: ? → +
Comment on attachment 8702712 [details] [diff] [review]
remove deferred pref

Fix a painful regression, taking it.
Attachment #8702712 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 12

2 years ago
status-firefox45: affected → fixed

Comment 13

2 years ago
I have reproduced this bug with Firefox nightly 46.0a1(build id:20151228030213)on 
windows 7(64 bit)

I have verified as fixed this bug with Firefox aurora 45.0a2(build id:20160114004007)
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0

status-firefox45: fixed → verified

Comment 15

2 years ago


Test Successful

Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel          beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Verified and working

Actual Results: 
As expected
You need to log in before you can comment on or make changes to this bug.