Closed Bug 1480951 Opened Last year Closed Last year

Cleared sessionStorage is resurrected when reopening Firefox

Categories

(Firefox :: Session Restore, defect)

63 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 64
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- verified
firefox64 --- verified

People

(Reporter: calebdotmiller, Assigned: mconley)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/68.0.3440.84 Safari/537.36

Steps to reproduce:

- Ensure Firefox is set to "Restore previous session" on startup.
- Populate sessionStorage with one or more key/value pairs.
- Call sessionStorage.clear()
- Close FF.
- Relaunch FF.


Actual results:

Firefox resurrects the state of sessionStorage prior to calling sessionStorage.clear().


Expected results:

Firefox should have remembered that sessionStorage was deliberately cleared.

This is happening in FF versions 61 - 63.

-----------------------------------------------------

Minimal Repro Demo:

- Ensure FF is set to "Restore previous session" on startup.
- Navigate to https://s.codepen.io/MillerTime/debug/d0cf119d3ab082e8e0f153ebc636b10b
- Click "Log In"
- Click "Log Out"
- Close FF
- Relaunch FF

After "restoring" the page, it says you are logged in, when a logout had previously occurred.
This same behavior can also be triggered by closing only the tab, and then pressing Ctrl+Shift+T to reopen it.

Clicking "Log In", then "Log Out", then refreshing the page, the empty session is preserved correctly. Interestingly, the page refresh seems to fully flush the cleared sessionStorage. A relaunch of FF after the refresh preserves the empty sessionStorage.

The code is very simple and can be viewed here:
https://codepen.io/MillerTime/pen/d0cf119d3ab082e8e0f153ebc636b10b?editors=1010
regressed since 48.
Component: Untriaged → Session Restore
Keywords: regression
Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=303f10857c8e7a7de7fa095fbff7f1291cb6bf6e&tochange=8e697c3542ffbe810d6ecc9912a510216471e99b

Regressed by: 8e697c3542ff	Gabor Krizsanits — Bug 1262661 - part2: optimization on session storage and history messages. r=mconley
Blocks: 1262661
Status: UNCONFIRMED → NEW
Ever confirmed: true
Mike, unfortunately Gabor is no longer around to help out here, but since you reviewed that patch... can you take a look, perhaps?
Flags: needinfo?(mconley)
Yeah, I'll take a look at this, thanks.
Assignee: nobody → mconley
Flags: needinfo?(mconley)
Comment on attachment 9006654 [details]
Bug 1480951 - Make SessionStore respect sessionStorage.clear(). r?mikedeboer

Mike de Boer [:mikedeboer] has approved the revision.
Attachment #9006654 - Flags: review+
Comment on attachment 9006656 [details]
Bug 1480951 - Regression test. r?mikedeboer

Mike de Boer [:mikedeboer] has approved the revision.
Attachment #9006656 - Flags: review+
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30e975786e48
Make SessionStore respect sessionStorage.clear(). r=mikedeboer
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a6bad0f57ea
Regression test. r=mikedeboer
https://hg.mozilla.org/mozilla-central/rev/30e975786e48
https://hg.mozilla.org/mozilla-central/rev/6a6bad0f57ea
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Awesome, thank you all!
Mike, do you intend to request an uplift to beta for this regression?
Flags: needinfo?(mconley)
Comment on attachment 9006654 [details]
Bug 1480951 - Make SessionStore respect sessionStorage.clear(). r?mikedeboer

Approval Request Comment
[Feature/Bug causing the regression]:

Bug 1262661

[User impact if declined]:

Sites that use window.sessionStorage.clear() to clear stored data will have that storage unexpectedly restored if the tab is ever restored from SessionStore.

[Is this code covered by automated tests?]:

Yes - it's in the other patch in this bug.

[Has the fix been verified in Nightly?]:

Yes, I've just manually verified this.

[Needs manual test from QE? If yes, steps to reproduce]: 

STR are in comment 0 of this bug.

[List of other uplifts needed for the feature/fix]:

Just the other patch in this bug, which is the regression test.

[Is the change risky?]:

No.

[Why is the change risky/not risky?]:

We forgot the clear() case when we were attempting to compact the size of our IPC messages. This adds support for it.

[String changes made/needed]:

None.
Flags: needinfo?(mconley)
Attachment #9006654 - Flags: approval-mozilla-beta?
Comment on attachment 9006656 [details]
Bug 1480951 - Regression test. r?mikedeboer

Approval Request Comment

See above.
Attachment #9006656 - Flags: approval-mozilla-beta?
Comment on attachment 9006656 [details]
Bug 1480951 - Regression test. r?mikedeboer

Thanks, uplift approved for 63 beta 5
Attachment #9006656 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9006654 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
Flags: in-testsuite+
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:63.0) Gecko/20100101 Firefox/63.0
Build ID: 20180910132416

Verified as fixed on the latest Nightly build (64.0a1) and on the latest Beta build (63b5).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1482983
You need to log in before you can comment on or make changes to this bug.