User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:2.0.1) Gecko/20110608 Firefox/4.0.1 SeaMonkey/2.1 Build Identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.0.1) Gecko/20110608 Firefox/4.0.1 SeaMonkey/2.1 If an IPv6 cookie is logged, Data Manager will display domains, but no data in any category (Cookies, Permissions, Preferences, Passwords), is displayed. Reproducible: Always Steps to Reproduce: 1. new Profile 2. visit a site that logs IPv6 cookies (Perhaps here, but I'm not sure: http://[2001:db8::1]/ ) (Or try URL, above, if you're on Comcast) 3. open Data Manager (about:data) --- These specific steps work for me: 1. new Profile 2a. open http://test-ipv6.comcast.net/ 2b. open chrome://communicator/content/permissions/cookieViewer.xul in a tab 2c. open http://www.seamonkey-project.org/releases/seamonkey2.1/ in a tab 3. open Data Manager (about:data) Actual Results: Data Manager opens. Domains are listed. No data of any type (Cookies, Permissions, Preferences, Passwords) is listed for any domain. Expected Results: Data should be listed for every domain. chrome://communicator/content/permissions/cookieViewer.xul does load correctly & does show all cookies, including IPv6 cookies. For me, this page seems to set a IPv6 cookie, http://test-ipv6.comcast.net/ . Error Console: ======= Error: uncaught exception: [Exception... "Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIURLParser.parseURL]" nsresult: "0x804b000a (NS_ERROR_MALFORMED_URI)" location: "JS frame :: chrome://communicator/content/dataman/dataman.js :: domain_getDomainFromHost :: line 460" data: no] ======= A useful utility: "MozillaCookiesView: Cookies Manager For Mozilla/Firefox/Netscape Browsers" http://www.nirsoft.net/utils/mzcv.html

per http://www-archive.mozilla.org/projects/netlib/cookies/cookie-log.html

Are you connected via IPv6? As I don't see this issue via your steps.

Hmm, would be cool if we could add a case about those in our automated tests. Could you provide a safe IPv6 address/domain name to use there? I think we also should add a IPv4 case in the tests, Data Manager was not designed with any plain-IP-address cases in mind, but we should ensure they work reasonably.

IPv6 is enabled, though I don't think I can actually connect to anything IPv6. (As in some parts are there, but my modem/router or whatever are not aware.) Nonetheless, my steps using the Comcast URL (I am on Comcast) are sufficient for an IPv6 cookie to be generated, logged.

My Comcast IPv6 "Readiness" report.

This patch fixes support for IPv6 URLs, makes Data Manager tolerant against other problems that could make the URL parser throw, and ensures that we don't double-delete entries in lists (happened when first reacting to the data delete and then removing another entry from the list, fixed by first removing from list as reactToChange is tolerant against this - uncovered by the test I wrote for the IPv6 stuff). All bugs this fixes are IMHO grave enough that we should drive this patch even into beta.

Oh, and I switched the test to using a setTimeout just so console messages will be handled interleaved with the test messages, but as this is event-driven anyhow, it doesn't change the actual test.

I realized that the setTimeout() change in the test also needs to be finish() called with setTimeout() so that it isn't called before the last test step (i.e. closing the Data Manager) is closed.

