Perma orange on Linux debug: TEST-UNEXPECTED-FAIL | test-account-settings-infrastructure.js::test_account_onchange_handler

RESOLVED FIXED in Thunderbird 25.0

Status

Thunderbird
Account Manager
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: standard8, Assigned: hiro)

Tracking

({intermittent-failure})

Trunk
Thunderbird 25.0
All
Linux
intermittent-failure

Thunderbird Tracking Flags

(thunderbird24 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 years ago
Since bug 804091 landed, we've been seeing this failure pretty much permanently on Linux debug builds, and occasionally on the opt builds, so I suspect this is a timing issue:

SUMMARY-UNEXPECTED-FAIL | test-account-settings-infrastructure.js | test-account-settings-infrastructure.js::test_account_onchange_handler
  EXCEPTION: a != b: '30' != '5'.
    at: test-folder-display-helpers.js line 2805
       assert_true test-folder-display-helpers.js 2805
       assert_equals test-folder-display-helpers.js 2792
       subtest_check_onchange_handler test-account-settings-infrastructure.js 403
       Runner.prototype.wrapper frame.js 577
       WindowWatcher_notify test-window-helpers.js 327

https://tbpl.mozilla.org/php/getParsedLog.php?id=21190304&tree=Thunderbird-Trunk#error0
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)

Comment 6

4 years ago
Yeah, we tried to make the timing correct by adding some 'wait for' stuff, but it may not be enough on linux. May be similar to the popups on linux problem.
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Assignee)

Comment 93

4 years ago
I've been running the failure test on my local linux box.

So, onAutosyncChange is not invoked even if tree.focus() is invoked. A similar issue is described in bug 530054. Is it related to this?
(Assignee)

Comment 94

4 years ago
Created attachment 767669 [details] [diff] [review]
A workaround

I am not sure toolkit has the bug related to this issue, but we need to deal with this orange anyway.

This workaround invokes onAutosyncChange() explicitly so that preference values are surely updated. I tried the failed test with this workaround 10 times on my local, and the test never failed.
Assignee: nobody → hiikezoe
Status: NEW → ASSIGNED
Attachment #767669 - Flags: review?(mbanner)

Comment 95

4 years ago
Thanks. But I can run the test on linux 10 times even without the workaround and I get no failure. It is simply random. Also the whole point of this test it to check if the onChange handler is fired automatically. So running it explicitly defeats the purpose of the test. But if you just want to silence the test then it could work. But then who comes back to fix it later.

I don't know if bug 530054 is related. It is reported on Windows (while this test fails only on linux). We have explicit focus() call in the code to ensure the focus has changed away from the iframe. The bug looks like it talks about somehow unfocusing (and running onchange) automatically.
(Assignee)

Comment 96

