Closed Bug 608545 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Networking: Cookies, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b7
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: mash, Assigned: dwitte)

Details

Attachments

(1 file)

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
Regression or expected behaviour for Firefox 4.0?
Is it the same cookie being deleted and then changed, or is it a different cookie?
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.
This may end up being WONTFIX, but blocking beta 7 on a decision or a fix.
blocking2.0: --- → beta7+
Last good nightly: 2010-10-19 First bad nightly: 2010-10-20

Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c9df0c5cbf8c&tochange=7aa9763e9d41
Over to dwitte for the decision. Shawn, why does this need to block b7?
Assignee: nobody → dwitte
(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.
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?
Version: unspecified → Trunk
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...
Attached patch patchSplinter Review
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:
Attachment #487487 - Flags: review?(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
Attachment #487487 - Flags: review?(sdwilsh) → review+
Will update and land tonight.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
http://hg.mozilla.org/mozilla-central/rev/45d6a73ef138
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b7
(In reply to comment #14)
> http://hg.mozilla.org/mozilla-central/rev/45d6a73ef138
Er, that didn't address my comment though.
Right you are. Pushed followup http://hg.mozilla.org/mozilla-central/rev/46a81506595f.
(In reply to comment #16)
> Right you are. Pushed followup
> http://hg.mozilla.org/mozilla-central/rev/46a81506595f.
Thanks
You need to log in before you can comment on or make changes to this bug.