The default bug view has changed. See this FAQ.

Make Data Manager safe for IPv6 (and more tolerant against other errors)

RESOLVED FIXED in seamonkey2.5

Status

SeaMonkey
Passwords & Permissions
--
major
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: therube, Assigned: Robert Kaiser)

Tracking

unspecified
seamonkey2.5
x86
Windows 7

SeaMonkey Tracking Flags

(seamonkey2.3 fixed, seamonkey2.4 fixed)

Details

(URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

6 years ago
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
(Reporter)

Comment 1

6 years ago
Created attachment 540672 [details]
Cookie Log

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

Updated

6 years ago
Depends on: 569341
(Reporter)

Comment 2

6 years ago
Created attachment 540674 [details]
Screenshot of sample cookies.

Comment 3

6 years ago
Are you connected via IPv6? As I don't see this issue via your steps.
(Assignee)

Comment 4

6 years ago
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.
(Reporter)

Comment 5

6 years ago
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.
(Reporter)

Comment 6

6 years ago
Created attachment 540736 [details]
Screenshot.

 
My Comcast IPv6 "Readiness" report.
(Assignee)

Updated

6 years ago
Assignee: nobody → kairo
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Summary: Data Manager about:data is not IPv6 ready? → Make Data Manager safe for IPv6 (and more tolerant against other errors)
(Assignee)

Comment 7

6 years ago
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.
Attachment #540672 - Attachment is obsolete: true
Attachment #540674 - Attachment is obsolete: true
Attachment #540736 - Attachment is obsolete: true
Attachment #547962 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 8

6 years ago
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.
(Assignee)

Comment 9

6 years ago
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.
Attachment #547962 - Attachment is obsolete: true
Attachment #548060 - Flags: review?(iann_bugzilla)
Attachment #547962 - Flags: review?(iann_bugzilla)
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.
(Assignee)

Comment 11

6 years ago
(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.
(Assignee)

Updated

6 years ago
Duplicate of this bug: 635783

Comment 13

6 years ago
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

6 years ago
When we can see it in a binary code?
(Assignee)

Comment 15

6 years ago
(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

6 years ago
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.
Attachment #548060 - Flags: review?(iann_bugzilla) → review+

Updated

6 years ago
Attachment #548060 - Flags: approval-comm-beta+
Attachment #548060 - Flags: approval-comm-aurora+
(Assignee)

Comment 17

6 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.3
(Assignee)

Updated

6 years ago
status-seamonkey2.3: --- → fixed
status-seamonkey2.4: --- → fixed

Comment 18

6 years ago
All works good. Big Thanks!
(Reporter)

Comment 19

6 years ago
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

Updated

6 years ago
Target Milestone: seamonkey2.3 → seamonkey2.5
You need to log in before you can comment on or make changes to this bug.