728647 - [OSX] browser_dataman_basics.js and browser_dataman_callviews.js fail, wrt IPv6

Status: VERIFIED FIXED

(SeaMonkey :: Passwords & Permissions, defect, P2)

Description

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1329619612.1329623931.5998.gz
OS X 10.5 comm-central-trunk debug test mochitest-other on 2012/02/18 18:46:52
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1329624302.1329629638.17173.gz
OS X 10.6 comm-central-trunk debug test mochitest-other on 2012/02/18 20:05:02
+
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Aurora/1329628564.1329632893.23153.gz
OS X 10.5 comm-aurora debug test mochitest-other on 2012/02/18 21:16:04
+
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Beta/1329513480.1329517113.13259.gz
OS X 10.6 comm-beta debug test mochitest-other on 2012/02/17 13:18:00
{
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/dataman/tests/browser_dataman_basics.js | IPv6 domain is selected - Got *, expected [::1]
[and many more]

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/dataman/tests/browser_dataman_callviews.js | Step 6: The correct domain is selected - Got [::1], expected *
[and a few more]
}

Comment 1

[Robert Kaiser]

Sounds like either the sorting has changed or some default entries have.

Comment 2

[Robert Kaiser]

On my Linux machine, all dataman tests pass, so this is either coming from somewhere else or it's OSX-specific.

Comment 3

[Robert Kaiser]

Still, this one:

TEST-INFO | chrome://mochitests/content/browser/suite/common/dataman/tests/browser_dataman_basics.js | run test #2 of 10 (test_forget_ipv6)
NEXT ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/dataman/tests/browser_dataman_basics.js | IPv6 domain is selected - Got *, expected [::1]

pretty much tells the story that the order in the domain pane is wrong. This only does a select for the second entry in the list, see http://mxr.mozilla.org/comm-central/source/suite/common/dataman/tests/browser_dataman_basics.js#150 and gets *, which should _always_ be the first item. Maybe I need to find some way to enforce that somewhat harder.

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to Robert Kaiser (:[e-mail]) from comment #2)
> On my Linux machine, all dataman tests pass, so this is either coming from
> somewhere else or it's OSX-specific.

This bug _is_ filed as OSX-specific.

Comment 5

[Robert Kaiser]

Attachment: v1: try fixing by force-sorting "*" as first entry

Comment 6

[Robert Kaiser]

Comment on attachment 600824 [details] [diff] [review]
v1: try fixing by force-sorting "*" as first entry

(hrm, my comment wasn't submitted, I messed up a bit when attaching this, so delivering the comment now)

This is my try at fixing this problem by always force-sorting "*" to the top of the list. The more I think about this, the more I think we should have done that from the beginning anyhow.

Comment 7

[Robert Kaiser]

Thanks, pushed as http://hg.mozilla.org/comm-central/rev/042808980445 - I hope it really fixed the tests (as I can't test on Mac).

Comment 8

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1330818089.1330824354.559.gz
OS X 10.6 comm-central-trunk debug test mochitest-other on 2012/03/03 15:41:29
still has same failures :-/

Trying to guess:
Could it be that this.sort() needs to be called from gDomains.initialize(), like some of the other *.initialize() do?

Comment 9

[Serge Gautherie (:sgautherie)]

(In reply to Robert Kaiser (:[e-mail]) from comment #7)
> I can't test on Mac

Any chance to add some debugging code, like (somehow) taking a (few) snapshot?


(In reply to Serge Gautherie (:sgautherie) from comment #8)
> Could it be that this.sort() needs to be called from gDomains.initialize(),
> like some of the other *.initialize() do?

Hum, maybe irrelevant, but...

Comment 10

[Robert Kaiser]

(In reply to Serge Gautherie (:sgautherie) from comment #9)
> (In reply to Robert Kaiser (:[e-mail]) from comment #7)
> > I can't test on Mac
> 
> Any chance to add some debugging code, like (somehow) taking a (few)
> snapshot?

I don't think that would be a good idea. Someone should try it on a Mac and tell us what (s)he sees.

> (In reply to Serge Gautherie (:sgautherie) from comment #8)
> > Could it be that this.sort() needs to be called from gDomains.initialize(),
> > like some of the other *.initialize() do?
> 
> Hum, maybe irrelevant, but...

It is irrelevant, sorting is called.

Comment 11

[Robert Kaiser]

Attachment: Correct earlier patch to do what it should do

D'oh. Here's the patch that actually makes it do the correct thing. Ian, I won't tell anyone you gave the first one your review if you don't tell anyone I wrote it, OK? (If we had looked the original .localeCompare-containing line a bit more careful, we would have spotted all along what was wrong with that patch.)

I checked with injecting an empty ("") domain in the list, which localeCompare sorts before "*" even on Linux, and could verify that it was wrong before the patch and correct after the patch. I still won't add that case to the test as it's hacky and we have Mac to verify the sorting is correct anyhow.

Comment 12

[Serge Gautherie (:sgautherie)]

Comment on attachment 604548 [details] [diff] [review]
Correct earlier patch to do what it should do

Obviously ;->

Comment 13

[Robert Kaiser]

OK, this should really do it now: http://hg.mozilla.org/comm-central/rev/e197041b4425 (thankfully still in time for 2.10, from what I see).

Comment 14

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1331684074.1331690325.18324.gz
OS X 10.6 comm-central-trunk debug test mochitest-other on 2012/03/13 17:14:34

V.Fixed

Comment 15

[Serge Gautherie (:sgautherie)]

Comment on attachment 604548 [details] [diff] [review]
Correct earlier patch to do what it should do

[Approval Request Comment]
For (the merge of) both patches.

Comment 16

[Justin Wood (:Callek)]

Comment on attachment 604548 [details] [diff] [review]
Correct earlier patch to do what it should do

I'll grant it, but I want Robert or IanN to decide explicitly that it is safe enough from their minds.

Comment 17

[Robert Kaiser]

I'm sure it's safe enough, but I wouldn't waste sleep over the test failures on beta, we should get some more green on trunk first.
That said, I've landed it still because I think not always sorting "*" to the top is a significant flaw and would be good to already have fixed in a 2.9 release.

http://hg.mozilla.org/releases/comm-beta/rev/9ac132949e97 (merged both patches to a single correct one)

Comment 18

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Beta/1331888181.1331892041.23235.gz
OS X 10.5 comm-beta debug test mochitest-other on 2012/03/16 01:56:21

seamonkey2.9: verified.


(In reply to Robert Kaiser (:[e-mail]) from comment #17)

> I'm sure it's safe enough, but I wouldn't waste sleep over the test failures
> on beta, we should get some more green on trunk first.

(Makes me wonder what I have been doing for the past 3 months...)

> That said, I've landed it still because I think not always sorting "*" to
> the top is a significant flaw and would be good to already have fixed in a
> 2.9 release.

(Iow, this is not a test fix but an application "fix"...)

Comment 19

[Robert Kaiser]

(In reply to Serge Gautherie (:sgautherie) from comment #18)
> (Makes me wonder what I have been doing for the past 3 months...)

A damn great job, AFAICT, given how much green there is already. :)

> (Iow, this is not a test fix but an application "fix"...)

Yes, found by the test (thanks for reporting) but ultimately a correctness fix in the code itself.