1446050 - Complete loss of Mozmill testing - Exception: Sorry, cannot connect to jsbridge extension, port 24242

Status: RESOLVED FIXED

(Thunderbird :: Testing Infrastructure, enhancement)

Description

[Jorg K (CEST = GMT+2)]

Sadly we lost all Mozmill testing today. In this Daily run
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=fc57e733b75a63884cc171f2b8256497ef7c0148

the Linux jobs ran on M-C fcb11e93adf57210167de0b27b15433e9c and failed, Windows ran on 68dfe5ee5b80ee99b7e389d739a30089f6 and succeeded.

M-C last good: 68dfe5ee5b80ee99b7e389d739a30089f6
M-C first bad: fcb11e93adf57210167de0b27b15433e9c
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=68dfe5ee5b80ee99b7e389d739a30089f6&tochange=fcb11e93adf57210167de0b27b15433e9c

This looks like bug 1445780. Apparently Mozmill uses the jsbridge extension which we know since we recently tweaked it here:
https://hg.mozilla.org/comm-central/rev/6654b9108d7e46f720e1581b121bd2baa1a3f519#l4.12

Comment 1

[Jorg K (CEST = GMT+2)]

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

INFO -  JavaScript error: chrome://calendar/content/calendar-migration-dialog.js, line 13: NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIXPCComponents_Utils.importGlobalProperties]
INFO -  JavaScript error: resource://mozmill/stdlib/json2.js, line 163: ReferenceError: assignment to undeclared variable JSON
INFO -  JavaScript error: resource://jsbridge/modules/json2.js, line 165: ReferenceError: assignment to undeclared variable JSON

calendar-migration-dialog.js, line 13 is:
  Components.utils.importGlobalProperties(["XMLHttpRequest"]);
and something like that was just changed here:

https://hg.mozilla.org/mozilla-central/rev/a3631cb49bef#l6.6
-Cu.importGlobalProperties(["XMLHttpRequest"]);
+// This test requires an XMLHttpRequest constructor which isn't
+// associated with a window.
+const {XMLHttpRequest} = Cu.Sandbox(window, {wantGlobalProperties: ["XMLHttpRequest"]});

So that would be the first thing to try.

All other "jsbridge" errors in the log seem to appear in the same context.

There are more |Components.utils.importGlobalProperties(["XMLHttpRequest"]);| in the code base, so we'll see whether they also fail: https://dxr.mozilla.org/comm-central/search?q=XMLHttpRequest&redirect=false

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1446050-bustage.patch

So this makes the Calendar error go away, but I still get:
JavaScript error: resource://mozmill/stdlib/json2.js, line 163: ReferenceError: assignment to undeclared variable JSON
JavaScript error: resource://jsbridge/modules/json2.js, line 165: ReferenceError: assignment to undeclared variable JSON

and

Traceback (most recent call last):
  File "runtest.py", line 523, in <module>
    ThunderTestCLI().run()
  File "c:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\_tests\mozmill-virtualenv\lib\site-packages\mozmill\__init__.py", line 790, in run
    self.mozmill.start(runner=runner, profile=runner.profile)
  File "runtest.py", line 287, in start
    return super(ThunderTestMozmill, self).start(profile, runner)
  File "c:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\_tests\mozmill-virtualenv\lib\site-packages\mozmill\__init__.py", line 230, in start
    self.create_network()
  File "c:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\_tests\mozmill-virtualenv\lib\site-packages\mozmill\__init__.py", line 204, in create_network
    self.jsbridge_port)
  File "c:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\_tests\mozmill-virtualenv\lib\site-packages\jsbridge\__init__.py", line 72, in wait_and_create_network
    raise Exception("Sorry, cannot connect to jsbridge extension, port %s" % port)
Exception: Sorry, cannot connect to jsbridge extension, port 24242
mozmake: *** [c:/mozilla-source/comm-central/mozilla/../mail/testsuite-targets.mk:42: mozmill-one] Error 1

Comment 3

[Jorg K (CEST = GMT+2)]

Oops, quoted the wrong bug number, this is caused by bug 1445551.

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: 1446050-bustage.patch WIP v2

Fixing the reference errors, I now get:
[3856, Main Thread] WARNING: NS_ENSURE_TRUE(*out) failed: file c:/mozilla-source/comm-central/mozilla/js/xpconnect/src/Sandbox.cpp, line 1198
JavaScript error: resource://jsbridge/modules/server.js, line 204: NS_ERROR_ILLEGAL_VALUE: Illegal value

which appears to come from:
C:\mozilla-source\comm-central\mail\test\resources\jsbridge\jsbridge\extension\resource\modules\server.js
Line 204 reads:
  this.sandbox = Cu.Sandbox(backstage, { wantGlobalProperties: ["ChromeUtils"] });