Comment on attachment 548060 [details] [diff] [review] v1.1: fix IPv6 and be more tolerant against errors >diff --git a/suite/common/dataman/tests/browser_dataman_basics.js b/suite/common/dataman/tests/browser_dataman_basics.js >--- a/suite/common/dataman/tests/browser_dataman_basics.js >+++ b/suite/common/dataman/tests/browser_dataman_basics.js >@@ -84,37 +93,37 @@ function test() { > Services.obs.addObserver(testObs, TEST_DONE, false); > // Trigger the first test now! > Services.obs.notifyObservers(window, TEST_DONE, null); > } > else { > // TEST_DONE triggered, run next test > info("run test #" + (testIndex + 1) + " of " + testFuncs.length + > " (" + testFuncs[testIndex].name + ")"); >- testFuncs[testIndex++](win); >+ setTimeout(testFuncs[testIndex++], 0, win); > > if (testIndex >= testFuncs.length) { > // Finish this up! > Services.obs.removeObserver(testObs, TEST_DONE); > Services.cookies.removeAll(); > gLocSvc.fhist.removeAllEntries(); >- finish(); >+ setTimeout(finish, 0); > } > } > } Nit pull the |// Finish this up!| section to be its own "test func" that is executed at the end of everything (and add a comment for the "close dataMan" and "clean up" functions must be last) That should make sure we don't remove the observer for TEST_DONE too early, and ensure that we don't confuse knowledge here. (no need to test testIndex//testFuncs.length if we stuff it at the end anyway) and all vars referenced in that block are globals, so we should be in great shape, to not even need an explicit setTimeout.

(In reply to comment #10) > That should make sure we don't remove the observer for TEST_DONE too early, > and ensure that we don't confuse knowledge here. (no need to test > testIndex//testFuncs.length if we stuff it at the end anyway) and all vars > referenced in that block are globals, so we should be in great shape, to not > even need an explicit setTimeout. The last testFuncs element is already test_close, which is doing nothing but closing the Data Manager, so we already assert what you want with this.

Comment on attachment 548060 [details] [diff] [review] v1.1: fix IPv6 and be more tolerant against errors Doing a code review first. >+++ b/suite/common/dataman/dataman.js >+ try { >+ var schemePos = {}, schemeLen = {}, authPos = {}, authLen = {}, pathPos = {}, >+ pathLen = {}, usernamePos = {}, usernameLen = {}, passwordPos = {}, >+ passwordLen = {}, hostnamePos = {}, hostnameLen = {}, port = {}; Do we really need to move these into the try? >+ gLocSvc.url.parseURL(aHostname, -1, schemePos, schemeLen, authPos, authLen, >+ pathPos, pathLen); Nit: indentation >+ var auth = aHostname.substring(authPos.value, authPos.value + authLen.value); >+ gLocSvc.url.parseAuthority(auth, authLen.value, usernamePos, usernameLen, >+ passwordPos, passwordLen, hostnamePos, hostnameLen, port); Nit: indentation >+ hostName = auth.substring(hostnamePos.value, hostnamePos.value + hostnameLen.value); >+ } >+ catch (e) { >+ // IPv6 host names can come in without [] around them and therefore >+ // cause an error. Those consist of at least two colons and else only >+ // hexadecimal digits. Fix them by putting [] around them. >+ if (/^[a-f0-9]*:[a-f0-9]*:[a-f0-9:]*$/.test(aHostname)) { >+ gDataman.debugMsg("bare IPv6 address found: " + aHostname); >+ hostName = "[" + aHostname + "]"; >+ } I guess the bare IPv6 is fairly rare at the moment, so is okay sitting in the catch. I'll do some testing next.

When we can see it in a binary code?

(In reply to comment #14) > When we can see it in a binary code? Never, as nothing of this is in a language that is compiled to binary. Still, what you probably mean is when it will be in any builds you can use, and the only thing I can tell you there is when Ian's review is done and I'll write here that it's "checked in". I hope that it will make SeaMonkey 2.3 which is due for mid to late August.

Comment on attachment 548060 [details] [diff] [review] v1.1: fix IPv6 and be more tolerant against errors r=me with those issues addressed Is the notifyObserver call in test_close really needed as I believe the observer is removed just after the function is called? Any other test enhancements/simplifications should probably be spun off into another bug.

Pushed: http://hg.mozilla.org/comm-central/rev/1461665d8d73 http://hg.mozilla.org/releases/comm-aurora/rev/8965e28225cc http://hg.mozilla.org/releases/comm-beta/rev/afe1e1cd3d5d

All works good. Big Thanks!

Confirmed working. With an IPv6 cookie in existence, Data Manager is now showing expected data. Mozilla/5.0 (Windows NT 6.1; rv:6.0) Gecko/20110731 Firefox/6.0 SeaMonkey/2.3