The nsICookieManager.remove() does not reference originAttributes at release build.

RESOLVED FIXED in Firefox 48

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: timhuang, Assigned: timhuang)

Tracking

(Blocks 1 bug)

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48+ fixed, firefox49 fixed)

Details

(Whiteboard: [oa][necko-active][userContextId])

Attachments

(1 attachment, 2 obsolete attachments)

According to https://dxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.cpp#2390 , the NeckoOriginAttributes will not be set correctly at release build.
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;
}
Baku, thanks for your review comment. This patch does what you said.
Attachment #8749149 - Flags: review?(ehsan)
Assignee

Updated

3 years ago
Attachment #8749113 - Attachment is obsolete: true
Attachment #8749113 - Flags: review?(ehsan)
Whiteboard: [oa] → [oa][necko-active]
Fix treeherder failures of marionette framework unit tests.
Attachment #8749241 - Flags: review?(ehsan)
Assignee

Updated

3 years ago
Attachment #8749149 - Attachment is obsolete: true
Attachment #8749149 - Flags: review?(ehsan)
Will this need uplift?
Whiteboard: [oa][necko-active] → [oa][necko-active][userContextId]
(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)
> [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)
(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.
Priority: -- → P1
Attachment #8749241 - Flags: review?(ehsan) → review+
Try test looks good.
Keywords: checkin-needed

Comment 14

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/002cfe847c66
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
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?
(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).
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+
You need to log in before you can comment on or make changes to this bug.