The final error has shifted to:
jsbridge.network.JSBridgeDisconnectError: Connection timed out
mozmake: *** [c:/mozilla-source/comm-central/mozilla/../mail/testsuite-targets.mk:42: mozmill-one] Error 1

Comment 5

[Jorg K (CEST = GMT+2)]

Reading Sandbox.cpp, line 1198 this comes from the first argument passed in. The comment above says:
 * For sandbox constructor the first argument can be a principal object or
 * a script object principal (Document, Window).

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: 1446050-bustage.patch

With this, I can run a Mozmill test. Sorry about a bit of unrelated clean-up.

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

Comment 7

[:aceman]

Comment on attachment 8959350 [details] [diff] [review]
1446050-bustage.patch

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

Thanks, works also for me locally.

::: mail/test/resources/mozmill/mozmill/extension/resource/stdlib/json2.js
@@ +158,5 @@
>  var EXPORTED_SYMBOLS = ["JSON"];
>  
>  // Create a JSON object only if one does not already exist. We create the
>  // object in a closure to avoid creating global variables.
>  

I wonder why this file is missing the if (!this.JSON) guard the other json2.js has, while the comment about it is the same. We can try adding that in the next patch.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/08a841e8b1af
Port bug 1445551 to TB: Remove compartment-per-add-on. r=aceman

Comment 9

[Jorg K (CEST = GMT+2)]

Comment on attachment 8959350 [details] [diff] [review]
1446050-bustage.patch

Hi Philipp, I'm sure you have more insights into this than the humble fire fighters. I'm happy to land a follow-up. The Calendar change is copied from:
https://hg.mozilla.org/mozilla-central/rev/a3631cb49bef#l6.6 (see comment #1).
I can't even make much sense of the comment.

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Jörg, can you try to remove the importGlobalProperties line for calendar-migration-dialog? This is a script for a window, so there should be no need to import XHR.

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: 1446050-followup.patch

Sorry about the hasty landing of the bustage fix, but having *all* Mozmill red/orange wasn't much fun.

I tried the removal on one Mozmill test locally and it passed. So here's a try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b937c4c0c75cdecc92ffaf9edd5a1c688cda88d3

What is that calendar-migration-dialog.js about?

Comment 12

[Jorg K (CEST = GMT+2)]

That worked :-)

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b7d028d0299c
Follow-up: Remove import of XMLHttpRequest. r=philipp

Comment 14

[:aceman]

Attachment: 1446050.patch

Add the check for JSON to get the json2.js files to sync in this.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=45c7dbe5929bd291f865cd6e61a07429e1cc9510

Comment 15

[Jorg K (CEST = GMT+2)]

Hmm, but obviously the check wasn't necessary. Why not try to remove the other one instead?

Comment 16

[:aceman]

I'd think that the 'var' does the magic, if you declare the thing multiple times it may not matter. With the patch it may just be faster that we do not need to read the source of the thing again, when it already exists.

Comment 17

[Jorg K (CEST = GMT+2)]

Comment on attachment 8959702 [details] [diff] [review]
1446050.patch

Arai, can you please look at this. What is 'this' in this context?

Note that we already have this code here:
https://dxr.mozilla.org/comm-central/rev/a8eecfe6de793af00e98cb1488515199c5fb73fb/mail/test/resources/jsbridge/jsbridge/extension/resource/modules/json2.js#160

in another copy of this file.

Comment 18

[Tooru Fujisawa [:arai]]

