Closed
Bug 1270423
Opened 8 years ago
Closed 8 years ago
The nsICookieManager.remove() does not reference originAttributes at release build.
Categories
(Core :: Networking: Cookies, defect, P1)
Core
Networking: Cookies
Tracking
()
RESOLVED
FIXED
mozilla49
People
(Reporter: timhuang, Assigned: timhuang)
References
(Blocks 1 open bug)
Details
(Whiteboard: [oa][necko-active][userContextId])
Attachments
(1 file, 2 obsolete files)
2.17 KB,
patch
|
ehsan.akhgari
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
According to https://dxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.cpp#2390 , the NeckoOriginAttributes will not be set correctly at release build.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8749113 -
Flags: review?(ehsan)
Assignee | ||
Comment 2•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b2ea3ad034a
Comment 3•8 years ago
|
||
Comment on attachment 8749113 [details] [diff] [review] Fix the nsICookieManager.remove() to reference originAttributes correctly at release build. Review of attachment 8749113 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/cookie/nsCookieService.cpp @@ +2386,5 @@ > "nsICookieManagerRemoveDeprecated"); > } > > NeckoOriginAttributes attrs; > + if (!aOriginAttributes.isObject() || I think you should do: if (aArgc == 1 && (!aOriginAttributes.isObject() || !attrs.Init(aCx, aOriginAttributes))) { return NS_ERROR_INVALID_ARG; }
Assignee | ||
Comment 4•8 years ago
|
||
Baku, thanks for your review comment. This patch does what you said.
Attachment #8749149 -
Flags: review?(ehsan)
Assignee | ||
Updated•8 years ago
|
Attachment #8749113 -
Attachment is obsolete: true
Attachment #8749113 -
Flags: review?(ehsan)
Assignee | ||
Comment 5•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=804415f67e53
Updated•8 years ago
|
Whiteboard: [oa] → [oa][necko-active]
Assignee | ||
Comment 6•8 years ago
|
||
Fix treeherder failures of marionette framework unit tests.
Attachment #8749241 -
Flags: review?(ehsan)
Assignee | ||
Updated•8 years ago
|
Attachment #8749149 -
Attachment is obsolete: true
Attachment #8749149 -
Flags: review?(ehsan)
Assignee | ||
Comment 7•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=18f52123f39e
Comment 8•8 years ago
|
||
Will this need uplift?
Whiteboard: [oa][necko-active] → [oa][necko-active][userContextId]
Comment 9•8 years ago
|
||
(In reply to Tanvi Vyas [:tanvi] from comment #8) > Will this need uplift? According to mxr [1], no addon is using the OriginAttributes feature while calling cookies functions at this moment. (There is no addon calling nsICookieManager::remove() with the parameter aOriginAttributes.) So I think it's not necessary to uplift this bug since no addon will be broken by it. Baku, can you confirm my thought? [1] https://mxr.mozilla.org/addons/search?string=cookies.remove%28
Flags: needinfo?(amarchesini)
Comment 10•8 years ago
|
||
> [1] https://mxr.mozilla.org/addons/search?string=cookies.remove%28
This is correct. But it's also true that many addon developers could start using this new remove() param soon. This code will be in release in weeks. I prefer to have it in aurora.
Flags: needinfo?(amarchesini)
Comment 11•8 years ago
|
||
(In reply to Andrea Marchesini (:baku) from comment #10) > This is correct. But it's also true that many addon developers could start > using this new remove() param soon. This code will be in release in weeks. I > prefer to have it in aurora. Sure. Let's uplift it to aurora.
Updated•8 years ago
|
Priority: -- → P1
Updated•8 years ago
|
Attachment #8749241 -
Flags: review?(ehsan) → review+
Comment 13•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/002cfe847c66
Keywords: checkin-needed
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/002cfe847c66
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee | ||
Comment 15•8 years ago
|
||
Comment on attachment 8749241 [details] [diff] [review] Fix the nsICookieManager.remove() to reference originAttributes correctly at release build. Approval Request Comment [Feature/regressing bug #]: bug 1245184 [User impact if declined]: The nsICookieManager.remove() will not reference originAttributes at release build. [Risks and why]: This patch should be uplifted to allow remove() working properly. And this patch is simple, it should be no risk. [String/UUID change made/needed]: none
Attachment #8749241 -
Flags: approval-mozilla-aurora?
Comment 16•8 years ago
|
||
(In reply to Tim Huang[:timhuang] from comment #15) > [User impact if declined]: The nsICookieManager.remove() will not reference > originAttributes at release build. Annotation: This means if any addon developers manipulate cookies by using the new feature of nsICookieManager.remove(), they won't get their expected results (e.g. cookies not being removed).
Updated•8 years ago
|
status-firefox48:
--- → affected
tracking-firefox48:
--- → +
Comment 17•8 years ago
|
||
Comment on attachment 8749241 [details] [diff] [review] Fix the nsICookieManager.remove() to reference originAttributes correctly at release build. New feature that we want to work correctly for addon devs. Let's uplift this. Is there a good way to test cookie removal once this lands in aurora? Who should do that testing?
Attachment #8749241 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 18•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/17fd7b532dcf
You need to log in
before you can comment on or make changes to this bug.
Description
•