1395481 - Fix fallout from bug 1394556: Compile JSM/component scripts in strict mode by default

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

So far a few failing Xpcshell tests:

TEST-UNEXPECTED-FAIL | mailnews/base/test/unit/test_testsuite_fakeserverAuth.js | xpcshell return code: 0
ReferenceError: assignment to undeclared variable sp at resource://testing-common/mailnews/auth.js:123
TEST-UNEXPECTED-FAIL | mailnews/compose/test/unit/test_smtpAuthMethods.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/compose/test/unit/test_smtpAuthMethods.js | do_check_transaction - [do_check_transaction : 71] "EHLO test,AUTH CRAM-MD5" == "EHLO test,AUTH CRAM-MD5,MAIL FROM:<[e-mail]> BODY=8BITMIME SIZE=155,RCPT TO:<[e-mail]>,DATA"
TEST-UNEXPECTED-FAIL | mailnews/local/test/unit/test_pop3AuthMethods.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/local/test/unit/test_pop3AuthMethods.js | OnStopRunningUrl - [OnStopRunningUrl : 68] 2147500037 == 0
TEST-UNEXPECTED-FAIL | mailnews/local/test/unit/test_pop3GSSAPIFail.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | mailnews/local/test/unit/test_pop3GSSAPIFail.js | OnStopRunningUrl - [OnStopRunningUrl : 66] 2147500037 == 0
TEST-UNEXPECTED-TIMEOUT | mailnews/base/test/unit/test_quarantineFilterMove.js | Test timed out

Maybe fixing the first error will fix some of the others.

That first error is a missing 'let' at mailnews/test/fakeserver/auth.js:
sp = line.split(" "); where 'sp' is never declared.

Mozmill looks quite ugly, all directories fail with:
Exception: Sorry, cannot connect to jsbridge extension, port 24242 [log…]
TEST-UNEXPECTED-FAIL | (runtestlist.py) | Exited with code 1 during directory run

And in the logs we have 82 times:
JavaScript error: resource://mozmill/stdlib/securable-module.js, line 369: ReferenceError: assignment to undeclared variable name
JavaScript error: resource://jsbridge/modules/server.js, line 93: ReferenceError: assignment to undeclared variable globalRegistry

I'll attach a patch that fixes those three and we'll see how we go.

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1395481-strict-errors.patch

Let's see where this gets us:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=613c58bc28b1f590a9c320de263f0b185b51dc22

Comment 2

[Jorg K (CEST = GMT+2)]

Looks like the auth errors have gone, now I see:
PROCESS-CRASH | testing/xpcshell/example/unit/test_do_check_matches_failing.js | application crashed [@ nsThread::ShutdownInternal(bool)]
TEST-UNEXPECTED-FAIL | testing/xpcshell/example/unit/test_do_check_matches_failing.js | run_test - [run_test : 5] {"x":1} deepEqual {}
TEST-UNEXPECTED-TIMEOUT | mailnews/base/test/unit/test_quarantineFilterMove.js | Test timed out [log…]

So looks like were down to two errors here. No Mozmill results yet. I'm going to cancel the Dailies since I'm not sure how badly broken the application will be.

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1395481-strict-errors.patch (v2)

This fixes test_quarantineFilterMove.js.

I'm a bit confused about test_do_check_matches_failing.js since that's an M-C test:
https://dxr.mozilla.org/mozilla-central/source/testing/xpcshell/example/unit/test_do_check_matches_failing.js
Oh, only failed on Mac, so let's not worry about it for now.

Mozmill still not looking good:
JavaScript error: resource://mozmill/modules/frame.js, line 70: ReferenceError: assignment to undeclared variable arrayRemove
JavaScript error: resource://jsbridge/modules/server.js, line 202: ReferenceError: assignment to undeclared variable

So I fixed those, too. Running a test locally I found more and fixed them, too, until I got to:
server.js, line 264: ReferenceError: session is not defined
mail\test\resources\jsbridge\jsbridge\extension\resource\modules\server.js
Session.prototype.onQuit = function() {
  this.instream.close();
  this.outstream.close();
  sessions.remove(session);
};