(In reply to Jorg K (GMT+1) from comment #17)
> Comment on attachment 8959702 [details] [diff] [review]
> 1446050.patch
> 
> Arai, can you please look at this. What is 'this' in this context?

the global object.
so, `this.JSON` is basically the same thing as referring global variable `JSON`.

Comment 19

[Jorg K (CEST = GMT+2)]

Hmm, sorry to ask again: So does the patch make sense? We're checking the 'JSON' property of the global object and if not there, we assign 'var JSON = ...';

Note that previously there was no 'var', see:
https://hg.mozilla.org/comm-central/rev/08a841e8b1af77cffce8eefa0eeced022d36d63a#l2.12
Back then, 'JSON' would have been a property of the global object, but now it's not any more?
Sorry about my poor JS skills :-(

Comment 20

[Tooru Fujisawa [:arai]]

(In reply to Jorg K (GMT+1) from comment #19)
> Hmm, sorry to ask again: So does the patch make sense? We're checking the
> 'JSON' property of the global object and if not there, we assign 'var JSON =
> ...';

if `this.JSON = ...` works, that looks consistent with the check.

> Back then, 'JSON' would have been a property of the global object, but now
> it's not any more?

I'm not sure about that part.
is the `!this.JSON` condition become true?

Comment 21

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

If this.JSON doesnt work, you can try Components.getGlobalForObject(this).JSON

Comment 22

[Jorg K (CEST = GMT+2)]

Attachment: 1446050.patch - patch enhanced with debug

(In reply to Philipp Kewisch [:Fallen]  from comment #21)
> If this.JSON doesnt work, you can try
> Components.getGlobalForObject(this).JSON
Thanks for your comment. To determine what "doesn't work" mean here, we first need to define what the desired effect is.

I have the impression that the aim is to avoid parsing |var JSON = ...| more than once for performance reasons.

I ran an entire Mozmill directory with the attached patch, and all I saw was:
=== modules: JSON was NOT defined
=== stdlib: JSON was NOT defined
The other debug didn't show up at all.

So is there any evidence that the patch does anything useful and not just add dead code which should actually be removed from the other copy?

Comment 23

[Jorg K (CEST = GMT+2)]

Comment on attachment 8959702 [details] [diff] [review]
1446050.patch

Please provide evidence that this does anything useful.

Comment 24

[:aceman]

The idea was to sync the 2 copies of the file that we have in the tree and that the check should not hurt. It could help if we would be parsing the file multiple times. We we aren't right now, then that is great, but some robustness should hurt. I don't think adding one check should slow this down in any significant way.
The current version of this 3rd party lib still has this guard and the comment: https://github.com/douglascrockford/JSON-js/blob/master/json2.js , albeit done in a different way, which I'm not sure prevents parsing of the whole code again.

It may be that inside c-c we import this file as a jsm module, thus the added 'var EXPORTED_SYMBOLS = ["JSON"];' which may cause it only be parsed once (that is why it is cheap to import jsms into tons of other JS files).

Comment 25

[Jorg K (CEST = GMT+2)]

Attachment: 1446050.patch - with a comment

OK, let's get this settled.

Comment 26

[Jorg K (CEST = GMT+2)]

Attachment: 1446050.patch - with a comment

Typo.

Comment 27

[:aceman]

Comment on attachment 8960004 [details] [diff] [review]
1446050.patch - with a comment

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

Yes, that looks good ;)

Comment 28

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Looking closer, this file only exists for environments where JSON is not defined. For c-c it is defined in all scopes I know of. With the JSM compartment changes, this.JSON used to point to the global JSON object. This is no longer the case (similar to how this.Array no longer worked).

I think the intent will be to check if JSON is defined on the current global, in which case you can probably guard the implementation with if (typeof JSON == "undefined") or if (!Components.getGlobalForObject(this).JSON)

If you run it with debug you will see that with these checks JSON *is* defined, in which case the native implementation is used instead of the one from json2.js, which I would say is favorable.

Comment 29

[Jorg K (CEST = GMT+2)]

Attachment: 1446050.patch - NOT working

OK, this does what Philipp suggested. typeof JSON == "undefined" didn't work, but this does. The debug shows "we have JSON". Sadly the test then hangs and I get "Sorry, cannot connect to jsbridge extension, port 24242".

So the conclusion seems to be that we need to use the JSON defined here and not the global JSON object.

Comment 30

[Jorg K (CEST = GMT+2)]

Attachment: 1446050.patch

So this is following Philipp's idea, use the global JSON if available.

Comment 31

[:aceman]

Comment on attachment 8960039 [details] [diff] [review]
1446050.patch

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

Great, if this passes try :)

::: mail/test/resources/jsbridge/jsbridge/extension/resource/modules/json2.js
@@ +160,4 @@
>  
> +if (!JSON) {
> +  dump("jsbridge/extension/resource/modules/json2.js: creating JSON object\n");
> +  

whitespace here

Comment 32

[Jorg K (CEST = GMT+2)]

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

I've noticed the white-space, removed in local copy.

Comment 33

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

|if (!JSON)| will fail with an exception if it is actually undefined.

Comment 34

[Jorg K (CEST = GMT+2)]

Hmm, sorry, I tried
var EXPORTED_SYMBOLS = ["JSON"];
var JSON = Cu.getGlobalForObject(this).JSONxx; // Note JSONxx here.
if (!JSON) {
  dump("mozmill/extension/resource/stdlib/json2.js: creating JSON object\n");

So JSON should be undefined, right? My debug shows:
mozmill/extension/resource/stdlib/json2.js: creating JSON object

undefined is falsy, so !undefined should work, and it appears to work. What am I missing?

Comment 35

[Jorg K (CEST = GMT+2)]

OK, this is correct, as discussed on IRC.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/69eb5fe79295
use global JSON if available. r=aceman