Last Comment Bug 498543 - Audit users of JS_THIS_OBJECT in SpiderMonkey for correctly null-checking the result
: Audit users of JS_THIS_OBJECT in SpiderMonkey for correctly null-checking the...
Status: RESOLVED FIXED
[good first bug]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Felix Fung (:felix)
:
Mentors:
Depends on:
Blocks: 408416
  Show dependency treegraph
 
Reported: 2009-06-15 18:47 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2011-12-02 06:34 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Null-checking JS_THIS_OBJECT Results (21.93 KB, patch)
2011-09-28 00:31 PDT, Felix Fung (:felix)
jwalden+bmo: review-
Details | Diff | Splinter Review
Null-checking JS_THIS_OBJECT Results (21.36 KB, patch)
2011-11-16 15:34 PST, Felix Fung (:felix)
jwalden+bmo: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2009-06-15 18:47:25 PDT
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.
Comment 1 Wesley W. Garland 2009-06-15 18:50:32 PDT
Guilty here. Thought it was infallible. Shame on me! Docs are quite clear in this regard.
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2009-06-15 18:58:15 PDT
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.
Comment 3 Brendan Eich [:brendan] 2009-06-15 20:51:26 PDT
(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 4 Brendan Eich [:brendan] 2009-06-15 20:53:16 PDT
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
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2009-06-15 22:53:55 PDT
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?
Comment 6 Brendan Eich [:brendan] 2009-06-15 23:11:29 PDT
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
Comment 7 Jason Orendorff [:jorendorff] 2009-10-19 07:35:19 PDT
Bug 515496 could make JS_THIS_OBJECT infallible.
Comment 8 Felix Fung (:felix) 2011-09-28 00:31:24 PDT
Created attachment 562996 [details] [diff] [review]
Null-checking JS_THIS_OBJECT Results
Comment 9 Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-16 10:20:52 PST
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.
Comment 10 Felix Fung (:felix) 2011-11-16 15:34:11 PST
Created attachment 575019 [details] [diff] [review]
Null-checking JS_THIS_OBJECT Results

- Updated patch
- Note: WorkerScope.cpp seems to have been taken care of...
Comment 11 Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-30 14:52:53 PST
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.
Comment 13 Marco Bonardo [::mak] 2011-12-02 03:21:59 PST
https://hg.mozilla.org/mozilla-central/rev/c2e914523306
Comment 14 :Ms2ger (⌚ UTC+1/+2) 2011-12-02 06:15:10 PST
Why are some of these returning false and some true?
Comment 15 Felix Fung (:felix) 2011-12-02 06:34:51 PST
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.

Note You need to log in before you can comment on or make changes to this bug.