4 years ago
(In reply to :aceman from comment #95)

> Thanks. But I can run the test on linux 10 times even without the workaround
> and I get no failure. It is simply random. Also the whole point of this test
> it to check if the onChange handler is fired automatically. 

The test almost fails on my linux box without the workaround.

As far as I read the test code I can not see the check for onChange hander. 
The code does check that autosyncValue is surely updated when switching preference pages,
junk and offline. Am I missing something?

Comment 97

4 years ago
onAutosyncChange() is called from onchange event (http://hg.mozilla.org/comm-central/file/6f04beca8b73/mailnews/base/prefs/content/am-offline.xul#l67)
It changes the value of the textbox. And the test assumes the onchange was fired and checks if the textbox has the right value after that. By comparing the before and after value it checks if onchange was fired properly. Maybe we need to wait a bit somewhere?
I have created this test in bug 804091.
(Assignee)

Comment 98

4 years ago
(In reply to :aceman from comment #97)
> onAutosyncChange() is called from onchange event
> (http://hg.mozilla.org/comm-central/file/6f04beca8b73/mailnews/base/prefs/
> content/am-offline.xul#l67)
> It changes the value of the textbox. And the test assumes the onchange was
> fired and checks if the textbox has the right value after that. By comparing
> the before and after value it checks if onchange was fired properly.

I already know it. And you explain that implementation, right?
As you said that "test *assumes* the onchange...", the test does not check onchange event at all, I meant.

Comment 99

4 years ago
The test checks if onchange was fired automatically, by checking the changed value of the textbox. Running the function changing the value manually would render the test useless.
(Reporter)

Comment 100

4 years ago
Comment on attachment 767669 [details] [diff] [review]
A workaround

-    tree.focus();

Could it be in some cases that the tree already has focus? Or the timing is just slightly wrong?

Although, now I think about it, if you're selecting an item on the tree, shouldn't it already have focus?

The problem with the work around is also that if we want to add any more onchange handlers then it isn't obviously that they would need to be added here.
Attachment #767669 - Flags: review?(mbanner) → review-

Comment 101

4 years ago
(In reply to Mark Banner (:standard8) from comment #100)
> Could it be in some cases that the tree already has focus? Or the timing is
> just slightly wrong?
Maybe.

> Although, now I think about it, if you're selecting an item on the tree,
> shouldn't it already have focus?

See https://bugzilla.mozilla.org/show_bug.cgi?id=804091#c10 on why the explicit .focus() was added.

> 
> The problem with the work around is also that if we want to add any more
> onchange handlers then it isn't obviously that they would need to be added
> here.

Right. If we called the function explicitly we would not need the onchange handler. Also, onAccountTreeSelect may not be called when user click OK immediately on the Offline pane (so the workaround does not run). But onchange should fire, as the focus left the textbox and moved to the OK button.
(Assignee)

Comment 102

4 years ago
If the tree already has focused, then onchange should be fired before it. Right?

(In reply to :aceman from comment #101)
> > 
> > The problem with the work around is also that if we want to add any more
> > onchange handlers then it isn't obviously that they would need to be added
> > here.
> 
> Right. If we called the function explicitly we would not need the onchange
> handler. Also, onAccountTreeSelect may not be called when user click OK
> immediately on the Offline pane (so the workaround does not run). But
> onchange should fire, as the focus left the textbox and moved to the OK
> button.

I totally agree. But if the toolkit really has issue related onchange handler, how can we work around this issue here?
(Assignee)

Comment 103

4 years ago
What I observed on several trials with various modifications of the test code.

a) test_account_onchange_handler succeeds without other tests in test-account-settings-infrastructure.js (all the other tests were commented out).

b) test_account_onchange_handler succeeds if the test runs first before any other tests run.

c) test_account_onchange_handler fails even if the test runs first before any other tests but once open_advanced_settings is invoked.
(Assignee)

Comment 104

4 years ago
Created attachment 768769 [details] [diff] [review]
The modification used on b)

For record, attaching the modification used on b) test.

Comment 105

4 years ago
As the onchange seems to work on other platforms, I think this is another of the focus/popup problems that we see randomly on Linux. We have not yet found why this is happening. mconley works on it and also mozmill guys are making some changes (e.g. explicitly focusing active windows) to solve this.
(Assignee)

Comment 106

4 years ago
Yes, likely. Have you ever seen problems related to onchange handler on Linux?

Comment 107

4 years ago
I have not yet noticed any other problems with onchange. The problem in bug 804091 was on all platforms.
(Assignee)

Comment 108

4 years ago
Created attachment 770097 [details] [diff] [review]
Proper fix

I've finally found the root cause.

We have to spin GTK main loop to receive focus/blur (other events) surely before we go on test codes. This patch waits for focus event before subTest is run, so GTK main loop can be spin completely.

I've confirmed this patch also fixes bug 843257 and bug 803604.
Attachment #767669 - Attachment is obsolete: true
Attachment #768769 - Attachment is obsolete: true
Attachment #770097 - Flags: review?(mbanner)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)

Comment 111

4 years ago
Now a general fix in test-window-helpers.js seems promising, thanks a lot. It may also fix other tests.

This seems to be the try run:
https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=3358cd2c927c
(Reporter)

Comment 112

4 years ago
According to the try server builds this doesn't quite fix it:

https://tbpl.mozilla.org/php/getParsedLog.php?id=24828455&tree=Thunderbird-Try

Test Failure: Timeout waiting for current config to become non-null
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/account/test-mail-account-setup-wizard.js | test-mail-account-setup-wizard.js::test_mail_account_setup

Test Failure: Timeout waiting for create button to be visible and active
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/account/test-mail-account-setup-wizard.js | test-mail-account-setup-wizard.js::test_bad_password_uses_old_settings


https://tbpl.mozilla.org/php/getParsedLog.php?id=24828866&tree=Thunderbird-Try#error0

2013-07-02 05:06:14	mail.wizard	INFO	Shutting down email config dialog
2013-07-02 05:06:14	mail.wizard	INFO	Initializing setup wizard
2013-07-02 05:06:14	mail.wizard	INFO	Email account setup dialog loaded.
2013-07-02 05:06:14	mail.wizard	INFO	switching to UI mode start
2013-07-02 05:06:14	mail.wizard	INFO	Shutting down email config dialog
2013-07-02 05:06:14	mail.wizard	INFO	Initializing setup wizard
2013-07-02 05:06:14	mail.wizard	INFO	Email account setup dialog loaded.
2013-07-02 05:06:14	mail.wizard	INFO	switching to UI mode start
2013-07-02 05:06:15	mail.wizard	INFO	Shutting down email config dialog
Test Failure: Timeout waiting for create button to be visible and active
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/account/test-mail-account-setup-wizard.js | test-mail-account-setup-wizard.js::test_bad_password_uses_old_settings
(Assignee)

Comment 113

4 years ago
Created attachment 770646 [details] [diff] [review]
Fix v2

All tests in mozmill/account passed on my local Linux.
Attachment #770097 - Attachment is obsolete: true
Attachment #770097 - Flags: review?(mbanner)
Attachment #770646 - Flags: review?(mbanner)
(Reporter)

Comment 114

4 years ago
Pushed to try: https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=e80d7b70ea8a
(Reporter)

Comment 115

4 years ago
Comment on attachment 770646 [details] [diff] [review]
Fix v2

Review of attachment 770646 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great. Its survived 5 runs on both opt Linux platforms, so lets see how it goes on the tree.
Attachment #770646 - Flags: review?(mbanner) → review+
(Reporter)

Comment 116

4 years ago
https://hg.mozilla.org/comm-central/rev/fa87b36d83ed
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 25.0
(Reporter)

Comment 117

4 years ago
Comment on attachment 770646 [details] [diff] [review]
Fix v2

[Triage Comment]
Looking great on trunk, so taking onto aurora as well.
Attachment #770646 - Flags: approval-comm-aurora+
(Reporter)

Comment 118

4 years ago
https://hg.mozilla.org/releases/comm-aurora/rev/f188fd56cb7a
status-thunderbird24: --- → fixed

Comment 119

4 years ago
Great work, hiro!
You need to log in before you can comment on or make changes to this bug.