534647 - mochitest-browser-chrome: perma/random "browser_ApplicationPrefs.js | Timed out" after bug 152526 landing, caused by browser_bug431826.js

Status: VERIFIED FIXED

(SeaMonkey :: Testing Infrastructure, defect)

Description

[Serge Gautherie (:sgautherie)]

Bug 152526 comment 31:
{
Serge Gautherie (:sgautherie)      2009-12-02 07:40:12 PST

[
TEST-PASS |
chrome://mochikit/content/browser/suite/smile/test/browser_ApplicationPrefs.js
| A single preference should not be locked.
TEST-UNEXPECTED-FAIL |
chrome://mochikit/content/browser/suite/smile/test/browser_ApplicationPrefs.js
| Timed out
]
on all platforms.
}

Neil and I have already pushed a few fixes in other bugs.
The situation has improved, but our unit tests tinderboxes are still perma-orange, except Windows which is now random-orange only.

Needs to continue investigations to find out which other test(s) is interfering with this one.

Comment 1

[Robert Kaiser]

Same as bug 533290?

In any case, this is annoying, any idea what could be going on there?

Comment 2

[Serge Gautherie (:sgautherie)]

(In reply to comment #1)

> Same as bug 533290?

Oh, sure: I would I knew if Neil had linked that bug :-/

> In any case, this is annoying, any idea what could be going on there?

It seems to be +/- explained there!

Comment 3

[[e-mail]]

Assuming bug 488587 is the cause, it's a pity that Firefox passes :-( Perhaps they never use Application.prefs in their code?

Comment 4

[Serge Gautherie (:sgautherie)]

Yeah, I plan on asking for a test to be added when we know for sure what the actual cause is...

Comment 5

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.3a1pre) Gecko/20091218
SeaMonkey/2.1a1pre] (home, optim default) (W2Ksp4)

Bug still there: not fixed by bug 533355.

Comment 6

[Serge Gautherie (:sgautherie)]

