I see at least a handful of users who aren't null-checking, even passing on the result to methods that I predict don't expect a null-check.

Guilty here. Thought it was infallible. Shame on me! Docs are quite clear in this regard.

Another fun little wrinkle: assuming JS_InstanceOf is null-safe doesn't work, because that appears capable of double-reporting with certain methods of use (when the argv passed to JS_InstanceOf is non-null), and it appears we have some such uses.

(In reply to comment #2) > Another fun little wrinkle: assuming JS_InstanceOf is null-safe doesn't work, > because that appears capable of double-reporting with certain methods of use > (when the argv passed to JS_InstanceOf is non-null), and it appears we have > some such uses. What double-reporting? It's an old API "feature" that passing non-null argv to JS_InstanceOf helps it report the error for the caller, and with better blame. Good catch on thisObject fallibility. My secret shame! It's all "with"'s fault. We should try to get rid of thisObject if we can. /be

Comment 3 is talking about OBJ_THIS_OBJECT and JSObjectOps.thisObject, of course. JS_THIS/JS_THIS_OBJECT/JS_ComputeThis are a whole rerun of that bad old show, for JSFastNatives. Change the channel! /be

JS_THIS_OBJECT returns null, but before it does so it must report the error -- OOM, say, for a this-object to wrap a JSString. Now consider passing that in to JS_InstanceOf with a non-null vp. If you do that, then you hit an arm that tries to convert the callee value to a function (which could fail, of course) to report a nice error message. Why isn't that second report a double-report? What am I missing?

You're right -- that's a failure to check for failure :-P. I thought you were saying there's a double-report under JS_InstanceOf, but you're right that treating JS_THIS_OBJECT, or any API, as infallible is a bug that can result in double error reports/exceptions. If the first API called is really fallible, the embedding has to check, no way around it short of C++ exceptions. Trying to suppress second report due to cx->throwing is the way to madness: it will hide bugs and break legitimate code paths (generators, e.g.). Just noting, not suggesting you proposed this. /be

Bug 515496 could make JS_THIS_OBJECT infallible.

Comment on attachment 562996 [details] [diff] [review] Null-checking JS_THIS_OBJECT Results Review of attachment 562996 [details] [diff] [review]: ----------------------------------------------------------------- I despise bracing single-line ifs like that, but I guess the style guidelines wrongly say to do that now. :-( I think you might be missing a new instance in dom/workers/Events.cpp, solely going by the count of results from a grep there, and the count of changes you've made to it. I think you're missing a fair number more in dom/workers/WorkerScope.cpp. I should probably look at this again, due to the combination of new instances in the previous paragraph and the xpconnect files having been moved and renamed between the creation of this patch and now.

- Updated patch - Note: WorkerScope.cpp seems to have been taken care of...

Comment on attachment 575019 [details] [diff] [review] Null-checking JS_THIS_OBJECT Results Review of attachment 575019 [details] [diff] [review]: ----------------------------------------------------------------- js/xpconnect/wrappers/XrayWrapper.cpp seems to have a call that doesn't get null-checked, but other than that, this looks fine. Please fix that one before you push? No need for another review just for that, I think.

Why are some of these returning false and some true?

The only ones returning true are those in dom/workers/EventTarget.cpp and I happened to follow the return value used for a failed GetPrivate.