Closed
Bug 1514776
Opened 6 years ago
Closed 6 years ago
Potential problems with uncaught exception handling and same-compartment realms
Categories
(Core :: XPConnect, enhancement, P2)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla66
Tracking | Status | |
---|---|---|
firefox66 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
Details
Attachments
(2 files)
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•6 years ago
|
||
> 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...
Assignee | ||
Comment 2•6 years ago
|
||
(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...
Assignee | ||
Comment 3•6 years ago
|
||
(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•6 years ago
|
||
> 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.
Assignee | ||
Comment 5•6 years ago
|
||
(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.
Updated•6 years ago
|
Priority: -- → P2
Assignee | ||
Comment 7•6 years ago
|
||
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?
Flags: needinfo?(dkeeler)
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...
Flags: needinfo?(dkeeler)
Assignee | ||
Comment 10•6 years ago
|
||
(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?
It's C++ in production.
Assignee | ||
Comment 12•6 years ago
|
||
(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.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Comment 14•6 years ago
|
||
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•6 years ago
|
||
Pushed by jdemooij@mozilla.com: 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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e36d05affcff https://hg.mozilla.org/mozilla-central/rev/c903ba5922f2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox66:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in
before you can comment on or make changes to this bug.
Description
•