652494 - Uncaught JavaScript exceptions in chrome mochitests should cause test failures

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Cameron McCormack (:heycam)]

Some tests in mochitest-other output JavaScript strict warnings about MochiKit, for example:

2243 INFO TEST-START | chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_DOMWindowCreated.xul
++DOMWINDOW == 40 (10797F58) [serial = 560] [outer = 07A1A418]
pldhash: for the table at address 0B324918, the given entrySize of 48 probably favors chaining over double hashing.
++DOCSHELL 0B3248B0 == 7
++DOMWINDOW == 41 (0FD98DC8) [serial = 561] [outer = 00000000]
WARNING: Subdocument container has no content: file e:/builds/moz2_slave/cen-w32-dbg/build/layout/base/nsDocumentViewer.cpp, line 2380
++DOMWINDOW == 42 (0FD99380) [serial = 562] [outer = 0FD98D78]
WARNING: Subdocument container has no content: file e:/builds/moz2_slave/cen-w32-dbg/build/layout/base/nsDocumentViewer.cpp, line 2380
WARNING: Subdocument container has no content: file e:/builds/moz2_slave/cen-w32-dbg/build/layout/base/nsDocumentViewer.cpp, line 2380
JavaScript strict warning: chrome://mochikit/content/MochiKit/packed.js, line 18: assignment to undeclared variable MochiKit
JavaScript strict warning: chrome://mochikit/content/MochiKit/packed.js, line 995: assignment to undeclared variable compare
JavaScript strict warning: chrome://mochikit/content/MochiKit/packed.js, line 996: assignment to undeclared variable compose
JavaScript strict warning: chrome://mochikit/content/MochiKit/packed.js, line 997: assignment to undeclared variable serializeJSON
JavaScript strict warning: chrome://mochikit/content/MochiKit/packed.js, line 1606: assignment to undeclared variable reduce
JavaScript strict warning: chrome://mochikit/content/MochiKit/packed.js, line 1783: assignment to undeclared variable printfire
JavaScript strict warning: chrome://mochikit/content/MochiKit/packed.js, line 3342: assignment to undeclared variable withWindow
JavaScript strict warning: chrome://mochikit/content/MochiKit/packed.js, line 3343: assignment to undeclared variable withDocument
JavaScript strict warning: chrome://mochikit/content/MochiKit/packed.js, line 4648: assignment to undeclared variable i
JavaScript strict warning: chrome://mochikit/content/MochiKit/packed.js, line 4960: assignment to undeclared variable connect
JavaScript strict warning: chrome://mochikit/content/MochiKit/packed.js, line 4961: assignment to undeclared variable disconnect
JavaScript strict warning: chrome://mochikit/content/MochiKit/packed.js, line 4962: assignment to undeclared variable disconnectAll
JavaScript strict warning: chrome://mochikit/content/MochiKit/packed.js, line 4963: assignment to undeclared variable signal
pldhash: for the table at address 0B325A58, the given entrySize of 48 probably favors chaining over double hashing.
++DOCSHELL 0B3259F0 == 8
++DOMWINDOW == 43 (0FD80E08) [serial = 563] [outer = 00000000]
WARNING: NS_ENSURE_SUCCESS(rv, 0) failed with result 0x8000FFFF: file e:/builds/moz2_slave/cen-w32-dbg/build/content/base/src/nsContentUtils.cpp, line 2915
WARNING: NS_ENSURE_TRUE(pusher.Push(aBoundElement)) failed: file e:/builds/moz2_slave/cen-w32-dbg/build/content/xbl/src/nsXBLProtoImplMethod.cpp, line 327
WARNING: NS_ENSURE_TRUE(aURI) failed: file e:/builds/moz2_slave/cen-w32-dbg/build/content/base/src/nsContentUtils.cpp, line 5023
++DOMWINDOW == 44 (0FD7FD70) [serial = 564] [outer = 0FD80DB8]
2244 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_DOMWindowCreated.xul | DOMWindowCreated: [object Window @ 0x7d13e10 (native @ 0xfd7fd70)] - "DOMWindowCreated" should equal "DOMWindowCreated"
2245 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_DOMWindowCreated.xul | doneFunction was called
2246 INFO TEST-END | chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_DOMWindowCreated.xul | finished in 595ms

There are 12 of these in mochitest-other output these days.  The lines aren't prefaced with "TEST-UNEXPECTED-FAIL" so don't cause Tinderbox failures, but it makes me wonder whether the tests are actually running properly.

First, how are these few tests running the MochiKit files in strict mode?

Second, is the right solution to remove the strict mode hazards from MochiKit or to force these files to be run in non-strict mode in these tests?

