608545 - Set cookie: unexpected "deleted" observer notification when cookie already exist

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[Danil Ivanov]

User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b8pre) Gecko/20101030 Firefox/4.0b8pre
Build Identifier: 

let observer = {
  observe: function(aSubject, aTopic, aData) {
    if (aTopic == "cookie-changed")
      dump("observe: " + aData + "\n");
  }
};

Cc["@mozilla.org/observer-service;1"]
    .getService(Ci.nsIObserverService)
    .addObserver(observer, "cookie-changed", false);

let uri = Cc["@mozilla.org/network/standard-url;1"].createInstance(Ci.nsIURI);
uri.spec = "http://example.com";

let cookieStr = "test=test;path=/";

const cookieService = Cc["@mozilla.org/cookieService;1"].getService(Ci.nsICookieService);

cookieService.setCookieString(uri, null, cookieStr, null);
cookieService.setCookieString(uri, null, cookieStr, null);
cookieService.setCookieString(uri, null, cookieStr, null);

Reproducible: Always

Actual Results:  
(Firefox 4.0)
observe: added
observe: deleted
observe: changed
observe: deleted
observe: changed

Expected Results:  
(Firefox 3.0, 3.5, 3.6)
observe: added
observe: changed
observe: changed

Comment 1

[Danil Ivanov]

Regression or expected behaviour for Firefox 4.0?

Comment 2

[Shawn Wilsher :sdwilsh]

Is it the same cookie being deleted and then changed, or is it a different cookie?

Comment 3

[Danil Ivanov]

let observer = {
  observe: function(aSubject, aTopic, aData) {
    if (aTopic == "cookie-changed") {
      aSubject.QueryInterface(Ci.nsICookie);
      dump("observe: " + [aSubject.name, aSubject.host, aSubject.value, aData].join(", ") + "\n");
    }
  }
};
...

// Result:
observe: test, example.com, test, added
observe: test, example.com, test, deleted
observe: test, example.com, test, changed
observe: test, example.com, test, deleted
observe: test, example.com, test, changed

Same cookie.

Comment 4

[Shawn Wilsher :sdwilsh]

This may end up being WONTFIX, but blocking beta 7 on a decision or a fix.

Comment 5

[Danil Ivanov]

Last good nightly: 2010-10-19 First bad nightly: 2010-10-20

Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c9df0c5cbf8c&tochange=7aa9763e9d41

Comment 6

[Danil Ivanov]

Maybe http://hg.mozilla.org/mozilla-central/diff/03f4d774f862/netwerk/cookie/nsCookieService.cpp

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Over to dwitte for the decision. Shawn, why does this need to block b7?

Comment 8

[Shawn Wilsher :sdwilsh]

(In reply to comment #7)
> Over to dwitte for the decision. Shawn, why does this need to block b7?
There are two outcomes for this bug:
1) We decide that we are OK with this change in behavior, and WONTFIX it.
2) We realize this API change isn't what we want and therefor need to change it back the way it was.  It blocks beta 7 in this case because if we ship beta 7 with the API as it is on trunk (which we may decide is wrong), we are stuck with it for 2.0.

I think saying something is deleted and then saying it is changed is rather odd and certainly is a change from how we did things in the past.

Comment 9

[Dave Garrett]

Dumb question: Does it really even need to say "changed" if it's the same string each time? Shouldn't it just trigger the first "added" and ignore the next two attempts because they're not changing anything?

In the instance of an actual changed string, then the 1.9.x behavior makes more sense. Is there anything in 2.0 that is using the new "deleted" events?

Comment 10

[[e-mail]]

This is a bug. It will affect extensions, which I agree with Shawn is not a great situation even if it's an unintentional bug. The fix will be trivial, I'll look at it now...

Comment 11

[[e-mail]]

Attachment: patch

This does two things:

* Fixes this bug. The "deleted" notification just needs to be more narrowly scoped, to correspond with the behavior here: http://mxr.mozilla.org/mozilla1.9.2/source/netwerk/cookie/src/nsCookieService.cpp#1540

* Cleans up purge notifications. We're currently using "deleted" for single-cookie purges, but that's not really correct -- it's supposed to mean "the website deleted this cookie". The simplest thing here is just to use the existing "batch-deleted" notification with a single cookie.

There's a bug filed on getting tests for all the notifications -- bug 605882 -- I'll follow up with tests for this over there.

What we really want here is to treat purges differently from deletions:

Comment 12

[Shawn Wilsher :sdwilsh]

Comment on attachment 487487 [details] [diff] [review]
patch

>+      nsCOMPtr<nsIMutableArray> removedList =
>+        do_CreateInstance(NS_ARRAY_CONTRACTID);
>+      removedList->AppendElement(oldCookie, PR_FALSE);
>+      NotifyChanged(removedList, NS_LITERAL_STRING("batch-deleted").get());
You have this in two places.  Pull it into a helper and call it please

r=sdwilsh

Comment 13

[[e-mail]]

Will update and land tonight.

Comment 14

[[e-mail]]

http://hg.mozilla.org/mozilla-central/rev/45d6a73ef138

Comment 15

[Shawn Wilsher :sdwilsh]

(In reply to comment #14)
> http://hg.mozilla.org/mozilla-central/rev/45d6a73ef138
Er, that didn't address my comment though.

Comment 16

[[e-mail]]

Right you are. Pushed followup http://hg.mozilla.org/mozilla-central/rev/46a81506595f.

Comment 17

[Shawn Wilsher :sdwilsh]

(In reply to comment #16)
> Right you are. Pushed followup
> http://hg.mozilla.org/mozilla-central/rev/46a81506595f.
Thanks