Closed
Bug 1275415
Opened 8 years ago
Closed 8 years ago
Use Ci.nsICookieService.ACCEPT_SESSION instead of Services.cookies.ACCEPT_SESSION in SessionSaver.jsm
Categories
(Firefox :: Session Restore, defect)
Firefox
Session Restore
Tracking
()
RESOLVED
FIXED
Firefox 49
People
(Reporter: pauljt, Assigned: ttaubert)
References
Details
Attachments
(1 file)
1.52 KB,
patch
|
mikedeboer
:
review+
Sylvestre
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
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?
Reporter | ||
Updated•8 years ago
|
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.
Assignee | ||
Comment 2•8 years ago
|
||
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 | ||
Updated•8 years ago
|
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
status-firefox47:
--- → affected
status-firefox48:
--- → affected
status-firefox49:
--- → affected
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8757765 -
Flags: review?(mconley)
Comment 4•8 years ago
|
||
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)
Comment 5•8 years ago
|
||
Heh, I was already thinking about stealing the review! I will take this.
Comment 6•8 years ago
|
||
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
Comment 8•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7759a4010be3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Assignee | ||
Comment 9•8 years ago
|
||
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)
Comment 11•8 years ago
|
||
In nsICookieManager2.idl can not we just add: interface nsICookieService ?
Assignee | ||
Comment 12•8 years ago
|
||
(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 15•8 years ago
|
||
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+
Assignee | ||
Comment 16•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/7f8d31a531ca
You need to log in
before you can comment on or make changes to this bug.
Description
•