Deleting multiple cookies deletes wrong ones and/or not all selected

RESOLVED FIXED in Firefox 13

Status

()

Firefox
Preferences
RESOLVED FIXED
10 years ago
5 years ago

People

(Reporter: Dale, Assigned: cers)

Tracking

Trunk
Firefox 13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

10 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.4) Gecko/20070515 Firefox/2.0.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.4) Gecko/20070515 Firefox/2.0.0.4

When attempting to remove some cookies by selecting multiple cookies in the cookie manager, it will often delete the wrong cookies.

Reproducible: Always

Steps to Reproduce:
1. Convince any phpBB-driven forum to set its four *_data, *_sid, *_t, and *_f cookies. (For this, I used the MozillaZine forums, but any such forum should do.)
2. Open the cookie manager.
3. In the search box, enter a search that will display only those four cookies. (e.g. "mzf")
4. Select the first and third cookies.
5. Press the delete key or click the Remove Cookies button.
Actual Results:  
The first and *fourth* cookies get deleted. Apparently. However, re-opening the cookie manager shows that only the first cookie got deleted.

Expected Results:  
Exactly the selected cookies (the first and third) should get deleted, and the cookie manager should should reflect this immediately.

The workaround is obvious, but irritating: Delete cookies one at a time.
(Reporter)

Updated

10 years ago
Summary: Deleting multiple cookies sometimes deletes wrong ones → Deleting multiple cookies deletes wrong ones and/or not all selected
This bug was reported on Firefox 2.x or older, which is no longer supported and will not be receiving any more updates. I strongly suggest that you update to Firefox 3.6.3 or later, update your plugins (flash, adobe, etc.), and retest in a new profile. If you still see the issue with the updated Firefox, please post here. Otherwise, please close as RESOLVED > WORKSFORME
http://www.mozilla.com
http://support.mozilla.com/kb/Managing+profiles
http://support.mozilla.com/kb/Safe+mode
Whiteboard: [CLOSEME 5-15-2010]
Version: unspecified → 2.0 Branch
No reply, INCOMPLETE. Please retest with Firefox 3.6.x or later and a new profile (http://support.mozilla.com/kb/Managing+profiles). If you continue to see this issue with the newest firefox and a new profile, then please comment on this bug.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → INCOMPLETE
(Reporter)

Comment 3

7 years ago
Sorry. Missed your first comment. I retested with a new profile in Firefox 3.6.3[0] and it seems to be worse. This could, however, be a side effect of running a slightly different test.

1. Visit the Mozillazine forums. This will set three cookies: _sid, _k, and _u.
2. Open the cookie manager.
3. In the search box, enter "mzf". This should display only the three mozillazine cookies. If necessary, add more characters until only those three cookies are present.
4. Select the first and third cookies (_sid and _u).
5. Press the delete key or click the Remove Cookies button.

Actual Results:  
Only first cookie (_sid) gets deleted. Apparently. However, re-opening the
cookie manager shows that no cookies got deleted.

Expected results and workaround are as above.

[0] Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3
Status: RESOLVED → UNCONFIRMED
Resolution: INCOMPLETE → ---

Updated

7 years ago
Whiteboard: [CLOSEME 5-15-2010]
Version: 2.0 Branch → 3.6 Branch
Does this work now with the release of Firefox 4?
(Reporter)

Comment 5

6 years ago
Not fixed.

It is necessary to enter something into the search box to enable multi-select, and it appears that this only happens if the selection is non-contiguous, but it's still in the same state as comment 3: If the selection is non-contiguous, the first selected cookie is removed from the list but not deleted. All remaining cookies are neither removed from the list nor deleted.

Tested with a new profile on:
Mozilla/5.0 (Windows NT 5.1; rv:2.0.1) Gecko/20100101 Firefox/4.0.1.
Version: 3.6 Branch → 4.0 Branch
This bug was reported using a pre-release version of Firefox 4. Now that Firefox 4.0.1 final has been released, can you please update and retest your bug? A fresh profile would be a good starting place to test, 
http://support.mozilla.com/kb/Managing+profiles. If you continue to see the issue, can you please update this bug with your results?

Filter: firefox4prebugsunco
(Assignee)

Comment 7

5 years ago
Created attachment 593382 [details] [diff] [review]
Proposed patch

I can confirm this bug in recent nightlies. The problem is traversing forwards through non-adjacent selections, while deleting said selections in the process.
Attachment #593382 - Flags: review?
(Assignee)

Comment 8

5 years ago
Created attachment 593388 [details] [diff] [review]
Proposed patch

Whoops, was missing a (cosmetic) space in comparison
Attachment #593382 - Attachment is obsolete: true
Attachment #593388 - Flags: review?
Attachment #593382 - Flags: review?
Assignee: nobody → cers
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Updated

5 years ago
Component: General → Preferences
OS: Windows XP → All
QA Contact: general → preferences
Hardware: x86 → All
Version: 4.0 Branch → Trunk
(Assignee)

Updated

5 years ago
Attachment #593388 - Flags: review? → review?(jwein)
Comment on attachment 593388 [details] [diff] [review]
Proposed patch

Looks good to me, but I can't grant review here unfortunately.

>+      // traverse backwards through selections to avoid messing 
>+      // up the indices when they are deleted
>+      // see bug 388079

Nit: Can you please use capitalize the sentence and add a period here?

>+      for (var i = rangeCount-1; i > -1; --i) {

Nit: Can you please place a space between operators, such as |rangeCount - 1|?
Attachment #593388 - Flags: review?(ttaubert)
Attachment #593388 - Flags: review?(jwein)
Attachment #593388 - Flags: feedback+
Comment on attachment 593388 [details] [diff] [review]
Proposed patch

Review of attachment 593388 [details] [diff] [review]:
-----------------------------------------------------------------

What Jared said. Everything else looks good... Thanks for this patch!

r=me with those nits fixed.

::: browser/components/preferences/cookies.js
@@ +695,5 @@
>        var rangeCount = seln.getRangeCount();
> +      // traverse backwards through selections to avoid messing 
> +      // up the indices when they are deleted
> +      // see bug 388079
> +      for (var i = rangeCount-1; i > -1; --i) {

While you're at this line anyway, please s/var/let/.
Attachment #593388 - Flags: review?(ttaubert) → review+
(Assignee)

Comment 11

5 years ago
Created attachment 593435 [details] [diff] [review]
Proposed patch

New version with corrections pointed out above
Attachment #593388 - Attachment is obsolete: true
Attachment #593435 - Flags: review+
(Assignee)

Comment 12

5 years ago
Created attachment 593469 [details] [diff] [review]
patch for checkin

Created the patch with Mercurial Queues
Attachment #593435 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Comment 13

5 years ago
Created attachment 593585 [details] [diff] [review]
patch for checkin

I've just found that I get this error after compiling with the  previous patch:

JavaScript error: chrome://browser/content/preferences/cookies.js, line 615: missing ; after for-loop initializer

That's the new for loop:
for (let i = rangeCount - 1; i >= 0; --i) {

I've changed the let to var and now it works.
Attachment #593469 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/faada6fcee1d
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 13
https://hg.mozilla.org/mozilla-central/rev/faada6fcee1d

Thanks, Christian!
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]

Comment 16

5 years ago
This is interesting. "let" should not cause problems in JavaScript >= 1.7, though it certainly will with anything below that.
You need to log in before you can comment on or make changes to this bug.