Not sure which session if meant there. We could be here for a while :-(

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: 1395481-strict-errors.patch (v3) - landed in comment #6

This is getting a little boring :-(
I'm running
mozmake SOLO_TEST=composition/test-forward-rfc822-attach.js mozmill-one
to the next error and fixing them as I see them. Here's an intermediate result.

Comment 5

[Jorg K (CEST = GMT+2)]

With v3 I can't see any more JS errors, but the test run fails with:
WARNING | endRunner was never called. There must have been a failure in the framework.
The application starts, but no tests are run.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c33bf7b6b5a5
Port bug 1394556: Part 1: Fix JS errors caused by using strict. rs=bustage-fix

Comment 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 8903116 [details] [diff] [review]
1395481-strict-errors.patch (v3) - landed in comment #6

I've landed this since there was an M-C merge in the meantime. Xpcshell should be green, and we'll have to follow up on Mozmill. I assume declaring the missing variables for Mozmill was OK, the only problem seems to have been the
 Session.prototype.onQuit = function() {
   this.instream.close();
   this.outstream.close();
-  sessions.remove(session);
+  sessions.remove(this.session);
 };
I tried calling with |undefined|, but that had no positive or negative effect. Maybe keeping track of those sessions is not essential.

I tried to imitate the surrounding code, so were 'var' was uses, I used it instead of 'let'.

Comment 8

[Jorg K (CEST = GMT+2)]

The latest Mozmill results have come in. Apparently we run 41 directories, so I see
  WARNING | endRunner was never called. There must have been a failure in the framework.
41 times.

I wonder whether

07:53:42     INFO -  1504191222314	addons.xpi	WARN	Error parsing extensions state: [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [amIAddonManagerStartup.readStartupData]"  nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame :: resource://gre/modules/addons/XPIProvider.jsm :: loadExtensionState :: line 1505"  data: no] Stack trace: loadExtensionState()@resource://gre/modules/addons/XPIProvider.jsm:1505 < getInstallState()@resource://gre/modules/addons/XPIProvider.jsm:1540 < checkForChanges()@resource://gre/modules/addons/XPIProvider.jsm:3075 < startup()@resource://gre/modules/addons/XPIProvider.jsm:2142 < callProvider()@resource://gre/modules/AddonManager.jsm:263 < _startProvider()@resource://gre/modules/AddonManager.jsm:731 < startup()@resource://gre/modules/AddonManager.jsm:898 < startup()@resource://gre/modules/AddonManager.jsm:3082 < observe()@jar:file:///C:/slave/test/build/application/thunderbird/omni.ja!/components/addonManager.js:65
07:53:43     INFO -  1504191223043	addons.xpi-utils	WARN	addMetadata: Add-on [e-mail] is invalid: Error: Invalid addon ID: expected addon ID [e-mail], found [e-mail] in manifest (resource://gre/modules/addons/XPIProvider.jsm -> resource://gre/modules/addons/XPIProviderUtils.js:1238:15) JS Stack trace: [e-mail]:1238:15 < [e-mail]:1594:23 < [e-mail]:3136:34 < [e-mail]:2142:25 < [e-mail]:263:12 < [e-mail]:731:5 < [e-mail]:898:9 < [e-mail]:3082:5 < [e-mail]:65:9
07:53:43     INFO -  1504191223044	addons.xpi-utils	WARN	Could not uninstall invalid item from locked install location
07:53:43     INFO -  [calBackendLoader] Using icaljs backend at C:\slave\test\build\application\thunderbird\extensions\{e2fda1a4-762b-4020-b5ad-a41df1933103}\components\icaljs-manifest

is relevant so we might be looking at some other bustage here.

Comment 9

[Jorg K (CEST = GMT+2)]

No, we see this in earlier successful runs as well.

Comment 10

[Jorg K (CEST = GMT+2)]

In the log file I also see:
!!! error running onStopped callback: TypeError: callback is not a function
which comes from httpd.js#767:
  dump("!!! error running onStopped callback: " + e + "\n");

Comment 11

[:aceman]

Comment on attachment 8903116 [details] [diff] [review]
1395481-strict-errors.patch (v3) - landed in comment #6

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

Thanks.

::: mail/test/resources/jsbridge/jsbridge/extension/resource/modules/server.js
@@ +260,5 @@
>  };
>  Session.prototype.onQuit = function() {
>    this.instream.close();
>    this.outstream.close();
> +  sessions.remove(this.session);

I have played with this and this.session is still 'undefined'. It seems to me this should be 'sessions.remove(this);'. We are inside the session object here, so remove ourselves fro the list. The this is an object that was previously passed into sessions.add().

Comment 12

[:aceman]

Unfortunately we can't take inspiration from mozmill 2.0, it has a quite rewritten server.jsm.
Only e.g. that in sessions.quit() the line 'this._list.splice(0, this._list.length);' should be outside of the this._list.forEach() loop. That sounds reasonable. Also I don't see any implementation of session.quit().

Comment 13

[:aceman]

Attachment: fix mozmill

This makes mozmill pass for me tests I have tried (not all yet).

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: 1395481.patch

Thanks a lot. I changed 'let' to 'var' to match the rest of the code and restored the |this.transpart = transport;| hunk.

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: 1395481.patch (v3) - more mozmill

Oops, needed to refresh.

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: 1395481.patch (v3b) - more mozmill - landed in comment #17

With index issue fixed.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/34498ab8ad48
Port bug 1394556: Part 2: Fix more JS errors caused by using strict. r=jorgk

Comment 18

[:aceman]

Attachment: 1395481-p3.patch

Some more undeclared variables I found using grep (they are not reported as those files outside mozmill aren't run in strict mode yet).

Only the fix for _document in elements.lib should be needed to fix the calendar tests:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c1d9a48131e53338a6875082b97d3501d82bc4f0

Comment 19

[Jorg K (CEST = GMT+2)]

Comment on attachment 8903425 [details] [diff] [review]
1395481-p3.patch

OK, but you didn't fix the calendar tests, they are all missing 'controller'.

Comment 20

[Jorg K (CEST = GMT+2)]

Attachment: 1395481-controller.patch - NOT used

This will hopefully fix the failing tests.

There are more undefined 'controller' variables:
https://dxr.mozilla.org/comm-central/search?q=controller+%3D+mozmill.getMail3PaneController()%3B&redirect=false

The tests in calendar/test/mozmill/recurrenceRotated are disabled, so they don't fail, and the ones in calendar/test/mozmill/timezoneTests might be disabled as well. Not sure about calendar/test/mozmill/views, calendar/test/mozmill/eventDialog and why calendar/test/mozmill/testAlarmDefaultValue.js hasn't failed since it sits next to calendar/test/mozmill/testBasicFunctionality.js which has failed.

Anyway, let's see what that gives:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=665949541b8c859766cfcf0faa41da7c62da8854

Comment 21

[Jorg K (CEST = GMT+2)]

Comment on attachment 8903466 [details] [diff] [review]
1395481-controller.patch - NOT used

That made no difference :-( But 'controller' is undefined, no?

Comment 22

[Jorg K (CEST = GMT+2)]

I ran
  mozmake -C calendar/test/mozmill SOLO_TEST=testTodayPane.js mozmill-one
locally, and got:

SUMMARY-UNEXPECTED-FAIL | c:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\_tests\mozmill\stage\testTodayPane.js | testTodayPane.js::testTodayPane
  EXCEPTION: Timeout exceeded for waitForElement Lookup: /id("messengerWindow")/id("tabmail-container")/id("tabmail")/id("tabpanelcontainer")/id("calendarTabPanel")/id("calendarContent")/id("ltnSidebar")/id("minimonth-pane")/{"align":"center"}/id("calMinimonthBox")/id("calMinimonth")/anon({"anonid":"minimonth-header"})/anon({"anonid":"today-button"})
    at: utils.js line 409
       TimeoutError utils.js:409 13
       waitFor utils.js:447 11
       MozMillController.prototype.waitFor controller.js:687 3
       MozMillController.prototype.waitForElement controller.js:701 3
       MozMillController.prototype.waitThenClick controller.js:751 3
       testTodayPane testTodayPane.js:60 5

mozmake -C calendar/test/mozmill SOLO_TEST=testBasicFunctionality.js mozmill-one fails with:

SUMMARY-UNEXPECTED-FAIL | c:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\_tests\mozmill\stage\testBasicFunctionality.js | testBasicFunctionality.js::testSmokeTest
  EXCEPTION: assignment to undeclared variable requirementPass
    at: elementslib.js line 347

mozmake -C calendar/test/mozmill SOLO_TEST=testLocalICS.js mozmill-one fails with a timeout.

mozmake -C calendar/test/mozmill SOLO_TEST=cal-recurrence/testAnnualRecurrence.js mozmill-one
fails with the requirementPass error.

Comment 23

[Jorg K (CEST = GMT+2)]

Attachment: 1395481-2.patch - Part 3 - landed in comment #24

I added
-      requirementPass = 0;
-      requirementLength = 0;
+      var requirementPass = 0;
+      var requirementLength = 0;
in two functions.

mozmake -C calendar/test/mozmill SOLO_TEST=cal-recurrence/testAnnualRecurrence.js mozmill-one

now passes. Let's see what else is needed:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=500c44d6819abfac855c42b3c216d36ff43960be

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7f670cfaf58e
Port bug 1394556: Part 3: Fix more JS errors caused by using strict. r=jorgk

Comment 25

[Jorg K (CEST = GMT+2)]

Comment on attachment 8903497 [details] [diff] [review]
1395481-2.patch - Part 3 - landed in comment #24

The try run was hanging there forever, so I cancelled it since an M-C merge had occurred in the meantime.

All the test I ran manually passed, even the ones with a timeout, so let's see where we get which this.

Looks like declaring 'controller' is not required since none of the tests failed in setupModule(). So if we decide to fix the missing declaration, we can do so later and not as a bustage fix.

Comment 26

[Jorg K (CEST = GMT+2)]

Comment on attachment 8903466 [details] [diff] [review]
1395481-controller.patch - NOT used

Should we use this patch and/or fix all the other missing declarations:
https://dxr.mozilla.org/comm-central/search?q=controller+%3D+mozmill.getMail3PaneController()%3B&redirect=false

Comment 27

[:aceman]

Comment on attachment 8903466 [details] [diff] [review]
1395481-controller.patch - NOT used

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

In TB tests, controller is defined in test-folder-display-helpers.js and most/all tests include that.
Calendar tests import test-calendar-utils.js which imports test-folder-display-helpers.js, which again declares and initializes 'controller'. But to some other object (import of controller.js), that then is overwritten in the calendar test to just mozmill.getMail3PaneController();. So it seems to me those tests where controller does not need to be declared work by accident as they overload on already existing variable by the same name. Preferably the calendar tests would rename their variable, but that is a decision for Fallen and is not a bustage fix.

So I would not land this if it is not needed to get the test working.

Comment 28

[Jorg K (CEST = GMT+2)]

Comment on attachment 8903466 [details] [diff] [review]
1395481-controller.patch - NOT used

If we do this, lets fix *all* the tests and move it to a different bug.

Comment 29

[:aceman]

Yes, that is what I said :)

Comment 30

[Jorg K (CEST = GMT+2)]

I think we're done here, the bug goes to Aceman since he did 2/3 of the work, and the hard one, too.

Comment 31

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

Comment on attachment 8903466 [details] [diff] [review]
1395481-controller.patch - NOT used

Having controller defined in test-folder-display-helpers.js sounds like a tricky side-effect to me. I think the better fix would be to correctly modularize/import the symbols instead of relying on a main variable like controller to be defined in a random helper.

f+ for the patch as is, it is a sufficient band-aid

Comment 32

[Jorg K (CEST = GMT+2)]

Comment on attachment 8903466 [details] [diff] [review]
1395481-controller.patch - NOT used

I had forgotten all about this. The patch wasn't necessary at the time and according to Aceman, it would be better for the calendar tests to simply rename their variable. See comment #27.