View cookies button in page info's security tab should pre-filter the cookies dialog

VERIFIED FIXED in Firefox 3 beta3

Status

()

VERIFIED FIXED
12 years ago
8 years ago

People

(Reporter: johnath, Assigned: johnath)

Tracking

Trunk
Firefox 3 beta3
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 -
wanted-firefox3 +
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

(Assignee)

Description

12 years ago
The new security info allows me to view cookies - this dialog should be linked to the site currently being inspected.

Comment 1

12 years ago
johnath, are you working on this? i can help with some backend fu here.
(Assignee)

Comment 2

12 years ago
Created attachment 264101 [details] [diff] [review]
Naive patch to address this

This is the naive approach to launching the dialog with filter pre-populated.  It lets openDialog pass a filterString argument, and pre-sets it on init.  

The obvious snag is that the change in security.js (from page info) passes in hostname, and most cookies are domain cookies rather than host-specific, so this ends up filtering them out, since it overspecifies.

Dan, do you have any thoughts on a better approach here?
(In reply to comment #2)
> The obvious snag is that the change in security.js (from page info) passes in
> hostname, and most cookies are domain cookies rather than host-specific, so
> this ends up filtering them out, since it overspecifies.

Sounds like you could use the nsIEffectiveTLDService and filter on eTLD+1 instead of the full host.
(bug 367446 would also help here, I think)
(Assignee)

Comment 5

12 years ago
I've added a comment to bug 367446 asking if it can be revived.  I agree that it would be cleaner to use the helpers there than stringmunge on my own with the current nsIEffectiveTLDService.

Gavin, Dan, Jesse and I batted around some ideas today in IRC about having the cookies backend do the query for us, since substring matching on effectiveDomain will be over-lenient, displaying cookies that wouldn't actually be sent to the website in question.  

After a bunch of back and forth, I think we agreed that effectiveDomain is a reasonably-decent solution most of the time, though, and a helluvalot less work than reworking the way the dialog interacts with the backend.  And unlike the naive patch posted, it won't exclude any relevant cookies, it just risks including some irrelevant ones.

Comment 6

12 years ago
Comment on attachment 264101 [details] [diff] [review]
Naive patch to address this

>Index: base/content/pageinfo/security.js
>     var win = wm.getMostRecentWindow("Browser:Cookies");
>     if (win)
>       win.focus();
You also need to handle this case, when the Cookies window is already open.

Comment 7

12 years ago
So the backend says: yes, mxr.mozilla.org is storing cookies on my computer. But in fact, it's the domain cookie .mozilla.org. So why can't the backend tell us the domain (mozilla.org) of the domain cookie, so that we can filter on it?
OS: Mac OS X → All
Hardware: PC → All

Comment 8

12 years ago
in general, there could be many different domains of domain cookie that will match a given hostname. the only way for the cookiemgr to reliably display these cookies would be to ask the backend for a list of cookies for the hostname. this was proposed in comment 5; it'd involve reworking cookiemgr to pass the filter string to the backend, and show the list the backend gives back. this would be tricky to do, and would also deviate from standard filter string behavior which (i assume?) does a simple substring match.

the etld approach will simply return a greedy list of cookies.
(Assignee)

Comment 9

12 years ago
It would be nice in a general sort of way for the cookie dialog to accept this kind of filtration mechanism, but I think it's a tad out of scope for this bug, which is really concerned with a button on page info.  I would hope to fix *this bug* using eTLD+1, producing a greedy list that just behaves with the standard string-filter mechanic.  

If someone wants to open a bug against the cookie dialog code to support more sophisticated filtering, I'll happily take a follow-up bug to bring the page info button in line with the new code, once it exists.  But I don't think that work should block this fix.

Updated

12 years ago
Depends on: 368989
Summary: View cookies button in security info should pre-filter the cookies dialog → View cookies button in page info's security tab should pre-filter the cookies dialog

Updated

11 years ago
Flags: in-litmus?
Duplicate of this bug: 396383
Litmus Triage Team: Tomcat will add the test case in Litmus.

Updated

11 years ago
Flags: blocking-firefox3?
Not blocking, but very nice to have.
Flags: blocking-firefox3? → blocking-firefox3-
Whiteboard: [wanted-firefox3]
(Assignee)

Comment 13

11 years ago
Created attachment 289502 [details] [diff] [review]
Use eTLD+1 instead of hostname

This seems to do the trick, and by using eTLD+1, we are deliberately over-greedy - we might show you cookies that aren't sent to the site but we won't fail to show cookies that are.*

* Actually, obviously, since a site can embed ads or tracking bugs from other domains, this approach isn't guaranteed to show you all the cookies that might be sent from this page, but that's a much harder problem to solve in a generalized way, and I think this fix, which does let the user say "I want to see all cookies sent to ebay.com," is a significant improvement from the status quo.

Tagging mano because I've been tagging gavin a lot lately.  :)
Attachment #264101 - Attachment is obsolete: true
Attachment #289502 - Flags: review?
(Assignee)

Updated

11 years ago
Attachment #289502 - Flags: review? → review?(mano)

Comment 14

11 years ago
Comment on attachment 289502 [details] [diff] [review]
Use eTLD+1 instead of hostname

i can't resist doing a driveby review here ;)

>Index: browser/base/content/pageinfo/security.js
>===================================================================
>     if (win)
>       win.focus();

should we do anything with the window if it's already open (like fill the filter and call onFilterInput()), or just silently do nothing?

>+      var eTLD = eTLDService.getBaseDomainFromHost(this._getSecurityInfo()
>+                                                       .hostName);

please use the getBaseDomain() form with this._getSecurityInfo().fullLocation; it's preferred (and better!).

nice patch! r=me if you want it and consider it sufficient... ;)
(Assignee)

