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

Status: VERIFIED FIXED

(Firefox :: Page Info Window, defect)

Description

[Johnathan Nightingale [:johnath]]

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

Comment 1

[[e-mail]]

johnath, are you working on this? i can help with some backend fu here.

Comment 2

[Johnathan Nightingale [:johnath]]

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

Comment 3

[:Gavin Sharp [email: [e-mail]]]

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

Comment 4

[:Gavin Sharp [email: [e-mail]]]

(bug 367446 would also help here, I think)

Comment 5

[Johnathan Nightingale [:johnath]]

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

[Steffen Wilberg]

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

[Steffen Wilberg]

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?

Comment 8

[[e-mail]]

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.

Comment 9

[Johnathan Nightingale [:johnath]]

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.

Comment 11

[Carsten Book [:Tomcat]]

Litmus Triage Team: Tomcat will add the test case in Litmus.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Not blocking, but very nice to have.

Comment 13

[Johnathan Nightingale [:johnath]]

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

Comment 14

[[e-mail]]

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... ;)

Comment 15

[Johnathan Nightingale [:johnath]]

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

[[e-mail]]

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

Comment 17

[Asaf Romano (gone)]

Accel+Z and you get back to your previous search.

Comment 18

[Asaf Romano (gone)]

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

Comment 19

[Johnathan Nightingale [:johnath]]

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

Comment 21

[Asaf Romano (gone)]

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.

Comment 22

[Johnathan Nightingale [:johnath]]

Attachment: Apply cleanly against trunk, remove trailing spaces (damnit)

Asking for approval to land this post-beta2 since it's wanted+, not blocking+

Comment 23

[Johnathan Nightingale [:johnath]]

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

Comment 24

[Jim Jeffery not reading bug-mail 1/2/11]

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

Comment 25

[Jim Jeffery not reading bug-mail 1/2/11]

Reopening per convo on IRC

Comment 26

[Johnathan Nightingale [:johnath]]

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

Comment 27

[:Gavin Sharp [email: [e-mail]]]

If this was only tested on Mac, the Mac menu overlays probably defined Cc for you.

Comment 28

[Johnathan Nightingale [:johnath]]

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

Comment 29

[Johnathan Nightingale [:johnath]]

Attachment: Followup: And for that matter, let's fix Ci too

Someone should take away my keyboard pretty soon.

Comment 30

[Johnathan Nightingale [:johnath]]

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

Comment 31

[Ronny Perinke]

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

Comment 32

[Ronny Perinke]

Attachment: follow-up to fix crashes (with removed trailing spaces)

removed trailing spaces -.-

Comment 33

[Florian Quèze [:florian]]

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 34

[Asaf Romano (gone)]

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.

Comment 35

[Reed Loden [:reed]]

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

Fix crashes.

Comment 36

[Florian Quèze [:florian]]

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

Comment 37

[Asaf Romano (gone)]

patch names says "crash", i didn't check getBaseDomainFromHost code.

Comment 38

[Johnathan Nightingale [:johnath]]

(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 39

[Mike Beltzner [:beltzner, not reading bugmail]]

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.

Comment 40

[Reed Loden [:reed]]

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

Comment 41

[[e-mail]]

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

[Ronny Perinke]

(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*

Comment 44

[Carsten Book [:Tomcat]]

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.