Last Comment Bug 665826 - Make Data Manager safe for IPv6 (and more tolerant against other errors)
: Make Data Manager safe for IPv6 (and more tolerant against other errors)
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Passwords & Permissions (show other bugs)
: unspecified
: x86 Windows 7
: -- major (vote)
: seamonkey2.5
Assigned To: Robert Kaiser
:
Mentors:
http://test-ipv6.comcast.net/
: 635783 (view as bug list)
Depends on: DataManager
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-20 20:34 PDT by therube
Modified: 2011-08-03 22:54 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed


Attachments
Cookie Log (45.54 KB, text/plain)
2011-06-20 20:50 PDT, therube
no flags Details
Screenshot of sample cookies. (24.77 KB, image/png)
2011-06-20 21:14 PDT, therube
no flags Details
Screenshot. (24.95 KB, image/png)
2011-06-21 07:00 PDT, therube
no flags Details
v1: fix IPv6 and be more tolerant against errors (11.67 KB, patch)
2011-07-23 15:19 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
v1.1: fix IPv6 and be more tolerant against errors (11.68 KB, patch)
2011-07-24 17:25 PDT, Robert Kaiser
iann_bugzilla: review+
iann_bugzilla: approval‑comm‑aurora+
iann_bugzilla: approval‑comm‑beta+
Details | Diff | Splinter Review

Description therube 2011-06-20 20:34:37 PDT
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
Comment 2 therube 2011-06-20 21:14:11 PDT
Created attachment 540674 [details]
Screenshot of sample cookies.
Comment 3 Ian Neal (Away until 7th Aug) 2011-06-21 03:35:00 PDT
Are you connected via IPv6? As I don't see this issue via your steps.
Comment 4 Robert Kaiser 2011-06-21 05:12:31 PDT
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.
Comment 5 therube 2011-06-21 06:59:14 PDT
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.
Comment 6 therube 2011-06-21 07:00:19 PDT
Created attachment 540736 [details]
Screenshot.

 
My Comcast IPv6 "Readiness" report.
Comment 7 Robert Kaiser 2011-07-23 15:19:50 PDT
Created attachment 547962 [details] [diff] [review]
v1: fix IPv6 and be more tolerant against errors

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.
Comment 8 Robert Kaiser 2011-07-23 15:21:36 PDT
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.
Comment 9 Robert Kaiser 2011-07-24 17:25:43 PDT
Created attachment 548060 [details] [diff] [review]
v1.1: fix IPv6 and be more tolerant against errors

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 10 Justin Wood (:Callek) 2011-07-24 18:53:51 PDT
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.
Comment 11 Robert Kaiser 2011-07-25 04:15:06 PDT
(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 12 Robert Kaiser 2011-07-25 12:27:19 PDT
*** Bug 635783 has been marked as a duplicate of this bug. ***
Comment 13 Ian Neal (Away until 7th Aug) 2011-07-26 06:36:57 PDT
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.
Comment 14 Alex 2011-07-26 10:55:14 PDT
When we can see it in a binary code?
Comment 15 Robert Kaiser 2011-07-26 12:21:42 PDT
(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 16 Ian Neal (Away until 7th Aug) 2011-07-27 09:47:34 PDT
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.
Comment 18 Alex 2011-08-03 02:41:18 PDT
All works good. Big Thanks!
Comment 19 therube 2011-08-03 16:21:55 PDT
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

Note You need to log in before you can comment on or make changes to this bug.