(In reply to comment #0)
> Need to continue investigations to find out which other test(s) is interfering
> with this one.

I tried with my comment 5 build, but it's tedious as this failure is now intermittent on Windows.

Helpwanted: Could someone else do it on Linux or MacOSX?

Comment 7

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Disable test on non-Windows ftb, Improve a few log messages [Checkin: See comment 41 & 42]

Our tree has been perma-orange for too long already...

Comment 8

[[e-mail]]

Does browser_bug463504.js run before or after browser_ApplicationPrefs.js? If it runs first then it might be inadvertently affecting the test.

Comment 9

[Serge Gautherie (:sgautherie)]

(In reply to comment #8)

chrome://mochikit/content/browser/suite/common/tests/browser/browser_bug463504.js
does run before
chrome://mochikit/content/browser/suite/smile/test/browser_ApplicationPrefs.js

(In reply to comment #6)
> Helpwanted: Could someone else do it on Linux or MacOSX?

Still applies: I'll delay checking in a little...

Comment 10

[[e-mail]]

(In reply to comment #9)
>(In reply to comment #8)
>chrome://mochikit/content/browser/suite/common/tests/browser/browser_bug463504.js
>does run before
>chrome://mochikit/content/browser/suite/smile/test/browser_ApplicationPrefs.js
Well that makes it a possible "culprit". Could we try temporarily disabling it?

Comment 11

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) Rename (SM) browser_bug463504.js to (FF) browser_bug431826.js, Fix sort order [Checkin: Comment 17+18]

I missed to do that previously: it makes it more obvious which files should be in sync'.


(In reply to comment #10)
> Well that makes it a possible "culprit". Could we try temporarily disabling it?

This reminded me of:

Bug 533176 comment 1:
{
Serge Gautherie (:sgautherie)      2009-12-06 17:00:59 PST

I'll post a workaround I have for that if need be, but I prefer to leave this
"unrelated" issue for later...
}

Before attaching a patch with that workaround,
I would just push the current patch on branch then try |$(warning browser_bug431826.js)|.

Comment 12

[[e-mail]]

(In reply to comment #11)
> I missed to do that previously: it makes it more obvious which files should be
> in sync'.
Which files are already in sync? None of those other five appear to be.

Comment 13

[Serge Gautherie (:sgautherie)]

Comment on attachment 420302 [details] [diff] [review]
(Bv1) Rename (SM) browser_bug463504.js to (FF) browser_bug431826.js, Fix sort order
[Checkin: Comment 17+18]


(In reply to comment #12)
> Which files are already in sync? None of those other five appear to be.

Argh, the following part doesn't show up in bugzilla diff :-/
{
diff --git a/suite/common/tests/browser/browser_bug463504.js b/suite/common/tests/browser/browser_bug431826.js
rename from suite/common/tests/browser/browser_bug463504.js
rename to suite/common/tests/browser/browser_bug431826.js
}

Comment 14

[[e-mail]]

I'm not sure what you mean there; I saw the rename, but I don't understand why.

Comment 15

[Serge Gautherie (:sgautherie)]

Ah, well,
http://mxr.mozilla.org/comm-central/source/suite/common/tests/browser/browser_bug463504.js
should stay in sync with
http://mxr.mozilla.org/mozilla-central/source/browser/components/certerror/test/browser_bug431826.js
as much as possible.
Having same name makes that more obvious, ease MXR file search, ...

I didn't check the other tests (names): I'm just aligning/sorting them.

Comment 16

[[e-mail]]

Comment on attachment 420302 [details] [diff] [review]
(Bv1) Rename (SM) browser_bug463504.js to (FF) browser_bug431826.js, Fix sort order
[Checkin: Comment 17+18]

I'm still unconvinced either way, so here's hoping KaiRo can decide for me ;-)

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 420302 [details] [diff] [review]
(Bv1) Rename (SM) browser_bug463504.js to (FF) browser_bug431826.js, Fix sort order
[Checkin: Comment 17+18]


http://hg.mozilla.org/comm-central/rev/6f191f4def81
http://hg.mozilla.org/releases/comm-1.9.1/rev/f17cff9471d9

Comment 18

[Serge Gautherie (:sgautherie)]

Comment on attachment 420302 [details] [diff] [review]
(Bv1) Rename (SM) browser_bug463504.js to (FF) browser_bug431826.js, Fix sort order
[Checkin: Comment 17+18]


http://hg.mozilla.org/comm-central/rev/ebda35e9d815
http://hg.mozilla.org/releases/comm-1.9.1/rev/2b9df1f26d4e
(Cv1) Bustage fix

Comment 19

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) browser_bug431826.js: work around extra about:blank load event

This is the workaround I was talking about in bug 533176 comment 1.
I can't reproduce the random failure on my local Windows anymore :-/

Before pushing patch A, I'd like to try this one:
it would be even better if someone could try it locally on Linux/MacOSX...

Comment 20

[[e-mail]]

Comment on attachment 421436 [details] [diff] [review]
(Cv1) browser_bug431826.js: work around extra about:blank load event

>   gBrowser.selectedTab = gBrowser.addTab();
Is this the "about:blank" addTab bug?

Comment 21

[[e-mail]]

(The tests pass in my trunk Linux VM)

Comment 22

[Serge Gautherie (:sgautherie)]

(In reply to comment #20)
> Is this the "about:blank" addTab bug?

I don't know: what is that bug?

(In reply to comment #21)
> (The tests pass in my trunk Linux VM)

Good. With or without my patch?
Though I'm most interested in SeaMonkey 2.0 (tinderboxes)...

Comment 23

[[e-mail]]

(In reply to comment #22)
> (In reply to comment #20)
> > Is this the "about:blank" addTab bug?
> I don't know: what is that bug?
That's the bug where calling addTab("about:blank") doesn't load about:blank (because it's the default) but calling addTab() does. It got fixed on trunk.

> (In reply to comment #21)
> > (The tests pass in my trunk Linux VM)
> Good. With or without my patch?
Without. Also my VM only has a trunk Linux build.

Comment 24

[Serge Gautherie (:sgautherie)]

(In reply to comment #11)
> (In reply to comment #10)
> > Could we try temporarily disabling it?
> 
> I would [...] try |$(warning browser_bug431826.js)|.

http://hg.mozilla.org/releases/comm-1.9.1/rev/a783a17de6cd
(Dv1-191) Disable browser_bug431826.js.

Comment 25

[Serge Gautherie (:sgautherie)]

(In reply to comment #23)
> That's the bug where calling addTab("about:blank") doesn't load about:blank
> (because it's the default) but calling addTab() does. It got fixed on trunk.

That would be bug 536940 ;-)

Comment 26

[Serge Gautherie (:sgautherie)]

(In reply to comment #24)
> http://hg.mozilla.org/releases/comm-1.9.1/rev/a783a17de6cd
> (Dv1-191) Disable browser_bug431826.js.

Backout after confirmation:
http://hg.mozilla.org/releases/comm-1.9.1/rev/0f09c0194431
http://hg.mozilla.org/releases/comm-1.9.1/rev/ea93572525a5

Comment 27

[[e-mail]]

So what happens is that browser_bug463504^H^H^H^H^H^H431826.js invokes Application.prefs which registers a weak preference observer. XPConnect manages to forget that there's still a reference to it and allows the xptcstub to get cycle collected between the two tests, if cycle collection runs. Then when browser_ApplicationPrefs.js runs it fails because the observer won't fire.

I filed the xptcstub cycle collection bug as bug 533290.

Comment 28

[Robert Kaiser]

Hmm, from the description, the problem sounds as if Firefox would have to run into it as well - any idea why they don't?

Comment 29

[[e-mail]]

(In reply to comment #28)
> Hmm, from the description, the problem sounds as if Firefox would have to run
> into it as well - any idea why they don't?
To run in to the problem, the XPConnect stub for the Application._prefs object has to be garbage collected before the test completes. So it depends on whether any of their other tests use Application._prefs and in which order.

Comment 30

[Serge Gautherie (:sgautherie)]

Comment on attachment 421436 [details] [diff] [review]
(Cv1) browser_bug431826.js: work around extra about:blank load event


Bug 536940 landed on c-1.9.1, "but" this failure is still there.
I'd like to try(!) this workaround (on branch) nontheless: I'll back it out if it doesn't help at all.

Comment 31

[[e-mail]]

Comment on attachment 421436 [details] [diff] [review]
(Cv1) browser_bug431826.js: work around extra about:blank load event

Sorry, but I don't see the how this patch relates to the browser_ApplicationPrefs.js test failure.

Comment 32

[Serge Gautherie (:sgautherie)]

(In reply to comment #31)

This workaround helped on my local Windows, before a few other issues were fixed...
Now, do you prefer that I re-disable browser_bug431826.js or land patch A?

Comment 33

[[e-mail]]

You don't have to disable browser_bug431826.js, just patch it not to use Application.prefs, which is what's causing the problem.

Comment 34

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1) Revert to using "gPrefService" in browser_bug431826.js ftb [Checkin: See comment 38 & 36]

Per your comment 33:
this comments out part of the change from bug 533210 and reverts code to be like Firefox.

Comment 35

[[e-mail]]

Comment on attachment 423002 [details] [diff] [review]
(Dv1) Revert to using "gPrefService" in browser_bug431826.js ftb
[Checkin: See comment 38 & 36]

>+// Workaround until Application.prefs can be used safely. (Bug 534647)
>+var gPrefService =
>+  Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch);
Nit: declare in full, like browser_bug515006/524345 please.

Comment 36

[Serge Gautherie (:sgautherie)]

Comment on attachment 423002 [details] [diff] [review]
(Dv1) Revert to using "gPrefService" in browser_bug431826.js ftb
[Checkin: See comment 38 & 36]


http://hg.mozilla.org/releases/comm-1.9.1/rev/b3c0757d7437
Dv1, with comment 35 suggestion(s).

Comment 37

[Serge Gautherie (:sgautherie)]

Attachment: (Fv1-FF) Improve a few log messages [Checkin: Comment 40 & 44 & 45]

Keep FF and SM in sync'.

Comment 38

[Serge Gautherie (:sgautherie)]

Comment on attachment 423002 [details] [diff] [review]
(Dv1) Revert to using "gPrefService" in browser_bug431826.js ftb
[Checkin: See comment 38 & 36]


http://hg.mozilla.org/comm-central/rev/0e2fd3b00c96

Comment 39

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 423121 [details] [diff] [review]
(Fv1-FF) Improve a few log messages
[Checkin: Comment 40 & 44 & 45]

This seems like a waste of time. Whether messages are positive or negative hardly matters, as long as they're unique enough to find the correct failure case.

Comment 40

[Serge Gautherie (:sgautherie)]

Comment on attachment 423121 [details] [diff] [review]
(Fv1-FF) Improve a few log messages
[Checkin: Comment 40 & 44 & 45]


http://hg.mozilla.org/mozilla-central/rev/56a74ab99f43

Comment 41

[Serge Gautherie (:sgautherie)]

Comment on attachment 420056 [details] [diff] [review]
(Av1) Disable test on non-Windows ftb, Improve a few log messages
[Checkin: See comment 41 & 42]


http://hg.mozilla.org/comm-central/rev/80f4c1737e2b
'Improve a few log messages' part only.

Comment 42

[Serge Gautherie (:sgautherie)]

Comment on attachment 420056 [details] [diff] [review]
(Av1) Disable test on non-Windows ftb, Improve a few log messages
[Checkin: See comment 41 & 42]


http://hg.mozilla.org/releases/comm-1.9.1/rev/19f1cc8894f3

Comment 43

[Serge Gautherie (:sgautherie)]

V.Fixed, per tinderboxes.

Comment 44

[Serge Gautherie (:sgautherie)]

Comment on attachment 420056 [details] [diff] [review]
(Av1) Disable test on non-Windows ftb, Improve a few log messages
[Checkin: See comment 41 & 42]


http://hg.mozilla.org/releases/mozilla-1.9.2/rev/cc946b1b0fba

Comment 45

[Serge Gautherie (:sgautherie)]

Comment on attachment 423121 [details] [diff] [review]
(Fv1-FF) Improve a few log messages
[Checkin: Comment 40 & 44 & 45]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c4a9da11f7e4

Comment 46

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #45)
> http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c4a9da11f7e4

Since the check-in the tree didn't turn orange due to this described issue. Marking as verified1.9.1.