Last Comment Bug 705422 - Remove all cookies button not disabled when filter is applied via Page Info dialog
: Remove all cookies button not disabled when filter is applied via Page Info d...
Status: RESOLVED FIXED
: regression
Product: Firefox
Classification: Client Software
Component: Preferences (show other bugs)
: 10 Branch
: All All
: -- normal (vote)
: Firefox 13
Assigned To: Christian Sonne [:cers]
:
Mentors:
: 703144 786241 (view as bug list)
Depends on:
Blocks: 495511 609588
  Show dependency treegraph
 
Reported: 2011-11-26 05:40 PST by Christian Sonne [:cers]
Modified: 2012-08-28 12:56 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
+
affected
+
affected
affected
-
wontfix


Attachments
Screen shot of active "Delete All Cookies" button (59.28 KB, image/png)
2011-11-26 05:40 PST, Christian Sonne [:cers]
no flags Details
Proposed patch (562 bytes, patch)
2011-11-26 14:29 PST, Christian Sonne [:cers]
no flags Details | Diff | Review
Proposed patch (562 bytes, patch)
2011-11-27 12:46 PST, Christian Sonne [:cers]
no flags Details | Diff | Review
Comprehensive version, enables "select all" both via keyboard and as button (17.42 KB, patch)
2011-11-27 13:07 PST, Christian Sonne [:cers]
no flags Details | Diff | Review
Comprehensive version, enables "select all" both via keyboard and as button (17.49 KB, patch)
2011-11-27 18:27 PST, Christian Sonne [:cers]
no flags Details | Diff | Review
"Remove All Cookies" removes only shown cookies (2.62 KB, patch)
2011-11-28 15:47 PST, Christian Sonne [:cers]
jaws: feedback+
Details | Diff | Review
"Remove All Cookies" removes only shown cookies (4.36 KB, patch)
2011-11-29 08:53 PST, Christian Sonne [:cers]
gavin.sharp: review+
jaws: feedback+
Details | Diff | Review
"Remove All Cookies" removes only shown cookies (4.25 KB, patch)
2011-11-30 12:21 PST, Christian Sonne [:cers]
no flags Details | Diff | Review
Test for cookie deletion codepaths (6.29 KB, patch)
2012-01-31 07:11 PST, Christian Sonne [:cers]
gavin.sharp: review+
Details | Diff | Review
patch for checkin (10.80 KB, patch)
2012-02-05 10:03 PST, Christian Sonne [:cers]
emorley: review+
Details | Diff | Review

Description Christian Sonne [:cers] 2011-11-26 05:40:24 PST
Created attachment 577050 [details]
Screen shot of active "Delete All Cookies" button

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:11.0a1) Gecko/20111124 Firefox/11.0a1
Build ID: 20111124031031

Steps to reproduce:

1. Go to Page Info->Security->View Cookies
2. Click "Remove All Cookies" (warning: don't actually do this)


Actual results:

Removes all cookies, not just the ones listed


Expected results:

Should either remove only listed cookies, or button should be disabled.
Note: if filter is applied manually, button is disabled.
Comment 1 Christian Sonne [:cers] 2011-11-26 05:48:15 PST
This was probably introduced by bug 378668. Bug 609588 might also be relevant.
Comment 2 Alice0775 White 2011-11-26 11:51:22 PST
confirmed on 
http://hg.mozilla.org/releases/mozilla-aurora/rev/66ca8846cb64
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a2) Gecko/20111125 Firefox/10.0a2 ID:20111125042027
and
http://hg.mozilla.org/mozilla-central/rev/c58bad0b4640
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111126 Firefox/11.0a1 ID:20111126031027

Not reproduced on Firefox8.0.1 and Firefox9.0beta.

Regression window(m-i):
Works:
http://hg.mozilla.org/mozilla-central/rev/9fa7d2c8ec2d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20111022 Firefox/10.0a1 ID:20111022105646
Fails:
http://hg.mozilla.org/mozilla-central/rev/72bb20c484a2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20111022 Firefox/10.0a1 ID:20111022121146
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9fa7d2c8ec2d&tochange=72bb20c484a2
Suspected:
4d5244c0a0de	Javi Rueda — Bug 495511 - Remove All Cookies button is enabled when no cookies are present. r=jwein


