1235379 - [e10s] Form data are lost when restore session

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Alice0775 White]

[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:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=45708ae19e28b92403549a6e42324f6cfdcb8f67&tochange=15ded62d213cb2e01feebcd21af52be74a535983

Regressed by: Bug 1177310


Need info ( because Dev are in PTO )
[e-mail],
[e-mail],
[e-mail]

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Mike's out, so I guess I'll take this.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This is pretty confusing. The problem seems to be here:
https://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/PrivacyLevel.jsm#44
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.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: remove deferred pref

I looked through the code more and I don't think it ever worked. If you look here:
http://hg.mozilla.org/mozilla-central/file/d8502fae9678/browser/components/sessionstore/src/nsSessionStore.js#l375
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:
http://hg.mozilla.org/mozilla-central/file/d8502fae9678/browser/components/sessionstore/src/nsSessionStore.js#l2974

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.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0cb7f423587d

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0cb7f423587d

Comment 7

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

Alice0775, when you have a moment, can you confirm that this is fixed in tomorrow's Nightly?

Comment 8

[Alice0775 White]

I cannot reproduce on the latest Nightly.

https://hg.mozilla.org/mozilla-central/rev/e0bcd16e1d4b99ba3e542149d0d41e0f60c54b5c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0 ID:20160107030235

Comment 9

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

[Tracking Requested - why for this release]:

Potential dataloss on application shutdown.

billm, do you feel like this is safe for requesting uplift?

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8702712 [details] [diff] [review]
remove deferred pref

Fix a painful regression, taking it.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d1d72c3be215

Comment 13

[Kazi Nuzhat Tasnem]

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


[testday-20160115]

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Thanks

Comment 15

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> VERIFIED

Comments:
Test Successful

Component: 
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