Closed Bug 1235379 Opened 9 years ago Closed 9 years ago

[e10s] Form data are lost when restore session

Categories

(Firefox :: Session Restore, defect)

45 Branch
defect
Not set
major

Tracking

()

RESOLVED FIXED
Firefox 46
Tracking Status
e10s m8+ ---
firefox44 --- unaffected
firefox45 + verified
firefox46 + fixed

People

(Reporter: alice0775, Assigned: billm)

References

Details

(Keywords: dataloss, regression)

Attachments

(1 file)

[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 ) mconley@mozilla.com, dteller@mozilla.com, wmccloskey@mozilla.com
Flags: needinfo?(wmccloskey)
Flags: needinfo?(mconley)
Flags: needinfo?(dteller)
See Also: → 1235368
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: 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.
Flags: needinfo?(ttaubert)
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.
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+
Status: NEW → RESOLVED
Closed: 9 years ago
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)
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
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?
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?
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+
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]
[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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: