Closed
Bug 608545
Opened 15 years ago
Closed 15 years ago
Set cookie: unexpected "deleted" observer notification when cookie already exist
Categories
(Core :: Networking: Cookies, defect)
Tracking
()
RESOLVED
FIXED
mozilla2.0b7
Tracking | Status | |
---|---|---|
blocking2.0 | --- | beta7+ |
People
(Reporter: mash, Assigned: dwitte)
Details
Attachments
(1 file)
5.87 KB,
patch
|
sdwilsh
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•15 years ago
|
||
Regression or expected behaviour for Firefox 4.0?
Comment 2•15 years ago
|
||
Is it the same cookie being deleted and then changed, or is it a different cookie?
Reporter | ||
Comment 3•15 years ago
|
||
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•15 years ago
|
||
This may end up being WONTFIX, but blocking beta 7 on a decision or a fix.
blocking2.0: --- → beta7+
Reporter | ||
Comment 5•15 years ago
|
||
Last good nightly: 2010-10-19 First bad nightly: 2010-10-20
Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c9df0c5cbf8c&tochange=7aa9763e9d41
Reporter | ||
Comment 6•15 years ago
|
||
Comment 7•15 years ago
|
||
Over to dwitte for the decision. Shawn, why does this need to block b7?
Assignee: nobody → dwitte
Comment 8•15 years ago
|
||
(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•15 years ago
|
||
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
Assignee | ||
Comment 10•15 years ago
|
||
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...
Assignee | ||
Comment 11•15 years ago
|
||
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 12•15 years ago
|
||
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+
Assignee | ||
Comment 13•15 years ago
|
||
Will update and land tonight.
Updated•15 years ago
|
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Comment 14•15 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b7
Comment 15•15 years ago
|
||
(In reply to comment #14)
> http://hg.mozilla.org/mozilla-central/rev/45d6a73ef138
Er, that didn't address my comment though.
Assignee | ||
Comment 16•15 years ago
|
||
Right you are. Pushed followup http://hg.mozilla.org/mozilla-central/rev/46a81506595f.
Comment 17•15 years ago
|
||
(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.
Description
•