Last Comment Bug 388079 - Deleting multiple cookies deletes wrong ones and/or not all selected
: Deleting multiple cookies deletes wrong ones and/or not all selected
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 13
Assigned To: Christian Sonne [:cers]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-07-13 13:11 PDT by Dale
Modified: 2012-09-05 08:29 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed patch (650 bytes, patch)
2012-02-01 04:52 PST, Christian Sonne [:cers]
no flags Details | Diff | Splinter Review
Proposed patch (651 bytes, patch)
2012-02-01 04:56 PST, Christian Sonne [:cers]
ttaubert: review+
jaws: feedback+
Details | Diff | Splinter Review
Proposed patch (655 bytes, patch)
2012-02-01 07:08 PST, Christian Sonne [:cers]
cers: review+
Details | Diff | Splinter Review
patch for checkin (918 bytes, patch)
2012-02-01 08:50 PST, Christian Sonne [:cers]
no flags Details | Diff | Splinter Review
patch for checkin (918 bytes, patch)
2012-02-01 13:20 PST, Christian Sonne [:cers]
no flags Details | Diff | Splinter Review

Description Dale 2007-07-13 13:11:29 PDT
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 [:Tyler] 2010-04-24 17:09:39 PDT
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 [:Tyler] 2010-05-22 17:42:16 PDT
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 2010-05-23 05:29:55 PDT
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 [:Tyler] 2011-04-30 06:35:39 PDT
Does this work now with the release of Firefox 4?
Comment 5 Dale 2011-04-30 16:47:24 PDT
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 [:Tyler] 2011-06-03 17:32:57 PDT
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] 2012-02-01 04:52:06 PST
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.
Comment 8 Christian Sonne [:cers] 2012-02-01 04:56:02 PST
Created attachment 593388 [details] [diff] [review]
Proposed patch

Whoops, was missing a (cosmetic) space in comparison
Comment 9 Jared Wein [:jaws] (please needinfo? me) 2012-02-01 05:50:31 PST
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] 2012-02-01 06:37:51 PST
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] 2012-02-01 07:08:36 PST
Created attachment 593435 [details] [diff] [review]
Proposed patch

New version with corrections pointed out above
Comment 12 Christian Sonne [:cers] 2012-02-01 08:50:35 PST
Created attachment 593469 [details] [diff] [review]
patch for checkin

Created the patch with Mercurial Queues
Comment 13 Christian Sonne [:cers] 2012-02-01 13:20:29 PST
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.
Comment 14 Tim Taubert [:ttaubert] 2012-02-01 14:06:20 PST
https://hg.mozilla.org/integration/fx-team/rev/faada6fcee1d
Comment 15 Tim Taubert [:ttaubert] 2012-02-02 01:10:33 PST
https://hg.mozilla.org/mozilla-central/rev/faada6fcee1d

Thanks, Christian!
Comment 16 Andreas Eibach 2012-09-05 08:29:47 PDT
This is interesting. "let" should not cause problems in JavaScript >= 1.7, though it certainly will with anything below that.

Note You need to log in before you can comment on or make changes to this bug.