Regression window(fx-team):
Works:
http://hg.mozilla.org/integration/fx-team/rev/7b05a3d1f56e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20111019 Firefox/10.0a1 ID:20111019040232
Fails:
http://hg.mozilla.org/integration/fx-team/rev/a67521de22d5
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20111021 Firefox/10.0a1 ID:20111021040340
Pushlog:
http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=7b05a3d1f56e&tochange=a67521de22d5
Triggered by:
4d5244c0a0de	Javi Rueda — Bug 495511 - Remove All Cookies button is enabled when no cookies are present. r=jwein
Comment 3 Christian Sonne [:cers] 2011-11-26 14:29:46 PST
Created attachment 577104 [details] [diff] [review]
Proposed patch

This seems to fix in on trunk. Not sure if it follows the style guide, but then again, I wouldn't have thought the original code did...
Comment 4 Javi Rueda 2011-11-26 16:38:15 PST
About attachment 577104 [details] [diff] [review], is the expected behavior to be disabled although the list been not empty?

Said in a different way: does a item in the filtered list must be removed manually?
Comment 5 Christian Sonne [:cers] 2011-11-26 17:30:11 PST
(In reply to Javi Rueda from comment #4)
> About attachment 577104 [details] [diff] [review] [diff] [details] [review], is the expected behavior
> to be disabled although the list been not empty?
> 
> Said in a different way: does a item in the filtered list must be removed
> manually?

The problem is that "Remove All Cookies" actually removes *all* cookies, not just the ones shown. If anything is to change this behavior, it should probably be that it only removes shown cookies.

The current problem is compounded by the fact that you can't easily select all shown cookies to delete them. See bug 403751.
Comment 6 Christian Sonne [:cers] 2011-11-27 12:46:46 PST
Created attachment 577157 [details] [diff] [review]
Proposed patch

Removed typo in the original patch
Comment 7 Christian Sonne [:cers] 2011-11-27 13:07:14 PST
Created attachment 577160 [details] [diff] [review]
Comprehensive version, enables "select all" both via keyboard and as button

This is a more comprehensive solution, as it also adds "Select All Cookies" as a button and via Ctrl+a/Cmd+a. (Ctrl+Shift+a/Cmd+Shift+a deselects all, but I'm not sure if this is expected behavior to everyone?)

I also corrected some minor issues with regards to style, so the document conforms better to the style guide.

It should effectively fix not only this bug, but also bug 403751 and possibly bug 609588.
Comment 8 Christian Sonne [:cers] 2011-11-27 18:27:13 PST
Created attachment 577181 [details] [diff] [review]
Comprehensive version, enables "select all" both via keyboard and as button

Added #ifdef's to tailor Ctrl/Cmd to target OS.
Comment 9 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2011-11-28 10:19:58 PST
I don't think we should add the key bindings in the fix for this bug, nor another button.

Select lists in HTML don't appear to respond to ctrl+shift+a or ctrl+a (ctrl+shift+a when focused on a <select multiple> opens the Add-Ons Manager for me).

The patch provided is very large due relative to size required to fix this bug. Are these the only relevant lines required for the fix?

 
-    document.getElementById("removeAllCookies").disabled = this._view._rowCount == 0;
+    document.getElementById("removeAllCookies").disabled = (this._view._rowCount == 0
+                                                         || this._view.filtered);
 

Whatever the case, it might make it easier to review if the patch is split in two parts: part A for the fix and part B for the style fixes.
Comment 10 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2011-11-28 10:21:15 PST
s/due// in previous comment.
Comment 11 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2011-11-28 10:25:33 PST
Comment on attachment 577157 [details] [diff] [review]
Proposed patch

This patch appears to fix the bug, but not the way that I think we should. The "Remove all cookies" button is still desirable in this view, but it should only remove the cookies that are shown to the user.

Instead of disabling the button, can we just make the button remove only the cookies that are being shown?
Comment 12 Christian Sonne [:cers] 2011-11-28 15:46:23 PST
(In reply to Jared Wein [:jwein and :jaws] from comment #9)
> I don't think we should add the key bindings in the fix for this bug, nor
> another button.
> 
> Select lists in HTML don't appear to respond to ctrl+shift+a or ctrl+a
> (ctrl+shift+a when focused on a <select multiple> opens the Add-Ons Manager
> for me).
> 
> The patch provided is very large due relative to size required to fix this
> bug. Are these the only relevant lines required for the fix?
> 
>  
> -    document.getElementById("removeAllCookies").disabled =
> this._view._rowCount == 0;
> +    document.getElementById("removeAllCookies").disabled =
> (this._view._rowCount == 0
> +                                                         ||
> this._view.filtered);
>  
> 
> Whatever the case, it might make it easier to review if the patch is split
> in two parts: part A for the fix and part B for the style fixes.

"Proposed patch" ( https://bugzilla.mozilla.org/attachment.cgi?id=577157 ) contains just "the fix", keeping the old behavior of the Remove All Cookies button, and no style fixes.

(In reply to Jared Wein [:jwein and :jaws] from comment #11)
> Comment on attachment 577157 [details] [diff] [review] [diff] [details] [review]
> Proposed patch
> 
> This patch appears to fix the bug, but not the way that I think we should.
> The "Remove all cookies" button is still desirable in this view, but it
> should only remove the cookies that are shown to the user.
> 
> Instead of disabling the button, can we just make the button remove only the
> cookies that are being shown?

I've just finished a patch that does this - the way I delete cookies when the tree is filtered might not be optimal (instead of having to implement something that deletes the items in the tree, I used existing functions to
1. select all in the tree
2. delete selected

Perhaps with a lot of results, this performs badly (on my limited test profile, I don't get to see the selection at all). Someone who knows more about the cookie manager might want to look at that.

When no filter is applied, I fall back to the old removeAll() function.
Comment 13 Christian Sonne [:cers] 2011-11-28 15:47:53 PST
Created attachment 577402 [details] [diff] [review]
"Remove All Cookies" removes only shown cookies
Comment 14 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2011-11-28 17:08:26 PST
Thank you for the quick turnaround on the patch. Is there a reason why you have switched this from ASSIGNED -> NEW and the Hardware from All -> x86?
Comment 15 Christian Sonne [:cers] 2011-11-28 17:19:21 PST
Whoops, no - I guess I just assumed it wouldn't change fields unless I did so manually - I guess they might have "stuck" because I kept the page opened and reloaded, causing Firefox to remember the values of the form elements.
Comment 16 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2011-11-28 21:13:37 PST
Comment on attachment 577402 [details] [diff] [review]
"Remove All Cookies" removes only shown cookies

This looks OK to me, except it's kind of a hack to select all the items in the view so the |deleteCookie| function will clear out the filtered selection, but I can live with it.

Please request review from somebody on this list when you are ready: https://wiki.mozilla.org/Modules/Firefox
Comment 17 Christian Sonne [:cers] 2011-11-29 08:53:25 PST
Created attachment 577639 [details] [diff] [review]
"Remove All Cookies" removes only shown cookies

Updated (less hacky) version of the patch that doesn't just selectAll/deleteCookie.

I figured I should prompt you for feedback on the new version before I requested review. (I've never actually submitted a patch before - do I just click "Splinter Review" next to the patch and add the request there?)
Comment 18 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-11-29 10:36:27 PST
*** Bug 703144 has been marked as a duplicate of this bug. ***
Comment 19 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2011-11-29 11:04:46 PST
Comment on attachment 577639 [details] [diff] [review]
"Remove All Cookies" removes only shown cookies

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

Looks good to me. I've gone ahead and requested review from Gavin Sharp for this patch for you. To request review in the future, do the same thing as you did when requesting feedback from me except use the "review" dropdown. 

And thanks for contributing!

::: browser/components/preferences/cookies.js
@@ +604,5 @@
> +      this._cm.remove(item.host, item.name, item.path, blockFutureCookies);
> +    }
> +  },
> +
> + deleteCookie: function () {

nit: please add another space here before |deleteCookie|. In total, this should have two spaces before it to be consistent with other functions.
Comment 20 Christian Sonne [:cers] 2011-11-29 17:52:38 PST
(In reply to Jared Wein [:jwein and :jaws] from comment #19)
> > +
> > + deleteCookie: function () {
> 
> nit: please add another space here before |deleteCookie|. In total, this
> should have two spaces before it to be consistent with other functions.

Good catch, I'll just wait for Gavin to chime in before I upload a new version so I can take his review into account..
Comment 21 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-11-30 11:06:48 PST
Comment on attachment 577639 [details] [diff] [review]
"Remove All Cookies" removes only shown cookies

>diff -r bc48009a6bbb browser/components/preferences/cookies.js

>   deleteAllCookies: function () {

>+    if (this._view._filtered) {
>+      var {rowCount} = this._view;

Using destructuring assignment for a single property is just confusing, just use this._view.rowCount.

>+      this._view._rowCount -= rowCount;

= 0; ?

This code is very hard to follow (nsITreeView API doesn't help). Any chance you'd be willing to write a test for this functionality?
Comment 22 Christian Sonne [:cers] 2011-11-30 12:21:22 PST
Created attachment 578043 [details] [diff] [review]
"Remove All Cookies" removes only shown cookies

(In reply to Jared Wein [:jwein and :jaws] from comment #19)
> nit: please add another space here before |deleteCookie|. In total, this
> should have two spaces before it to be consistent with other functions.

Fixed

(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #21)
> >+      var {rowCount} = this._view;
> 
> Using destructuring assignment for a single property is just confusing, just
> use this._view.rowCount.

Noted and done.

> >+      this._view._rowCount -= rowCount;
> 
> = 0; ?

I wasn't sure if _rowCount reflected the unfiltered or filtered number, but after testing it seem it does indeed reflect the filtered count, and 0 is therefor fine. I've changed it in this version

> This code is very hard to follow (nsITreeView API doesn't help). Any chance
> you'd be willing to write a test for this functionality?

I've never written a test for Firefox, and I'm not entirely sure what it would take to write a good one for this subject. Something like injecting X cookies into an empty manager, filtering, removing all, and then count? Maybe checking that none of the ones left match the filter after too (I guess that would make sure we deleted the right ones)?
Comment 23 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-12-01 10:26:20 PST
(In reply to Christian Sonne [:cers] from comment #22)
> I've never written a test for Firefox, and I'm not entirely sure what it
> would take to write a good one for this subject. Something like injecting X
> cookies into an empty manager, filtering, removing all, and then count?
> Maybe checking that none of the ones left match the filter after too (I
> guess that would make sure we deleted the right ones)?

Yeah, exactly - just trying to exercise the various "remove" code paths here, and then check their impact on both the UI and the underlying data store. http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/tests/ has some examples of existing preference pane tests that might be helpful, and you can ask on #fx-team for further guidance if needed.
Comment 24 Javi Rueda 2012-01-23 06:44:41 PST
The revision with the patch that caused the problem (mine), has been backed-out? Looking the code at the mozilla-central repository, it seems that it hasn't been.

However, I am not able to see this problem in my last nightly build.

Again, I am very worried by my TERRIBLY wrong patch code. If there ia anything I could do to improve this situation, please tell me. Thank you and sorry because all of that. :-S
Comment 25 Alice0775 White 2012-01-23 07:14:45 PST
(In reply to Javi Rueda from comment #24)
> The revision with the patch that caused the problem (mine), has been
> backed-out? Looking the code at the mozilla-central repository, it seems
> that it hasn't been.
> 
http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/cookies.js#92
Comment 26 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-23 17:07:10 PST
Christian: any update on writing a test? We should get this patch landed soon.
Comment 27 Christian Sonne [:cers] 2012-01-24 11:41:21 PST
I've spent some time trying to figure out how exactly to accomplish the tests, but there are a lot of things I just don't really know enough about to do on my own. I had talked to Blair (unfocused) about walking me through the steps, but we haven't been able to schedule that as of yet.

I do have a plan of what to test ready though, if that's any help:

1) open the cookie manager
2) inject X cookies
3) count all cookies == X (sanity check)
4) check if "delete all" is disabled (shouldn't be)
5) search for subset size Y>=5 of X injected cookies
6) count cookies == Y
7) check if "delete all" is disabled (shouldn't be)
8) select one cookie
9) delete cookie
10) count cookies == Y-1
11) select two cookies (not adjacent)
12) delete cookies
13) count cookies == Y-3
14) delete all cookies
15) count cookies == 0
16) check if "delete all" is disabled (should be)
17) clear search
18) count cookies == X-Y
19) check if "delete all" is disabled (shouldn't be)
20) delete all cookies
21) count cookies == 0
22) check if "delete all" is disabled (should be)

