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

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

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)

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

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?

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.

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.

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

Not blocking, but very nice to have.

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

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

(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

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

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

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

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

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

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

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

Someone should take away my keyboard pretty soon.

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

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.

removed trailing spaces -.-

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.

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

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

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

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

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

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.