Comment 15

11 years ago
I love drive-by reviews!  The more eyes the better.

(In reply to comment #14)
> should we do anything with the window if it's already open (like fill the
> filter and call onFilterInput()), or just silently do nothing?

Good question - I thought about it, and left it off because I couldn't be sure that wouldn't be annoying - do people keep cookie windows around with state and want that to be blown away?  Maybe, I could be persuaded, but the omission was intentional.  :) 

> >+      var eTLD = eTLDService.getBaseDomainFromHost(this._getSecurityInfo()
> >+                                                       .hostName);
> 
> please use the getBaseDomain() form with this._getSecurityInfo().fullLocation;
> it's preferred (and better!).

fullLocation is a Location, not a URI, which means doing a conversion.  I assume that the reason getBaseDomain() is preferred is that it spared the nsIURI conversion in getBaseDomainFromHost(), but that doesn't help if I need to do a conversion to call the former, and not to call the latter.  :)  Am I misunderstanding the reason for preferring the one over the other, or did you do what I always do, and forget the Location != URI thing?  :)

> nice patch! r=me if you want it and consider it sufficient... ;)

Thanks!  You know *I* consider it sufficient, because I have the love, but I'll still wait on a module peer, purely for formality.  :)

Comment 16

11 years ago
(In reply to comment #15)
> Good question - I thought about it, and left it off because I couldn't be sure
> that wouldn't be annoying - do people keep cookie windows around with state and
> want that to be blown away?  Maybe, I could be persuaded, but the omission was
> intentional.  :) 

hmm. seems better to have them do it once and learn their lesson, than keep furiously clicking the button wondering if their firefox is on holiday - how would they figure out they need to close the dialog first? if they actually want to keep their state, they'll quickly learn "don't do that" :)

> fullLocation is a Location, not a URI, which means doing a conversion.

ah. i did miss that; i saw the code doing ".host" and... yeah. having said that, what is a Location? if it contains an nsIURI and gives that back, then it's still preferred since it'll avoid re-normalization in the eTLD backend. if not, no big deal.
Accel+Z and you get back to your previous search.
Comment on attachment 289502 [details] [diff] [review]
Use eTLD+1 instead of hostname

