1514776 - Potential problems with uncaught exception handling and same-compartment realms

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement, P2)

Description

[Jan de Mooij [:jandem]]

Bug 1514210 hit an interesting problem on Try:

(1) We create an nsXPCWrappedJS for a JS-implemented runnable.

(2) We run() it at some point. The JS code throws an exception.

(3) The AutoJSAPI destructor in the nsXPCWrappedJS code tries to report it to the window.onerror handler based on the nsXPCWrappedJS's global.

The problem is that we have to be very careful about using the right global for the nsXPCWrappedJS. Using CurrentGlobalOrNull(cx) caused test failures because that was a sandbox global so we failed to call the onerror event handler. I changed it to use the JS object's global if the object is not a CCW - it fixed the test but I'm worried it's still possible to hit similar issues in the CCW case or elsewhere.

Comment 1

[Boris Zbarsky [:bzbarsky]]

> Using CurrentGlobalOrNull(cx) caused test failures 

Where is the relevant CurrentGlobalOrNull call?

nsXPCWrappedJSClass::CallMethod looks like it does an AutoEntryScript based on an UncheckedUnwrap of the "obj" of the nsXPCWrappedJS.  That would report things on the global of that underlying object, I would think, which makes sense to me...

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #1)
> Where is the relevant CurrentGlobalOrNull call?

https://searchfox.org/mozilla-central/rev/1a4a7745fd4a14402a0de0e9abc040724d4fcf8f/js/xpconnect/src/XPCWrappedJS.cpp#388

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #1)
> nsXPCWrappedJSClass::CallMethod looks like it does an AutoEntryScript based
> on an UncheckedUnwrap of the "obj" of the nsXPCWrappedJS.  That would report
> things on the global of that underlying object, I would think, which makes
> sense to me...

I think we ended up reporting it here:

https://searchfox.org/mozilla-central/rev/1a4a7745fd4a14402a0de0e9abc040724d4fcf8f/js/xpconnect/src/XPCWrappedJSClass.cpp#829

Called here:

https://searchfox.org/mozilla-central/rev/1a4a7745fd4a14402a0de0e9abc040724d4fcf8f/js/xpconnect/src/XPCWrappedJSClass.cpp#1148

That's within the JSAutoRealm scope here:

https://searchfox.org/mozilla-central/rev/1a4a7745fd4a14402a0de0e9abc040724d4fcf8f/js/xpconnect/src/XPCWrappedJSClass.cpp#960

And AutoJSAPI::ReportException also uses JS::CurrentGlobalOrNull...

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> I think we ended up reporting it here:

Maybe we can just stop doing that somehow, after all the exception handling clean up the past years?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> https://searchfox.org/mozilla-central/rev/1a4a7745fd4a14402a0de0e9abc040724d4fcf8f/js/xpconnect/src/XPCWrappedJS.cpp#388

OK.  So that's in the compartment of "jsObj".  That matches how webidl callbacks work...

> That's within the JSAutoRealm scope here:

OK, that's different from webidl callbacks.  For those, CallSetup::~CallSetup exits the realm of the callback (and hence reenters the realm of the callback's unwrapped object) before doing any exception-reporting, and has a nice comment explaining why that needs to happen.

> And AutoJSAPI::ReportException also uses JS::CurrentGlobalOrNull...

Right.  It's expecting to be called from ~AutoJSAPI, at which point we're in the "right" compartment if the AutoEntryScript was set up correctly...

> Maybe we can just stop doing that somehow, after all the exception handling clean up the past years?

It's possible.  The code is pretty complicated...  Maybe we can just remove the explicit ReportException call and have that happen when the AutoEntryScript goes out of scope (basically right when CheckForException returns, since its caller immediately returns too).

Two other options:

1) Both calls to CheckForException are tail-calls in the sense that they return immediately.  So we could make the relevant JSAutoRealm a Maybe and explicitly destroy it ... somewhere.

