Proxies make this hook fallible. We can run out-of-stack or the child processes might crash in the case of CPOWs etc.

This patch applies atop the one in bug 1187234, altering the name/signature of objectClassIs to be fallible. The two together demonstrate that every relevant spot was changed in bug 1187234. It's a bit unfortunate, to be sure, that class-checks are fallible now. But given the possibility of IPC failures (currently "handled" with sadface comments saying "return false because we can't do any better" -- too bad the authors never *asked* us to fix our signature :-( ), the already-throwing behavior of SecurityWrapper for this (despite objectClassIs being "infallible"), &c. it seems necessary. The fallout of making JS_IsArrayObject, JS_ObjectIsRegexp, JS_ObjectIsDate, &c. fallible is also a bit sadmaking, but oh well. For the places that do multiple tests, it's nice to convert them to doing getBuiltinClass+multiple comparisons, sort of. Note that, given this *exposes* a class, it wasn't possible to simply refactor getBuiltinClass to also be usable for IsArray. This method doesn't look through ES6 proxies. IsArray does. This ends up dropping a little bit of assertion safety in a few places, because doing a fallible class-check might throw and change semantics. I verified the removed assertions checked out, generally. Not sure there's much else to do about them. :-\ Returning ESClass_Other seems like a nice way to expose "something else" -- generalizes to everything quite nicely. It might not be a bad idea to add ESClass_TypedArray as well, eventually -- a couple places seemed to want it, to get rid of a one-off JS_IsTypedArrayObject call. But that can be another bug, later. There are clearly places doing JS_IsArrayObject or friends that have no proper handling of JS-thrown errors at all. In such places I generally just did as in Rome, without fixing up existing error handling. :-\ Not gonna yakshave any longer here. I'll post a second patch here that's a combination of this patch and the other one, to verify the two *do* add getUnsafeTarget implementations in every place I change to getBuiltinClass. There are also a few patches I did underneath both of these, as minor naming/code cleanups to some regexp code, that I'll file in another bug. Those are all pretty trivial, tho -- no effect on the major thrust of this work. I'll probably want some other people to look at various bits of this, like Codegen.py, but let's start with JS review first.

Should be easy, given this, to verify every places that adds a getUnsafeTarget *also* changes objectClassIs, so the patch in bug 1187234 is consistent with existing objectClassIs definition spots.

Largely the same as the previous patch, just rebased atop the fresh patch in bug 1187234.