>? obj-i386-apple-darwin8.10.1
>Index: browser/base/content/pageinfo/security.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/base/content/pageinfo/security.js,v
>retrieving revision 1.9
>diff -u -8 -p -r1.9 security.js
>--- browser/base/content/pageinfo/security.js	13 Nov 2007 08:46:16 -0000	1.9
>+++ browser/base/content/pageinfo/security.js	20 Nov 2007 15:59:50 -0000
>@@ -123,19 +123,27 @@ var security = {
>    */
>   viewCookies : function()
>   {
>     var wm = Components.classes["@mozilla.org/appshell/window-mediator;1"]
>                        .getService(Components.interfaces.nsIWindowMediator);
>     var win = wm.getMostRecentWindow("Browser:Cookies");
>     if (win)
>       win.focus();

See previous comments.

nits:
>-    else
>+    else {
>+      // Pass in eTLD+1 instead of hostName
>+      var eTLDService = Cc["@mozilla.org/network/effective-tld-service;1"]
>+                        .getService(Ci.nsIEffectiveTLDService);

the dot should be at the end of the first line.

>+      var eTLD = eTLDService.getBaseDomainFromHost(this._getSecurityInfo()
>+                                                       .hostName);
>+
>       window.openDialog("chrome://browser/content/preferences/cookies.xul",
>-                        "Browser:Cookies", "");
>+                        "Browser:Cookies", "",
>+                        {filterString : eTLD});
>+    }

on one line please.

>Index: browser/components/preferences/cookies.js
>===================================================================

trailing spaces
Attachment #289502 - Flags: review?(mano) → review-
(Assignee)

Comment 19

11 years ago
Created attachment 289840 [details] [diff] [review]
Review comments addressed, update the cookie window if it's already open

(In reply to comment #18)
> >     if (win)
> >       win.focus();
> 
> See previous comments.

I've exposed setFilter on gCookiesWindow so that this code can update the existing cookie window as well.

> nits:
> >+      var eTLDService = Cc["@mozilla.org/network/effective-tld-service;1"]
> >+                        .getService(Ci.nsIEffectiveTLDService);
> 
> the dot should be at the end of the first line.

Done.

> >       window.openDialog("chrome://browser/content/preferences/cookies.xul",
> >-                        "Browser:Cookies", "");
> >+                        "Browser:Cookies", "",
> >+                        {filterString : eTLD});
> >+    }
> 
> on one line please.

Done.

> trailing spaces

I think I caught them all, this time.
Attachment #289502 - Attachment is obsolete: true
Attachment #289840 - Flags: review?(mano)

Updated

11 years ago
Duplicate of this bug: 405161
Comment on attachment 289840 [details] [diff] [review]
Review comments addressed, update the cookie window if it's already open

>Index: browser/components/preferences/cookies.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/preferences/cookies.js,v
>retrieving revision 1.15
>diff -u -8 -p -r1.15 cookies.js
>--- browser/components/preferences/cookies.js	15 Sep 2006 21:28:47 -0000	1.15
>+++ browser/components/preferences/cookies.js	22 Nov 2007 20:09:50 -0000
>@@ -58,16 +58,20 @@ var gCookiesWindow = {
>     this._bundle = document.getElementById("bundlePreferences");
>     this._tree = document.getElementById("cookiesList");
>     
>     this._loadCookies();
>     this._tree.treeBoxObject.view = this._view;
>     this.sort("rawHost");
>     if (this._view.rowCount > 0) 
>       this._tree.view.selection.select(0);
>+
>+    if ("arguments" in window && window.arguments.length > 0 &&
>+        window.arguments[0].filterString)
>+      this.setFilter(window.arguments[0].filterString);
>       
>     document.getElementById("filter").focus();
>   },
>   
>   uninit: function ()
>   {
>     var os = Components.classes["@mozilla.org/observer-service;1"]
>                        .getService(Components.interfaces.nsIObserverService);
>@@ -895,11 +899,17 @@ var gCookiesWindow = {
>       this.clearFilter();
>   },
>   
>   focusFilterBox: function ()
>   { 
>     var filter = document.getElementById("filter");
>     filter.focus();
>     filter.select();
>+  },
>+  

spaces ;)

r=mano otherwise.
Attachment #289840 - Flags: review?(mano) → review+
(Assignee)

Comment 22

11 years ago
Created attachment 291675 [details] [diff] [review]
Apply cleanly against trunk, remove trailing spaces (damnit)