2) We could explicitly pass the "global to report to" into CheckForException and enter that Realm before doing the ReportException call.

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #4)
> It's possible.  The code is pretty complicated...  Maybe we can just remove
> the explicit ReportException call and have that happen when the
> AutoEntryScript goes out of scope (basically right when CheckForException
> returns, since its caller immediately returns too).

I tried this but there's an AutoSaveExceptionState hidden in the AutoScriptEvaluate in CallMethod unfortunately.

> 2) We could explicitly pass the "global to report to" into CheckForException
> and enter that Realm before doing the ReportException call.

This one works in some local tests. This code is a nest of Auto classes so it's sort of nice to be explicit about this.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1514776 - Enter the unwrapped object's realm before calling aes.ReportException() in nsXPCWrappedJSClass::CheckForException. r?bzbarsky

Depends on D14850

Comment 7

[Jan de Mooij [:jandem]]

Dana, I get one test failure with this patch, in browser_loadPKCS11Module_ui.js

That test throws an (expected) exception here: https://searchfox.org/mozilla-central/rev/2e5e28f518524f8af5b158ddb605022b6a2d68cf/security/manager/ssl/tests/mochitest/browser/browser_loadPKCS11Module_ui.js#29

Before this patch, we would dump this exception to the browser console and move on. Now we try to report it to the test window and the test fails.

Adding the following to the test fixes it for me:

  window.onerror = function(message) {
      Assert.equal(message, `Error: addModule: Throwing exception`);
  }

Any thoughts?

Comment 8

[Jan de Mooij [:jandem]]

Oh I can probably call expectUncaughtException()

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

I'm not understanding something about how this works - shouldn't the exception thrown in the mock nsIPKCS11ModuleDB implementation be caught in https://searchfox.org/mozilla-central/rev/232ced2697b8938073fa79b8e6aa3718876c0b69/security/manager/pki/resources/content/load_device.js#39 ? From my reading there shouldn't be any reported exception at all...

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Dana Keeler [:keeler] (she/her) (use needinfo) from comment #9)
> I'm not understanding something about how this works - shouldn't the
> exception thrown in the mock nsIPKCS11ModuleDB implementation be caught in
> https://searchfox.org/mozilla-central/rev/
> 232ced2697b8938073fa79b8e6aa3718876c0b69/security/manager/pki/resources/
> content/load_device.js#39 ? From my reading there shouldn't be any reported
> exception at all...

The problem is that we end up in XPConnect land for the addModule call there and then we have an AutoEntryScript that takes care of reporting the exception. So as part of the addModule call we clear the exception, report it somewhere (browser console before, uncaught exception now, hence the test failure) and throw something else instead to our caller IIUC.

Maybe I should ask you this instead: what is that pkcs11ModuleDB.addModule call usually calling, in production code? Is that some JS code similar to the test, is it C++, something else?

Comment 11

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

It's C++ in production.

Comment 12

[Jan de Mooij [:jandem]]

(In reply to Dana Keeler [:keeler] (she/her) (use needinfo) from comment #11)
> It's C++ in production.

Thanks. In that case it's just a test implementation detail I think and we should just expectUncaughtException() IMO. I'll upload a patch.

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Bug 1514776 - Fix browser_loadPKCS11Module_ui.js test because we now report as uncaught exception instead of reporting to the browser console. r?keeler

Depends on D14851

Comment 14

[Kris Maglione [:kmag]]

Just as a note (I'm not saying this is either a good or bad solution): If the mock method throws an exception that looks something like `Components.Exception("...", Cr.NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)`, it will still be propagated to the caller, but won't be reported as a JS exception at the XPConnect boundary.

We should probably have a more reasonable way to create such exceptions... But for test-only code, either a hack or a whitelisted error is probably good enough.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e36d05affcff
Enter the unwrapped object's realm before calling aes.ReportException() in nsXPCWrappedJSClass::CheckForException. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/c903ba5922f2
Fix browser_loadPKCS11Module_ui.js test because we now report as uncaught exception instead of reporting to the browser console. r=keeler

Comment 16

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/e36d05affcff
https://hg.mozilla.org/mozilla-central/rev/c903ba5922f2