Comment 1

[(not currently active) Ted Mielczarek]

First, this is not ES5 strict mode, this is javascript.options.strict, which is enabled by default in debug builds. It's an older pref that covers a set of things that probably has some overlap with ES5 strict, but doesn't make anything fatal, I don't think, it just warns about them.

I doubt it's causing any problems, those just look like the usual "assigning to a variable without using the var keyword" sorts of things.

I don't think it's worth trying to fix MochiKit to fix things like "assignment to undeclared variable foo", unless we could drop-in a newer version as a replacement without breaking everything.

Comment 2

[Cameron McCormack (:heycam)]

Understood.  Those javascript.options.strict warnings get reported to the onerror handler, which find their way to the `function simpletestOnerror` at the end of SimpleTest.js.  In there, they are reported with `ok(false, ...)` calls, but for whatever reason these aren't being translated into TEST-UNEXPECTED-FAILUREs in the output.  I was doing some cleanup of the logging functions as part of bug 642175 which made these become "real" errors, which is how they came to my attention.

From bug 427500 comment 7 it sounds like we have patches on top of the base MochiKit files, so I doubt we could drop in a replacement easily anyway.  I think there is value in having strict warnings become real errors -- people are rarely going to notice these warning lines dumped in the logs if they don't trigger test failures.

(TBH I'm wondering whether it's worth eventually moving off MochiKit.  It seems to have a lot of cruft and functionality we don't need.)

Comment 3

[(not currently active) Ted Mielczarek]

Might be an option. I don't think we're actually using much of it, honestly. I believe it was just an easy place to start when sayrer first implemented Mochitest. It sure would make the test suite name ironic, though. :)

Comment 4

[Boris Zbarsky [:bzbarsky]]

For what it's worth, the lack of fail-on-exception in browser-chrome tests has been a real problem for me today: I had a patch that was broken, but the test kept claiming it was all OK....

Comment 5

[Nicholas Nethercote [inactive]]

FWIW, this bug caused bug 700190, where test_aboutmemory.xul was broken but nobody realized becaukse the exception was swallowed.

Comment 6

[Nicholas Nethercote [inactive]]

I'm told that making exceptions cause failures is difficult because it'd cause lots of new failures.  A half-assed improvement would be to have an opt-in option that lets a test request exceptions-as-failures.

Comment 7

[Cameron McCormack (:heycam)]

Better than that would be an opt-in to request exceptions to be ignored instead of reported, and for the existing known tests that do throw uncaught exceptions to be modified to opt in to this.

Comment 8

[Cameron McCormack (:heycam)]

Attachment: Report uncaught JS exceptions in chrome mochitests as test failures [Checked in: Comment 12]

The patch does the following:

1. Makes uncaught JS exceptions cause a test failure in (browser-)chrome
   mochitests instead of just dump()ing them.
2. Introduces a SimpleTest.ignoreAllUncaughtExceptions() function that
   causes all uncaught JS exceptions to be reported with todo(false)
   instead of ok(false).
3. Adds a call to ignoreAllUncaughtExceptions() for the tests that
   currently have uncaught exceptions and haven't been fixed yet (the
   open bugs blocking this one).
4. Simplifies the searching for the TestRunner/SpecialPowers objects
   done at the top of SimpleTest.js.  (I stuck this in an anonymous
   function to avoid polluting tests with the variables used as part of
   this.)
5. Ensures that SimpleTest.finish() is not called automatically on page
   load for secondary test windows that load SimpleTest.js -- this was
   causing multiple tests to run at once, causing confusing/incorrect
   test failure reporting.

Once this lands I'll file a followup bug to remove the
ignoreAllUncaughtExceptions() function after all the broken tests have
been fixed.

Comment 9

[Nicholas Nethercote [inactive]]

Nice patch!  Nice description of the patch! :)

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 573645 [details] [diff] [review]
Report uncaught JS exceptions in chrome mochitests as test failures
[Checked in: Comment 12]

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

r=me with the one comment addressed.

::: testing/mochitest/tests/SimpleTest/SimpleTest.js
@@ +33,5 @@
> +                parentRunner = w.wrappedJSObject.TestRunner;
> +            }
> +        }
> +        if (!window.SpecialPowers) {
> +            window.SpecialPowers = w.SpecialPowers;

Can we keep the comment about which test case and scenarios require this.  SpecialPowers is dicey when it comes to defining it and the one remaining change (bug 695292) requires some work on this.

Comment 11

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e804728b128b

I put the comments back in (reworded slightly since they're in a different position).

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e804728b128b