1480951 - Cleared sessionStorage is resurrected when reopening Firefox

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[calebdotmiller]

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

Comment 1

[Alice0775 White]

regressed since 48.

Comment 2

[Alice0775 White]

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

Comment 3

[Mike de Boer [:mikedeboer]]

Mike, unfortunately Gabor is no longer around to help out here, but since you reviewed that patch... can you take a look, perhaps?

Comment 4

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

Yeah, I'll take a look at this, thanks.

Comment 5

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

Attachment: Bug 1480951 - Make SessionStore respect sessionStorage.clear(). r?mikedeboer

Comment 6

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

Attachment: Bug 1480951 - Regression test. r?mikedeboer

Depends on D5066

Comment 7

[Mike de Boer [:mikedeboer]]

Comment on attachment 9006654 [details]
Bug 1480951 - Make SessionStore respect sessionStorage.clear(). r?mikedeboer

Mike de Boer [:mikedeboer] has approved the revision.

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 9006656 [details]
Bug 1480951 - Regression test. r?mikedeboer

Mike de Boer [:mikedeboer] has approved the revision.

Comment 9

[Pulsebot]

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

Comment 10

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/30e975786e48
https://hg.mozilla.org/mozilla-central/rev/6a6bad0f57ea

Comment 11

[calebdotmiller]

Awesome, thank you all!

Comment 12

[Pascal Chevrel:pascalc (PTO until April 26)]

Mike, do you intend to request an uplift to beta for this regression?

Comment 13

[Mike Conley (: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.

Comment 14

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

Comment on attachment 9006656 [details]
Bug 1480951 - Regression test. r?mikedeboer

Approval Request Comment

See above.

Comment 15

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9006656 [details]
Bug 1480951 - Regression test. r?mikedeboer

Thanks, uplift approved for 63 beta 5

Comment 16

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/releases/mozilla-beta/rev/736735078dd5
https://hg.mozilla.org/releases/mozilla-beta/rev/586d84ba47a6

Comment 17

[Cristian Craciun]

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