1852781 - SimpleTest reports an uncaught exception when MockRegistrar mocked a method that throws an exception even if the exception is actually caught

Status: NEW

(Testing :: Mochitest, defect, P2)

Description

[Masatoshi Kimura [:emk]]

Steps to reproduce:

1. Apply the attached patch.
2. ./mach test xpcom/tests/browser/browser_falseUncaughtException.js

Actual result:

Unexpected Results
------------------
xpcom/tests/browser/browser_falseUncaughtException.js
  FAIL uncaught exception - NS_ERROR_FAILURE:  at method1@chrome://mochitests/content/browser/xpcom/tests/browser/browser_falseUncaughtException.js:10:22
uncaught_exception@chrome://mochitests/content/browser/xpcom/tests/browser/browser_falseUncaughtException.js:29:18
handleTask@chrome://mochikit/content/browser-test.js:1131:26
_runTaskBasedTest@chrome://mochikit/content/browser-test.js:1203:18
Tester_execTest@chrome://mochikit/content/browser-test.js:1345:14
nextTest/<@chrome://mochikit/content/browser-test.js:1120:14
SimpleTest.waitForFocus/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1056:13

Stack trace:
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:simpletestOnerror:2003
chrome://mochitests/content/browser/xpcom/tests/browser/browser_falseUncaughtException.js:uncaught_exception:29
chrome://mochikit/content/browser-test.js:handleTask:1131
chrome://mochikit/content/browser-test.js:_runTaskBasedTest:1203
chrome://mochikit/content/browser-test.js:Tester_execTest:1345
chrome://mochikit/content/browser-test.js:nextTest/<:1120
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/<:1056

Expected result:
Test succeeds.

I found this when I was working on bug 1850631.

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: Bug 1852781 - Testcase for false uncaught exception bug

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:jmaher, could you have a look please?

For more information, please visit BugBot documentation.

Comment 3

[Nick Alexander :nalexander [he/him]]

Nika -- we talked about this a little and you said in #xpcom, "[W]hen you call a JS-implemented XPCOM object from C++, we will try to report exceptions that object throws as uncaught exceptions if thrown. Because the global is a window, that ends up calling the onerror callback."

I don't understand this: first, I don't see how anything is getting invoked from C++ -- at least, not directly. This is a JS test with a JS defined XPCOM implementation.

Second, I don't think this is specific to MockRegistrar. If I make some existing JS XPCOM implementation throw -- I choose Services.prompt.confirm here -- then I get the same failures and logging. This is from a hacked up test that just invokes the throwing method:

 0:01.36 PASS run_test - [run_test : 56] "undefined" === "undefined"
 0:01.36 pid:15192 JavaScript error: /Users/nalexander/Mozilla/objdirs/objdir-browser-compile/_tests/xpcshell/xpcom/tests/unit/test_notxpcom_scriptable.js, line 30: NS_ERROR_FAILURE: 
 0:01.36 PASS run_test - [run_test : 60] . Should throw
 0:01.36 INFO (xpcshell/head.js) | test MAIN run_test finished (1)

What's strange is that the "Should throw" line is from an Assert.throws(...). That is, the exception is thrown and caught, but it still "escapes" to some larger context (that prints the JavaScript error: line). It seems like it would be hard to use exceptions from JS-implemented XPCOM objects at all with this behaviour, so maybe there is some test-specific thing that's changing the behaviour?

Can you shed light on what's happening, and whether this can be worked around?

Comment 4

[Nika Layzell [:nika] (ni? for response)]

(In reply to Nick Alexander :nalexander [he/him] from comment #3)

Nika -- we talked about this a little and you said in #xpcom, "[W]hen you call a JS-implemented XPCOM object from C++, we will try to report exceptions that object throws as uncaught exceptions if thrown. Because the global is a window, that ends up calling the onerror callback."

I don't understand this: first, I don't see how anything is getting invoked from C++ -- at least, not directly. This is a JS test with a JS defined XPCOM implementation.

When you get a JS XPCOM interface from C++, such as by creating a JS xpcom interface through the component manager, we actually wrap the JS object with an XPCWrappedJS to turn it into a native interface, then wrap it again with an XPCWrappedNative to turn it back into a JS object. This means that things like the interface are standardized to look like any other implementation of that XPCOM interface, and that things like methods have the expected behaviours.

This is why you have to write .wrappedJSObject in some cases to get the underlying JS object which implements an XPIDL interface in some situations (such as within observer notification callbacks).

Second, I don't think this is specific to MockRegistrar.

Correct it is not, I believe it is a property of basically any JS implementation of an XPCOM interface.

If I make some existing JS XPCOM implementation throw -- I choose Services.prompt.confirm here -- then I get the same failures and logging. This is from a hacked up test that just invokes the throwing method:

 0:01.36 PASS run_test - [run_test : 56] "undefined" === "undefined"
 0:01.36 pid:15192 JavaScript error: /Users/nalexander/Mozilla/objdirs/objdir-browser-compile/_tests/xpcshell/xpcom/tests/unit/test_notxpcom_scriptable.js, line 30: NS_ERROR_FAILURE: 
 0:01.36 PASS run_test - [run_test : 60] . Should throw
 0:01.36 INFO (xpcshell/head.js) | test MAIN run_test finished (1)

What's strange is that the "Should throw" line is from an Assert.throws(...). That is, the exception is thrown and caught, but it still "escapes" to some larger context (that prints the JavaScript error: line). It seems like it would be hard to use exceptions from JS-implemented XPCOM objects at all with this behaviour, so maybe there is some test-specific thing that's changing the behaviour?

The error escaping to a larger context I believe is due to us reporting the error on the scope's onerror callback when it is caught in the XPConnect layer, probably around here: https://searchfox.org/mozilla-central/rev/648a427a0ffc4c62118dbb24bcd88a6b52f54d78/js/xpconnect/src/XPCWrappedJSClass.cpp#574-607. We have some special casing around that to make certain non-error exceptions (like NS_ERROR_NO_INTERFACE or NS_BASE_STREAM_WOULD_BLOCK) not be logged as errors.

In normal operation I don't think this causes problems most of the time, an extra error is logged in the console and that's largely it. These logs can be helpful when debugging things, as C++ callers of JS XPCOM interfaces generally don't propagate the JS exception information around. We do log errors in cases where they are recovered from in debug builds, especially when information is being lost, so this seems OK. I think the main reason it's a problem in comment 0 is because of the logic being run within a test, and the test is detecting the uncaught exception error log, causing a test failure.

In the second case from comment 3 with the Services.prompt.confirm method, it vaguely appears that the test did not fail, despite logging the error (though confirming that might be useful). This is probably because the error was logged from within the shared system global (JSM/sys.mjs global), rather than from within the test's window global, which is what has an onerror callback registered.

Can you shed light on what's happening, and whether this can be worked around?

Hopefully the other comment here can shed some light on what's happening under the hood. In terms of avoiding the test failure, the easiest way is probably to tell the test runner that you're expecting an uncaught exception by calling SimpleTest.expectUncaughtException() (https://searchfox.org/mozilla-central/rev/648a427a0ffc4c62118dbb24bcd88a6b52f54d78/testing/mochitest/tests/SimpleTest/SimpleTest.js#1753-1760).