Closed Bug 1395481 Opened 2 years ago Closed 2 years ago

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

Categories

(Thunderbird :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 57.0

People

(Reporter: jorgk, Assigned: aceman)

Details

Attachments

(4 files, 6 obsolete files)

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:<from@foo.invalid> BODY=8BITMIME SIZE=155,RCPT TO:<to@foo.invalid>,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.
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.
Attached patch 1395481-strict-errors.patch (v2) (obsolete) — Splinter Review
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 :-(
Attachment #8903064 - Attachment is obsolete: true
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.
Attachment #8903104 - Attachment is obsolete: true
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.
Keywords: leave-open
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/c33bf7b6b5a5
Port bug 1394556: Part 1: Fix JS errors caused by using strict. rs=bustage-fix
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'.
Attachment #8903116 - Attachment description: 1395481-strict-errors.patch (v3) → 1395481-strict-errors.patch (v3) - landed in comment #6
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 specialpowers@mozilla.org is invalid: Error: Invalid addon ID: expected addon ID specialpowers@mozilla.org, found special-powers@mozilla.org in manifest (resource://gre/modules/addons/XPIProvider.jsm -> resource://gre/modules/addons/XPIProviderUtils.js:1238:15) JS Stack trace: addMetadata@XPIProviderUtils.js:1238:15 < processFileChanges@XPIProviderUtils.js:1594:23 < checkForChanges@XPIProvider.jsm:3136:34 < startup@XPIProvider.jsm:2142:25 < callProvider@AddonManager.jsm:263:12 < _startProvider@AddonManager.jsm:731:5 < startup@AddonManager.jsm:898:9 < startup@AddonManager.jsm:3082:5 < observe@addonManager.js: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.
No, we see this in earlier successful runs as well.
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 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().
Attachment #8903116 - Flags: review+
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().
Attached patch fix mozmill (obsolete) — Splinter Review
This makes mozmill pass for me tests I have tried (not all yet).
Attachment #8903338 - Flags: review?(jorgk)
Attached patch 1395481.patch (obsolete) — Splinter Review
Thanks a lot. I changed 'let' to 'var' to match the rest of the code and restored the |this.transpart = transport;| hunk.
Attachment #8903338 - Attachment is obsolete: true
Attachment #8903338 - Flags: review?(jorgk)
Attachment #8903350 - Flags: review+
Oops, needed to refresh.
Attachment #8903350 - Attachment is obsolete: true
Attachment #8903351 - Flags: review+
With index issue fixed.
Attachment #8903351 - Attachment is obsolete: true
Attachment #8903372 - Flags: review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/34498ab8ad48
Port bug 1394556: Part 2: Fix more JS errors caused by using strict. r=jorgk
Attached patch 1395481-p3.patch (obsolete) — Splinter Review
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
Attachment #8903425 - Flags: review?(jorgk)
Comment on attachment 8903425 [details] [diff] [review]
1395481-p3.patch

OK, but you didn't fix the calendar tests, they are all missing 'controller'.
Attachment #8903425 - Flags: review?(jorgk) → review+
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
Attachment #8903372 - Attachment description: 1395481.patch (v3b) - more mozmill → 1395481.patch (v3b) - more mozmill - landed in comment #17
Comment on attachment 8903466 [details] [diff] [review]
1395481-controller.patch - NOT used

That made no difference :-( But 'controller' is undefined, no?
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.
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
Attachment #8903425 - Attachment is obsolete: true
Attachment #8903497 - Flags: review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/7f670cfaf58e
Port bug 1394556: Part 3: Fix more JS errors caused by using strict. r=jorgk
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.
Attachment #8903497 - Attachment description: 1395481-2.patch - Part 3 → 1395481-2.patch - Part 3 - landed in comment #24
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
Attachment #8903466 - Flags: feedback?(acelists)
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.
Attachment #8903466 - Flags: feedback?(philipp)
Attachment #8903466 - Flags: feedback?(acelists)
Attachment #8903466 - Flags: feedback+
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.
Yes, that is what I said :)
I think we're done here, the bug goes to Aceman since he did 2/3 of the work, and the hard one, too.
Assignee: nobody → acelists
Status: NEW → RESOLVED
Closed: 2 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 57.0
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
Attachment #8903466 - Flags: feedback?(philipp) → feedback+
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.
Attachment #8903466 - Attachment description: 1395481-controller.patch → 1395481-controller.patch - NOT used
You need to log in before you can comment on or make changes to this bug.