1230563 - Cookie permissions should override the CookiesLifetimePolicy

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Cookie permissions should override the CookiesLifetimePolicy

Comment 2

[Nika Layzell [:nika] (ni? for response)]

See bug 536509 comment 75

It appears as though the reason for this problem is that the cookies permissions were checked after cookiesLifetimePolicy (which means that they overrode each other in the wrong order.

This patch is untested - and mostly exists as a record.

Comment 3

[Stefan Plewako [:stef]]

What do we need to move this forward?

Comment 4

[Nika Layzell [:nika] (ni? for response)]

I think it's just confirming that it actually works, and writing the test - sorry for the delay, I've been busy. I'll try to take a shot at finishing it off once I land in orlando.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Try run looks pretty green, but I'm not sure what the best way to write a test for this would be.

Comment 6

[Nika Layzell [:nika] (ni? for response)]

(try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a51129b7580d)

Comment 7

[(no longer active)]

[Tracking Requested - why for this release]: This is a regression from bug 536509.

Comment 8

[(no longer active)]

(In reply to Michael Layzell [:mystor] from comment #5)
> Try run looks pretty green, but I'm not sure what the best way to write a
> test for this would be.

I'm not sure what actually causes the localStorage to be cleared when we restart, I expect that is one of the notifications that DOMStorageManager::Observe() or DOMStorageObserver::Observe() handles.

So in order to test this, you need to start by setting the network.cookie.lifetimePolicy pref, and then get access to the right observer based on the answer to the above question and emit the appropriate notification, and then check whether localStorage is empty afterwards.

Comment 9

[Nika Layzell [:nika] (ni? for response)]

Smaug suggested that you might know how persistence for localstorage works, which will be very useful for writing a test for this patch :)

Basically the question is how we'd go about testing whether or not localStorage data is persisted across sessions from within a test. I have been looking and it seems to me like the change is that Persist()'s return value changes here: https://dxr.mozilla.org/mozilla-central/source/dom/storage/DOMStorageCache.cpp#147 and that seems to control whether the data is saved anywhere. I haven't found any place where stuff is explicitly cleared.

Comment 10

[(no longer active)]

Search for "AsyncClear" in DOMStorageObserver.cpp, there's a couple of places where we do that.

Comment 11

[Nika Layzell [:nika] (ni? for response)]

(In reply to :Ehsan Akhgari from comment #10)
> Search for "AsyncClear" in DOMStorageObserver.cpp, there's a couple of
> places where we do that.

Async clear has this documentation:
  // Called when the whole storage is cleared by the DOM API, schedules delete of the scope

This isn't very likely to be called by the session storage stuff. I'm pretty sure from my investigation that the way this is done by simply not persisting changes to the DB when the session-only setting is set. I'm not sure if it's possible to simulate this behavior without actually restarting the browser.

Comment 12

[(no longer active)]

OK, fair enough!  We can't simulate a browser restart in our test suites unfortunately.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Regression, tracking.
Michael, we are in the second half of the beta cycle. Do you have an eta for this bug?
Thanks

Comment 14

[Honza Bambas (:mayhemer)]

(In reply to Michael Layzell [:mystor] from comment #9)
> Smaug suggested that you might know how persistence for localstorage works,
> which will be very useful for writing a test for this patch :)
> 
> Basically the question is how we'd go about testing whether or not
> localStorage data is persisted across sessions from within a test. I have
> been looking and it seems to me like the change is that Persist()'s return
> value changes here:
> https://dxr.mozilla.org/mozilla-central/source/dom/storage/DOMStorageCache.
> cpp#147 and that seems to control whether the data is saved anywhere. I
> haven't found any place where stuff is explicitly cleared.

I don't much understand the question.  If you want to test inter-session persistence, you can be inspired with [1].  The localStorageFlushAndReload() helper function will fire the given callback after the cache is flushed to disk and reloaded again.  This applies only to localStorage and its persistent data set.  However, see [2], it could be altered to clear also any private and session-only data (the mData member of DOMStorageCache has 3 items: kDefaultSet, kPrivateSet, kSessionSet.)

Does it help?

[1] http://hg.mozilla.org/mozilla-central/annotate/29258f59e545/dom/tests/mochitest/localstorage/test_bug600307-DBOps.html#l41
[2] http://hg.mozilla.org/mozilla-central/annotate/29258f59e545/dom/storage/DOMStorageCache.cpp#l600

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

It's not ideal that we shipped one version with this bug, but since i am a week away from entering RC mode, I am leaning towards wontfixing this.

Comment 16

[Andrew Overholt [:overholt]]

Should we move forward without a test?

Comment 17

[(no longer active)]

As I said in comment 12, yes.

Comment 18

[Nika Layzell [:nika] (ni? for response)]

Attachment: Test for corrected cookie permission behavior

Hey, I'm so sorry in the delay on getting to this. I didn't dig through my holiday bugmail backlog at all until today.

I've attached a potential mechanism for testing this behavior - unfortunately it extends the webidl interface for DOMStorage by adding a [ChromeOnly] method, so I am not sure it's a good idea.

I figured I'd post it at least as a potential mechanism for testing this patch, to make it safer to merge into beta/aurora.

Sorry again for the delay!

Comment 19

[(no longer active)]

Comment on attachment 8705766 [details] [diff] [review]
Test for corrected cookie permission behavior

Review of attachment 8705766 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this is great!

::: dom/tests/mochitest/localstorage/frameLocalStorageSessionOnly.html
@@ +1,5 @@
> +<!doctype html>
> +<html>
> +  <body>
> +    <script>
> +      debugger;

Why?

::: dom/tests/mochitest/localstorage/test_localStorageSessionPrefOverride.html
@@ +1,1 @@
> +<html xmlns="http://www.w3.org/1999/xhtml">

Please drop the @xmlns.  XHTML is dead.  :-)

Comment 20

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8695895 [details] [diff] [review]
Cookie permissions should override the CookiesLifetimePolicy

I just realized that I never asked anyone to review this :S

Comment 21

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Test for corrected cookie permission behavior

Removed the accidental debugger statement and xmlns decl.

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 46 assuming this also affects the current nightly.  Recent regression. 
Once this lands for 46 and we do some testing, please request uplift to 45 (whether it is aurora, or beta by that time). Thanks!

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c96da719d455
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffac58123eb0

Comment 24

[Nika Layzell [:nika] (ni? for response)]

Oops, somehow this slipped under my radar for an entire week!

Comment 25

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8695895 [details] [diff] [review]
Cookie permissions should override the CookiesLifetimePolicy

Approval Request Comment
[Feature/regressing bug #]: bug  536509
[User impact if declined]: Explicit cookie allow permissions will not override the session only lifetime policy for localstorage and indexeddb. Cookies are unaffected.
[Describe test coverage new/current, TreeHerder]: Tests are located in the other patch, and confirm the correct behavior in localStorage.
[Risks and why]: Should be fairly low risk, as it is a minor change with tests.
[String/UUID change made/needed]: N/A

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c96da719d455
https://hg.mozilla.org/mozilla-central/rev/ffac58123eb0

Comment 27

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8695895 [details] [diff] [review]
Cookie permissions should override the CookiesLifetimePolicy

Fix a regression, has test, taking it.

Comment 28

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8705783 [details] [diff] [review]
Part 2: Test for corrected cookie permission behavior

[Triage Comment]
let's take the test too

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3f9c48e199bf
https://hg.mozilla.org/releases/mozilla-aurora/rev/0991cdfecaee