I think that should just about cover all cases - unless we want to inject cookies while filtered, to check if that also works as expected?
Comment 28 Christian Sonne [:cers] 2012-01-24 13:23:47 PST
Mostly, I need help with points 1-2. The rest I should be able to manage...
Comment 29 Christian Sonne [:cers] 2012-01-30 23:08:15 PST
While writing the test (point 11-13), I have stumbled upon a new bug I think - one neither introduced or fixed by my patch.

If you perform those steps by hand, only the first cookie is deleted, while the second remains selected.

This behavior has been confirmed on nightly by glob in #fx-team

Should I skip that part of the test, file a new bug and add the test there, or leave it in this test and mark the other bug as blocking?
Comment 30 Christian Sonne [:cers] 2012-01-31 07:11:04 PST
Created attachment 593083 [details] [diff] [review]
Test for cookie deletion codepaths

(In reply to Christian Sonne [:cers] from comment #29)
> While writing the test (point 11-13), I have stumbled upon a new bug I think
> - one neither introduced or fixed by my patch.

I've changed this test to try adjacent items (which wasn't tested before), so I'll open a new bug for deleting non-adjacent items and submit a test for that there
Comment 31 Christian Sonne [:cers] 2012-02-02 15:23:12 PST
(In reply to Christian Sonne [:cers] from comment #29)
> While writing the test (point 11-13), I have stumbled upon a new bug I think
> - one neither introduced or fixed by my patch.
> 
> If you perform those steps by hand, only the first cookie is deleted, while
> the second remains selected.

This turned out to be bug 388079, which is now fixed on trunk.
Comment 32 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-02-03 16:46:39 PST
Thanks so much for persisting and driving this to completion, Christian. Test writing can be really tricky, particularly when you're dealing with XUL trees and previously-untested areas, but it's well worth it in the end - we have good coverage for this dialog now, which lets us be confident we won't break it again in the future :)
Comment 33 Christian Sonne [:cers] 2012-02-05 10:03:50 PST
Created attachment 594561 [details] [diff] [review]
patch for checkin

Combined attachment 578043 [details] [diff] [review] and attachment 593083 [details] [diff] [review]
Comment 34 Ed Morley [:emorley] 2012-02-05 10:12:19 PST
Comment on attachment 594561 [details] [diff] [review]
patch for checkin

(To keep autoland happy about permissions)
Comment 35 Mozilla RelEng Bot 2012-02-05 10:16:05 PST
Autoland Patchset:
	Patches: 594561
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/rev/9299aaf9fac7
Try run started, revision 9299aaf9fac7. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=9299aaf9fac7
Comment 37 Mozilla RelEng Bot 2012-02-05 14:45:23 PST
Try run for 9299aaf9fac7 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=9299aaf9fac7
Results (out of 207 total builds):
    exception: 1
    success: 188
    warnings: 17
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-9299aaf9fac7
Comment 38 Marco Bonardo [::mak] 2012-02-06 00:52:39 PST
https://hg.mozilla.org/mozilla-central/rev/e1312b1625a4
Comment 39 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-28 07:09:29 PDT
While this is a fairly low priority bug, it's been riding ESR for a while. Can/should this be landed for the next ESR release? (RE: Bug 786241)
Comment 40 Dão Gottwald [:dao] 2012-08-28 08:06:22 PDT
*** Bug 786241 has been marked as a duplicate of this bug. ***
Comment 41 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-28 12:56:00 PDT
I don't see any reason why we'd fix this on ESR, it's not a security/stability issue and it's a relatively minor bug.

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