1474219 - TEST-UNEXPECTED-FAIL | [snip]/mozmill/content-policy/test-general-content-policy.js

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/content-policy/test-general-content-policy.js | test-general-content-policy.js::test_generalContentPolicy
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/content-policy/test-general-content-policy.js | test-general-content-policy.js::test_insertImageIntoReply
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/content-policy/test-general-content-policy.js | test-general-content-policy.js::test_insertImageIntoForward

First seen Fri, Jul 6, 2018, 02:19:12 here:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=a2edf68a0bf2caf2b024c66ab5af74d0d7b9e115&selectedJob=186809428

Also sometimes seen on Mac, but not always.

https://taskcluster-artifacts.net/MtgyLQjRQ_ShnZ76WirjzQ/0/public/logs/live_backing.log

INFO -  SUMMARY-UNEXPECTED-FAIL | test-general-content-policy.js | test-general-content-policy.js::test_generalContentPolicy
INFO -    EXCEPTION: Timeout waiting for compose window editor to load
INFO -      at: utils.js line 406
INFO -         TimeoutError utils.js:406 13
INFO -         waitFor utils.js:444 11
INFO -         wait_for_compose_window test-compose-helpers.js:277 5
INFO -         open_compose_with_reply test-compose-helpers.js:93 10
INFO -         checkComposeWindow test-general-content-policy.js:175 33
INFO -         test_generalContentPolicy test-general-content-policy.js:412 9

Looks like it's downhill from there:

EXCEPTION: Thought we would find row 2 at 126,45 but we found -1
EXCEPTION: Thought we would find row 3 at 126,63 but we found -1
(check the log for details).

Comment 1

[Jorg K (CEST = GMT+2)]

For there record, the latest of Aceman's attempts:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ac863e216fbbd21aa99ab8f7630c7512dad42c85

Comment 2

[Geoff Lankow (:darktrojan)]

Attachment: 1474219-content-policy-1.diff

I think this was caused by bug 1437638, which shuffled around the interfaces on editor. Thus we end up comparing Ci.nsIDocShell.BUSY_FLAGS_NONE with undefined, instead of an actual value.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6bb41d64e40df646b8eb2b41761373d5ae8ad1ec&group_state=expanded
(The test is in Z4/Z2 for the try run because I moved it.)

Comment 3

[Jorg K (CEST = GMT+2)]

Comment on attachment 9007752 [details] [diff] [review]
1474219-content-policy-1.diff

Wow!

If I see this correctly, this is a one liner:
+  editor.webNavigation.QueryInterface(Ci.nsIDocShell);
and the rest is unrelated clean-up.

Comment 4

[:aceman]

Comment on attachment 9007752 [details] [diff] [review]
1474219-content-policy-1.diff

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

Yes, looks like the rest is from my patch on try :)

::: mail/test/mozmill/shared-modules/test-compose-helpers.js
@@ +259,5 @@
>    let replyWindow = windowHelper.wait_for_new_window("msgcompose");
>  
>    let editor = replyWindow.window.document.querySelector("editor");
>  
> +  editor.webNavigation.QueryInterface(Ci.nsIDocShell);

Can this be folded in the next line? Or do we need editor.webNavigation to be nsIDocShell elsewhere (this command permanently changes it)?

Comment 5

[Jorg K (CEST = GMT+2)]

So:
  if (editor.webNavigation.QueryInterface(Ci.nsIDocShell).busyFlags != Ci.nsIDocShell.BUSY_FLAGS_NONE) {
right?

Should we land that fix separately and do the clean-up in another patch?

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: 1474219-content-policy-tidy.diff

Anyone opposed to this split that maintains the authorship?

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: 1474219-content-policy-fix.diff

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6f85f43423fa88591d828129d345d141836b7f33

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2dfed3b88012
Tidy up test-general-content-policy.js. r=jorgk
https://hg.mozilla.org/comm-central/rev/9e4a6da09ad8
Fix failing mozmill test test-general-content-policy.js. r=jorgk

Comment 9

[:aceman]

Great, thanks.

Comment 10

[:aceman]

I wonder how you found this out. It seems to me the damn JS engine didn't warn about the missing property. Right when it actually would be useful.

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 9007786 [details] [diff] [review]
1474219-content-policy-tidy.diff

Applies to both patches.

Comment 12

[Geoff Lankow (:darktrojan)]

Now that I've had some time to think about it, it could be just "editor.docShell.busyFlags" without the QI. Oh well, not going to waste time fixing that now.

(In reply to :aceman from comment #10)
> I wonder how you found this out. It seems to me the damn JS engine didn't
> warn about the missing property. Right when it actually would be useful.

I've seen warnings about that but I don't know why it happens. Is it a "strict" thing?
I (eventually) worked it out by assuming we were waiting for something that never happens and going backwards. This bit looked suspicious so I tried it by hand.

Comment 13

[:aceman]

I thought the tests already run in JS strict mode as we had to fix problems in mozmill when m-c enabled strict mode by default for some type of files (chrome?).

But really, forcing strict mode in the mozmill shared modules seems to cause new problems:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e32aea5270ca072dfac0c13327865bc9db8a998d

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: 1474219-busy-follow-up.patch

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e6370e830deb9dad14b933630790440c48969292

Geoff, I made the patch in your name.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/14f5afb1271f
Follow-up: Simplify check for busy. r=jorgk DONTBUILD

Comment 16

[Jorg K (CEST = GMT+2)]

TB 63 beta:
https://hg.mozilla.org/releases/comm-beta/rev/e34ce33eccaa
https://hg.mozilla.org/releases/comm-beta/rev/8386e5022c93 (merged with simplification)

Comment 17

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #13)

I thought the tests already run in JS strict mode as we had to fix problems
in mozmill when m-c enabled strict mode by default for some type of files
(chrome?).

But really, forcing strict mode in the mozmill shared modules seems to cause
new problems:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-
central&revision=e32aea5270ca072dfac0c13327865bc9db8a998d

I have (almost) updated my local patches against the latest clang-formatted source tree.
However, I notice a few failures on tryserver.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&selectedJob=257866716

I don't see the problems such as this bugzilla entry on my local PC during local testing.
This is very frustrating.

Any hints such as from strict JS checking would be helpful to figure out what is causing the issues.

What was exactly the new problems if we enable strict mode in the mozmill shared modules?
Are they so hard to solve and attempting to enable strict mode is not worth the trouble?

TIA

Comment 18

[Jorg K (CEST = GMT+2)]

Note that the previous technique for applying M-C patches in a try run as attempted here:
https://hg.mozilla.org/try-comm-central/rev/b590b186963f4fab9e1a504fbd4adbf4c88b0099
doesn't work any more. This page
https://wiki.mozilla.org/ReleaseEngineering/ThunderbirdTryServer#Pushing_mozilla-central_patches
documents how it's done today.

Comment 19

[:aceman]

It seems to me all tests now run in strict mode (contains the strict clause in the header), and also the problematic modules in mozmill/shared-modules, which were done in bug 1490448.