Comment on attachment 8657969 [details] [diff] [review] Patch, v2 Review of attachment 8657969 [details] [diff] [review]: ----------------------------------------------------------------- OK, all of the mechanical changes look OK to me. I am going to also enlistthe help of a dom peer to look at the various DOM code briefly to make sure the error handling is correct. The JS pieces are sound. ::: dom/bindings/Date.cpp @@ +22,5 @@ > JS::Rooted<JSObject*> obj(aCx, aObject); > + > + double msecs; > + if (!js::DateGetMsecSinceEpoch(aCx, obj, &msecs)) { > + return false; are callers here prepared for it to be fallible? ::: dom/geolocation/nsGeolocationSettings.cpp @@ +322,5 @@ > aes.TakeOwnershipOfErrorReporting(); > JSContext *cx = aes.cx(); > > + bool isArray; > + if (!JS_IsArrayObject(cx, obj, &isArray) || !isArray) { "error handling"

I won't get to this until at least Tuesday, more likely Wednesday. I agree that all this being fallible is sadfaces; need to think a bit about it, which is why more likely Wednesday.

No rush, I have a tweak or two to make anyway, since I somehow clearly failed to make the one SetTimeStamp caller fallible-correct. I will concede that not all users of all the methods here, are fully fallible-correct, sadly. :-( But many of those callers *also* were previously fallible-incorrect as far as JSAPI method calls in the same method went, so it seemed at least not a *regression*.

I'm curious as to which callers you think those are, by the way, so I can file followup bugs on them as needed.

I dealt with Date::SetTimeStamp fallibility in its one caller, also adjusted the ctypes code as discussed on IRC. Tryservering of previous iterations of this seems to only hit current intermittent oranges, maybe, so it's sort of good. But it might have bumped the frequency of one to very-high levels, so it's not necessarily 100% yet. If any tweaks needed for that (if I'm at fault) end up big, I'll bring the delta back for review, as usual. == Mishandling JSAPI failure/other failure cases == (Honestly, I'm not entirely sure how this is supposed to be handled for nsresult-returning code. I tried to sort-of assume that if different nsresults were returned, things were sensible. But really the impedance mismatch is sad, and I don't necessarily claim to understand how it should work.) Cache::FetchHandler::ResolvedCallback treats failures of JS_GetElement/etc. as Fail(), but also cases where there's no such failure but merely a value that's unexpectedly not an object (no exception pending). Either I misunderstand WebIDL error handling, or the two cases merit different handling. nsGeolocationSettings::HandleGeolocationAlwaysPreciseChange returns, doing nothing else, for both !JS_GetElement(cx, obj, i, &value) || !value.isString(), in existing code. One's an exception-pending case, one not. MmsMessage::Create returns one nsresult for the !JS_GetElement(aCx, deliveryInfoObj, i, &infoJsVal) || !infoJsVal.isObject() case. Seems like separate values for the pending-error/not-pending cases would be better. I kind-of added them, at least as regards my changes. MobileMessageThread::Create returns one value for !JS_GetElement(aCx, obj, i, &val) || !val.isString(), conflating pending/not-pending cases. mozJSComponentLoader::ImportInto conflates JSAPI-failure with bad-data failure, treating !JS_GetElement(cx, symbolsObj, i, &value) || !value.isString() || !JS_ValueToId(cx, value, &symbolId) as requiring only a single handling-path. CallMethodHelper::GetArraySizeFromParam previously conflated handling of !JS_IsArrayObject(mCallContext, maybeArray) || !JS_GetArrayLength(mCallContext, arrayOrNull, &GetDispatchParam(paramIndex)->val.u32) when the former was "infallible". Likewise, NativeJSContainerImpl::ArrayInValue conflates !val.isObject() handling with JS_GetElement failing.

Thanks for making that list. > Cache::FetchHandler::ResolvedCallback ... > Either I misunderstand WebIDL error handling, or the two cases merit different handling. This isn't a WebIDL callback. But I agree that there is bogosity here, which is subtle in various ways, not least because the contract with the caller is not clear. I filed 1206809 on it. > nsGeolocationSettings::HandleGeolocationAlwaysPreciseChange This one is not a problem, because it does: AutoEntryScript aes(global, "geolocation.always_precise indexing"); aes.TakeOwnershipOfErrorReporting(); Then ~AutoEntryScript will check for exception on the JSContext and report-and-clear it as needed. > MmsMessage::Create This is being called via XPConnect (not directly, but pretty close). As long as it returns a failure nsresult, caller will check for an exception on the JSContext and leave it there if there is one. If there is no exception, it will create a new exception from the nsresult and set it on the JSContext. So the exact nsresult returned in the case when there is already an exception on the JSContext is irrelevant. > MobileMessageThread::Create Same thing. > mozJSComponentLoader::ImportInto Looks buggy to me. Filed bug 1206817. > CallMethodHelper::GetArraySizeFromParam This one is OK because of the semantics of Throw. It will leave an existing exception in place, else create and set one based on the passed-in nsresult. > NativeJSContainerImpl::ArrayInValue This code is all totally broken in terms of error handling. And it's new code too. :( Filed bug 1206822.

Comment on attachment 8661105 [details] [diff] [review] Patch, addressing efaust review comments >+++ b/dom/bindings/Codegen.py >@@ -5452,37 +5452,53 @@ def getJSToNativeConversionInfo(type, de ... >+ if (!${testConvertible}(cx, ${val}, &isConvertible)) { >+ $*{exceptionCodeIndented} I'd prefer it if you used $*{exceptionCode}, indented it by two spaces, and then did exceptionCode=exceptionCode in the arguments to this fill() call. >+ failureCode=CGGeneric(failureCode).define(), That's the same thing as: failureCode=failureCode, except slower. Drop the CGGeneric() and define() bit. >+ conversionCode=CGGeneric(conversionCode).define()) And here. >+++ b/dom/media/webspeech/synth/nsSpeechTask.cpp > if (JS_IsInt16Array(darray)) { Is that going to become fallible too at some point? Followup bug? >+++ b/js/ipc/WrapperAnswer.cpp >+WrapperAnswer::RecvGetBuiltinClass(const ObjectId& objId, ReturnStatus* rs, I really hope the code here won't be turning JS exceptions into process aborts.... I have low confidence in my ability to do a good review on the js/ipc changes. I skipped all the js/src stuff. r=me with the above nits

I suppose if I'm being honest, I'm not certain the IPC parts of this were fully reviewed, and bz's uncertainty (which I'm pretty sure is unfounded, because careefully cargo-culted. "pretty sure") doesn't help. !summon billm

(In reply to Boris Zbarsky [:bz] from comment #11) > >+++ b/dom/media/webspeech/synth/nsSpeechTask.cpp > > if (JS_IsInt16Array(darray)) { > > Is that going to become fallible too at some point? Followup bug? It probably should. Filed bug 1207410.