TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/content-tabs/test-content-tab.js | test-content-tab.js::test_content_tab_onbeforeunload

RESOLVED FIXED in Thunderbird 45.0

Status

Thunderbird
Toolbars and Tabs
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: aleth, Assigned: aleth)

Tracking

({intermittent-failure, regression})

Trunk
Thunderbird 45.0
intermittent-failure, regression

Thunderbird Tracking Flags

(thunderbird43 unaffected, thunderbird44 fixed, thunderbird45 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
EXCEPTION: Timeout waiting for modal dialog to open
https://dxr.mozilla.org/comm-central/source/mail/test/mozmill/content-tabs/test-content-tab.js#162

Looks like this is caused by bug 636905.
(Assignee)

Updated

3 years ago
Keywords: regression

Comment 1

3 years ago
Thanks for investigating this. So you suspect the onbeforeunload event is not run, therefore we do not get a dialog (probably containing the event.returnValue text). We could probably interact with the tab to fix the test. But do we need to test this that event? I could not find any use of it in the base code. There is only a comment at http://mxr.mozilla.org/comm-central/source/mail/base/content/tabmail.xml#697.
Component: General → Toolbars and Tabs
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → Trunk
(Assignee)

Comment 2

3 years ago
Ask a peer of the code whether the test can be disabled? Otherwise you'd probably fix it like so: https://hg.mozilla.org/mozilla-central/rev/ef432f51a591
(Assignee)

Comment 3

3 years ago
Created attachment 8669191 [details] [diff] [review]
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/content-tabs/test-content-tab.js | test-content-tab.js::test_content_tab_onbeforeunload
Attachment #8669191 - Flags: review?(acelists)
(Assignee)

Updated

3 years ago
Assignee: nobody → aleth
Status: NEW → ASSIGNED

Comment 4

3 years ago
Comment on attachment 8669191 [details] [diff] [review]
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/content-tabs/test-content-tab.js | test-content-tab.js::test_content_tab_onbeforeunload

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

As our code does mention the possibility of running the onbeforeunload event, I am not sure if we have some code that needs to run always, not only if we interacted with the tab. We may need to make this pref global for the app (in defaults.js) unless we audit all potential users of it.
So f+ that this fixes the particular test, but it still needs to be decided if this is the right way to do it.
Attachment #8669191 - Flags: review?(richard.marti)
Attachment #8669191 - Flags: review?(mkmelin+mozilla)
Attachment #8669191 - Flags: review?(acelists)
Attachment #8669191 - Flags: feedback+
Comment on attachment 8669191 [details] [diff] [review]
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/content-tabs/test-content-tab.js | test-content-tab.js::test_content_tab_onbeforeunload

I'm far away to be able to review code in tests, so removing me. I've never wrote a test and can't say if something is correct or wrong.

Magnus surely is the correct person to review this.
Attachment #8669191 - Flags: review?(richard.marti)

Comment 6

3 years ago
The question was not about the test but about tabs needing onbeforeunload event always.
Also for this question, I don't know enough to say what this change would imply.

Comment 8

3 years ago
Comment on attachment 8669191 [details] [diff] [review]
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/content-tabs/test-content-tab.js | test-content-tab.js::test_content_tab_onbeforeunload

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

Looks reasonable I think. Please also clearUserPref after the test is done.
Attachment #8669191 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 9

3 years ago
(In reply to Magnus Melin from comment #8)
> Looks reasonable I think. Please also clearUserPref after the test is done.

That's not needed afaik, each test runs in a fresh environment.

Comment 10

3 years ago
No, in mozmill all tests in a directory run in the same environment. Each test directory starts fresh.
(Assignee)

Comment 11

3 years ago
(In reply to :aceman from comment #10)
> No, in mozmill all tests in a directory run in the same environment. Each
> test directory starts fresh.

Thanks! I guess it's only for xpcshell then.
(Assignee)

Comment 12

3 years ago
Created attachment 8672254 [details] [diff] [review]
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/content-tabs/test-content-tab.js | test-content-tab.js::test_content_tab_onbeforeunload
(Assignee)

Updated

3 years ago
Attachment #8669191 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8672254 - Flags: review+
(Assignee)

Comment 13

3 years ago
Created attachment 8672255 [details] [diff] [review]
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/content-tabs/test-content-tab.js | test-content-tab.js::test_content_tab_onbeforeunload
(Assignee)

Updated

3 years ago
Attachment #8672254 - Attachment is obsolete: true
What is the status of this bug? thunderbird-tree-status says it is checkin-needed, but it doesn't seem so.
Flags: needinfo?(aleth)
(Assignee)

Comment 15

3 years ago
https://hg.mozilla.org/comm-central/rev/071eec01aca961c0d603b94c41a89a17645f4af6
Bug 1209791 - TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/content-tabs/test-content-tab.js | test-content-tab.js::test_content_tab_onbeforeunload. r=mkmelin a=aleth
(Assignee)

Comment 16

3 years ago
(In reply to Philipp Kewisch [:Fallen] from comment #14)
> What is the status of this bug? thunderbird-tree-status says it is
> checkin-needed, but it doesn't seem so.

The tree was closed...
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Flags: needinfo?(aleth)
Resolution: --- → FIXED
(Assignee)

Updated

3 years ago
Target Milestone: --- → Thunderbird 45.0
(Assignee)

Updated

2 years ago
Attachment #8672255 - Flags: approval-comm-aurora?
Failing on aurora too
> https://treeherder.mozilla.org/logviewer.html#?job_id=21364&repo=comm-aurora
status-thunderbird44: --- → affected
status-thunderbird45: --- → fixed

Comment 18

2 years ago
Comment on attachment 8672255 [details] [diff] [review]
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/content-tabs/test-content-tab.js | test-content-tab.js::test_content_tab_onbeforeunload

https://hg.mozilla.org/releases/comm-aurora/rev/5e0f8ffa1acb
Attachment #8672255 - Flags: approval-comm-aurora? → approval-comm-aurora+

Updated

2 years ago
status-thunderbird43: --- → unaffected
status-thunderbird44: affected → fixed
You need to log in before you can comment on or make changes to this bug.