Asking for approval to land this post-beta2 since it's wanted+, not blocking+
Attachment #289840 - Attachment is obsolete: true
Attachment #291675 - Flags: approval1.9?

Updated

11 years ago
Attachment #291675 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 23

11 years ago
Checking in browser/base/content/pageinfo/security.js;
/cvsroot/mozilla/browser/base/content/pageinfo/security.js,v  <--  security.js
new revision: 1.11; previous revision: 1.10
done
Checking in browser/components/preferences/cookies.js;
/cvsroot/mozilla/browser/components/preferences/cookies.js,v  <--  cookies.js
new revision: 1.17; previous revision: 1.16
done
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
The view button in Windows Vista appears to not do anything.  

Also see this in the Console2: 

Error: Cc is not defined
Source file: chrome://browser/content/pageinfo/security.js
Line: 101


Reopening per convo on IRC
(Assignee)

Comment 26

11 years ago
10:50 <littlemutt> I see this in the console2 error console 
10:50 <littlemutt> Error: Cc is not defined
10:50 <littlemutt> Source file: chrome://browser/content/pageinfo/security.js
10:50 <littlemutt> Line: 101
10:50 <johnath> oh dear me
10:50 <johnath> I mean, easy to fix, but wtf is that not defined for you - it really shouldn't be platform-specific
10:51 <littlemutt> dunno - I am on vista 
10:52 <johnath> Okay, hrm.  Wanna re-open the bug with that comment and I'll take a look soonest?
10:53 <littlemutt> k
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
If this was only tested on Mac, the Mac menu overlays probably defined Cc for you.
(Assignee)

Comment 28

11 years ago
Created attachment 292975 [details] [diff] [review]
Followup - s/Cc/Components.classes/

(In reply to comment #27)
> If this was only tested on Mac, the Mac menu overlays probably defined Cc for
> you.

Sigh.  Yes.  That would do it.
(Assignee)

Comment 29

11 years ago
Created attachment 292976 [details] [diff] [review]
Followup: And for that matter, let's fix Ci too

Someone should take away my keyboard pretty soon.
Attachment #292975 - Attachment is obsolete: true
(Assignee)

Comment 30

11 years ago
Tested on Vista & Mac with this version no errors logged, and bevaviour as expected. 

Checking in browser/base/content/pageinfo/security.js;
/cvsroot/mozilla/browser/base/content/pageinfo/security.js,v  <--  security.js
new revision: 1.12; previous revision: 1.11
done
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED

Comment 31

11 years ago
Created attachment 293308 [details] [diff] [review]
follow-up to fix crashes

first:
-----
window.arguments[0] is null when opening the cookie-manager via pref dialog.


second:
-------
don't crash and fail to open on about:blank

this._getSecurityInfo().hostName is null, if the current site is about:blank or any other about-page. nsEffectiveTLDService::GetBaseDomainInternal will return NS_ERROR_ILLEGAL_VALUE in such case.
Attachment #293308 - Flags: review?(mano)

Comment 32

11 years ago
Created attachment 293311 [details] [diff] [review]
follow-up to fix crashes (with removed trailing spaces)

removed trailing spaces -.-
Attachment #293308 - Attachment is obsolete: true
Attachment #293311 - Flags: review?(mano)
Attachment #293308 - Flags: review?(mano)
Johnath: Any reason we can click the View Cookies button when there is no cookies for the page?  I wonder if it's by design or if it's a bug.
Comment on attachment 293311 [details] [diff] [review]
follow-up to fix crashes (with removed trailing spaces)

r=mano, please file a bug on the crash in getBaseDomainFromHost.
Attachment #293311 - Flags: review?(mano) → review+
Comment on attachment 293311 [details] [diff] [review]
follow-up to fix crashes (with removed trailing spaces)

Fix crashes.
Attachment #293311 - Flags: approval1.9?
(In reply to comment #34)
> please file a bug on the crash in getBaseDomainFromHost.

It seems to be an exception, not a crash. Or Maybe I misunderstood comment 31?
patch names says "crash", i didn't check getBaseDomainFromHost code.
(Assignee)

Comment 38

11 years ago
(In reply to comment #33)
> Johnath: Any reason we can click the View Cookies button when there is no
> cookies for the page?  I wonder if it's by design or if it's a bug.

I agree it's a little odd, but the pre-populating we do in this bug (and the logic we use to answer the question about storing cookies) is imperfect.  The information presented answers the question of whether *this site* stores cookies, but obviously the site could host content (e.g. ads) from other sites, which cause cookies to be sent to those hosts, even if the answer here is "no".

If a user comes to this UI, and is interested in the security/privacy context of what they're doing, and decides that they want to manage their cookies, I don't see a lot of harm in always leaving that option available.  I don't think, for instance, that presence of the button in that context is likely to add to their confusion, or represent something unexpected and of unclear purpose.

I guess that's a long way of saying "by design," but if you disagree, we might want to take it into another bug anyhow.

(In reply to comment #36)
> (In reply to comment #34)
> > please file a bug on the crash in getBaseDomainFromHost.
> 
> It seems to be an exception, not a crash. Or Maybe I misunderstood comment 31?

I think both cases will be exceptions, not crashes, but it still makes sense to me to file a follow-up, to make it clear what happens in nsIEffectiveTLDService if a null host is provided.

Comment on attachment 293311 [details] [diff] [review]
follow-up to fix crashes (with removed trailing spaces)

a=beltzner for drivers

In the future, it would be best if this bug be marked REOPENED or a follow-up bug be filed.
Attachment #293311 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Checking in browser/components/preferences/cookies.js;
/cvsroot/mozilla/browser/components/preferences/cookies.js,v  <--  cookies.js
new revision: 1.18; previous revision: 1.17
done
Checking in browser/base/content/pageinfo/security.js;
/cvsroot/mozilla/browser/base/content/pageinfo/security.js,v  <--  security.js
new revision: 1.13; previous revision: 1.12
done
Keywords: checkin-needed
Target Milestone: --- → Firefox 3 M11

Comment 41

11 years ago
Comment on attachment 293311 [details] [diff] [review]
follow-up to fix crashes (with removed trailing spaces)

>Index: browser/components/preferences/cookies.js
>===================================================================
>+    var eTLD = "";
>+
>+    if (this._getSecurityInfo().hostName)
>+      eTLD = eTLDService.getBaseDomainFromHost(this._getSecurityInfo().hostName);

hm, you'd be far better off try/catching the result from getBaseDomainFromHost(), and then doing |eTLD = "";| if it fails. getBaseDomainFromHost() will also fail, for example, if hostName is an IP address - and in fact you might want to test for that failure case and set |eTLD = hostName|. (the nsresult will be NS_ERROR_HOST_IS_IP_ADDRESS.)

Comment 42

11 years ago
(In reply to comment #41)
> hm, you'd be far better off try/catching the result from
> getBaseDomainFromHost(), and then doing |eTLD = "";| if it fails.
> getBaseDomainFromHost() will also fail, for example, if hostName is an IP
> address - and in fact you might want to test for that failure case and set
> |eTLD = hostName|. (the nsresult will be NS_ERROR_HOST_IS_IP_ADDRESS.)
> 
Although I've looked at getBaseDomainFromHost, I did not saw it ...
Let's wait for a decision in bug 409147 and then check-in both patches together.


(In reply to comment #37)
> patch names says "crash", i didn't check getBaseDomainFromHost code.
> 

Sorry Mano, I meant "fail". Firefox does not crash. viewCookies() failed due to the unhandled exception from getBaseDomainFromHost in this case.

*note to myself: never say crash if fx doesn't crash*
Duplicate of this bug: 409882
verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9b3pre) Gecko/2008010705 Minefield/3.0b3pre ID:2008010705 and created also a litmus testcase.
Status: RESOLVED → VERIFIED
Flags: in-litmus? → in-litmus+
Flags: wanted-firefox3+
Whiteboard: [wanted-firefox3]

Updated

11 years ago
Duplicate of this bug: 415756

Updated

11 years ago
Depends on: 433959
You need to log in before you can comment on or make changes to this bug.