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

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Dale]

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.

Comment 1

[Tyler Downer [He/Him]]

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

Comment 2

[Tyler Downer [He/Him]]

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.

Comment 3

[Dale]

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

Comment 4

[Tyler Downer [He/Him]]

Does this work now with the release of Firefox 4?

Comment 5

[Dale]

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.

Comment 6

[Tyler Downer [He/Him]]

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

Comment 7

[Christian Sonne [:cers]]

Attachment: 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.

Comment 8

[Christian Sonne [:cers]]

Attachment: Proposed patch

Whoops, was missing a (cosmetic) space in comparison

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

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|?

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 11

[Christian Sonne [:cers]]

Attachment: Proposed patch

New version with corrections pointed out above

Comment 12

[Christian Sonne [:cers]]

Attachment: patch for checkin

Created the patch with Mercurial Queues

Comment 13

[Christian Sonne [:cers]]

Attachment: 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.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/faada6fcee1d

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/faada6fcee1d

Thanks, Christian!

Comment 16

[Andreas Eibach]

This is interesting. "let" should not cause problems in JavaScript >= 1.7, though it certainly will with anything below that.