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

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect, P1)

Description

[Tim Huang[:timhuang]]

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 1

[Tim Huang[:timhuang]]

Attachment: Fix the nsICookieManager.remove() to reference originAttributes correctly at release build.

Comment 2

[Tim Huang[:timhuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b2ea3ad034a

Comment 3

[Andrea Marchesini [:baku]]

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;
}

Comment 4

[Tim Huang[:timhuang]]

Attachment: Fix the nsICookieManager.remove() to reference originAttributes correctly at release build.

Baku, thanks for your review comment. This patch does what you said.

Comment 5

[Tim Huang[:timhuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=804415f67e53

Comment 6

[Tim Huang[:timhuang]]

Attachment: Fix the nsICookieManager.remove() to reference originAttributes correctly at release build.

Fix treeherder failures of marionette framework unit tests.

Comment 7

[Tim Huang[:timhuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=18f52123f39e

Comment 8

[Tanvi Vyas[:tanvi]]

Will this need uplift?

Comment 9

[Ethan Tseng [:ethan]]

(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

Comment 10

[Andrea Marchesini [:baku]]

> [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.

Comment 11

[Ethan Tseng [:ethan]]

(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.

Comment 12

[Tim Huang[:timhuang]]

Try test looks good.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/002cfe847c66

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/002cfe847c66

Comment 15

[Tim Huang[:timhuang]]

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

Comment 16

[Ethan Tseng [:ethan]]

(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 17

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

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?

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/17fd7b532dcf