Closed Bug 855687 Opened 12 years ago Closed 12 years ago

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

Categories

(Thunderbird :: Account Manager, defect)

Product:
Thunderbird
Component:
Account Manager
Platform:
All
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(thunderbird24 fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 25.0
Tracking Flags:
Tracking Status
thunderbird24 --- fixed

People

(Reporter: standard8, Assigned: hiro)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 3 obsolete files)

Fix v2
3.45 KB, patch
standard8
: review+
standard8
: approval-comm-aurora+
Details | Diff | Splinter Review
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
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.
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?
Attached patch A workaround (obsolete) — — Splinter Review
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)
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.
(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?
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.
(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.
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.
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-
(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.
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?
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.
Attached patch The modification used on b) (obsolete) — — Splinter Review
For record, attaching the modification used on b) test.
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.
Yes, likely. Have you ever seen problems related to onchange handler on Linux?
I have not yet noticed any other problems with onchange. The problem in bug 804091 was on all platforms.
Attached patch Proper fix (obsolete) — — Splinter Review
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)
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
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
Attached patch Fix v2 — — Splinter Review
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)
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+
https://hg.mozilla.org/comm-central/rev/fa87b36d83ed
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 25.0
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+
Great work, hiro!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: