Use Ci.nsICookieService.ACCEPT_SESSION instead of Services.cookies.ACCEPT_SESSION in SessionSaver.jsm

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: pauljt, Assigned: ttaubert)

Tracking

unspecified
Firefox 49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 wontfix, firefox48 fixed, firefox49 fixed)

Details

Attachments

(1 attachment)

As far as I can tell, https://bugzilla.mozilla.org/show_bug.cgi?id=529899#c28 was not acted upon, and the issue is still there. I've not  confirmed its a bug, but I didn't want the comment to get forgotten.

Tim, can you comment? Do you know if this is an issue or not?
Flags: needinfo?(ttaubert)
The issue is Services.cookies.ACCEPT_SESSION is undefined, I thought this must have been a recent change because I recall clearing sessionstore cookies on close working at one point. I went back to check the official builds when the bug landed and couldn't get it working, Services.cookies.ACCEPT_SESSION was undefined then too. Services.cookies is mapped to nsICookieManager2 which includes nsICookieManager but not nsICookieService which has ACCEPT_SESSION so I'm not sure how it could have ever worked.
Services.cookies.QueryInterface(Ci.nsICookieService) would work, if that was done once before trying to access ACCEPT_SESSION then the patch is fine. Looks like for some reason that happens (or happened?) when I was manually testing my patch.
Flags: needinfo?(ttaubert)
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Comment on attachment 8757765 [details] [diff] [review]
0001-Bug-1275415-Use-Ci.nsICookieService.ACCEPT_SESSION-i.patch

Hey mikedeboer! Feel like taking this small review on as part of your new SessionStore ownership duties?
Attachment #8757765 - Flags: review?(mconley) → review?(mdeboer)
Heh, I was already thinking about stealing the review! I will take this.
Comment on attachment 8757765 [details] [diff] [review]
0001-Bug-1275415-Use-Ci.nsICookieService.ACCEPT_SESSION-i.patch

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

LGTM!

::: browser/components/sessionstore/SessionSaver.jsm
@@ +213,5 @@
>  
>      // Clear all cookies on clean shutdown according to user preferences
>      if (RunState.isClosing) {
>        let expireCookies = Services.prefs.getIntPref("network.cookie.lifetimePolicy") ==
> +                            Services.cookies.QueryInterface(Ci.nsICookieService).ACCEPT_SESSION;

I don't know _why_ splinter decided to show this as a wrapped line, but in case it is: please land this with the correct indentation spanning two lines.
Attachment #8757765 - Flags: review?(mdeboer) → review+
Pushed by ttaubert@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/7759a4010be3
Use Ci.nsICookieService.ACCEPT_SESSION instead of Services.cookies.ACCEPT_SESSION in SessionSaver.jsm r=mikedeboer
https://hg.mozilla.org/mozilla-central/rev/7759a4010be3
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Comment on attachment 8757765 [details] [diff] [review]
0001-Bug-1275415-Use-Ci.nsICookieService.ACCEPT_SESSION-i.patch

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Bug 529899 will remain.
[Describe test coverage new/current, TreeHerder]: No tests.
[Risks and why]: Very low risk.
[String/UUID change made/needed]: None.

Don't know if it's too late for Beta to take this, but we should uplift to Aurora. This is a small and safe fix that should actually fix the issue from bug 529899.
Attachment #8757765 - Flags: approval-mozilla-beta?
Attachment #8757765 - Flags: approval-mozilla-aurora?
Hi Tim, is this a new regression in Fx47? Or has this been a known issue in 45/46? I think it is late as RC1 is done and at this point we are only taking fixes for issues that could cause dot releases. Do you think this fits that critical bar? Please let me know.
Flags: needinfo?(ttaubert)
In nsICookieManager2.idl can not we just add: interface nsICookieService ?
(In reply to Ritu Kothari (:ritu) from comment #10)
> Hi Tim, is this a new regression in Fx47? Or has this been a known issue in
> 45/46? I think it is late as RC1 is done and at this point we are only
> taking fixes for issues that could cause dot releases. Do you think this
> fits that critical bar? Please let me know.

This is an old issue, it won't cause a dot release if not fixed in 47. Uplifting to Aurora would still be nice :)
Flags: needinfo?(ttaubert)
Thanks Tim! It is too late to take this fix in Fx47 without disrupting the release quality/schedule. This is not a new/recent regression. Let's hope we can get the fix to go live with Fx48.
Comment on attachment 8757765 [details] [diff] [review]
0001-Bug-1275415-Use-Ci.nsICookieService.ACCEPT_SESSION-i.patch

Please see my previous comment.
Attachment #8757765 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 8757765 [details] [diff] [review]
0001-Bug-1275415-Use-Ci.nsICookieService.ACCEPT_SESSION-i.patch

Fix a long standing issue, taking it
Attachment #8757765 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.