Closed Bug 705422 Opened 13 years ago Closed 12 years ago

Remove all cookies button not disabled when filter is applied via Page Info dialog

Categories

(Firefox :: Settings UI, defect)

10 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 13
Tracking Status
firefox8 --- affected
firefox9 + affected
firefox10 + affected
firefox11 --- affected
firefox-esr10 - wontfix

People

(Reporter: cers, Assigned: cers)

References

Details

(Keywords: regression)

Attachments

(2 files, 8 obsolete files)

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.
This was probably introduced by bug 378668. Bug 609588 might also be relevant.
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
Blocks: 495511
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
OS: Mac OS X → All
Version: 11 Branch → 10 Branch
Attached patch Proposed patch (obsolete) — Splinter Review
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...
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?
(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.
Attached patch Proposed patch (obsolete) — Splinter Review
Removed typo in the original patch
Attachment #577104 - Attachment is obsolete: true
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.
Added #ifdef's to tailor Ctrl/Cmd to target OS.
Attachment #577160 - Attachment is obsolete: true
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.
Assignee: nobody → cers
Status: NEW → ASSIGNED
Hardware: x86 → All
s/due// in previous comment.
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?
(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.
Status: ASSIGNED → NEW
Hardware: All → x86
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?
Status: NEW → ASSIGNED
Hardware: x86 → All
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 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
Attachment #577402 - Flags: feedback+
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?)
Attachment #577402 - Attachment is obsolete: true
Attachment #577639 - Flags: feedback?(jwein)
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.
Attachment #577639 - Flags: review?(gavin.sharp)
Attachment #577639 - Flags: feedback?(jwein)
Attachment #577639 - Flags: feedback+
(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 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?
Attachment #577639 - Flags: review?(gavin.sharp) → review+
(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)?
Attachment #577639 - Attachment is obsolete: true
(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.
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
(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
Christian: any update on writing a test? We should get this patch landed soon.
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?
Mostly, I need help with points 1-2. The rest I should be able to manage...
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?
(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
Attachment #593083 - Flags: review?(gavin.sharp)
(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.
Target Milestone: --- → Firefox 12
Target Milestone: Firefox 12 → ---
Attachment #593083 - Flags: review?(gavin.sharp) → review+
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 :)
Combined attachment 578043 [details] [diff] [review] and attachment 593083 [details] [diff] [review]
Attachment #577157 - Attachment is obsolete: true
Attachment #577181 - Attachment is obsolete: true
Attachment #578043 - Attachment is obsolete: true
Attachment #593083 - Attachment is obsolete: true
Keywords: checkin-needed
Comment on attachment 594561 [details] [diff] [review]
patch for checkin

(To keep autoland happy about permissions)
Attachment #594561 - Flags: review+
Whiteboard: [autoland-try]
Whiteboard: [autoland-try] → [autoland-in-queue]
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
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
Whiteboard: [autoland-in-queue]
https://hg.mozilla.org/mozilla-central/rev/e1312b1625a4
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Blocks: 609588
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)
Blocks: 786241
No longer blocks: 786241
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.
You need to log in before you can comment on or make changes to this bug.