Audit users of JS_THIS_OBJECT in SpiderMonkey for correctly null-checking the result

RESOLVED FIXED in mozilla11

Status

()

Core
JavaScript Engine
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: Waldo, Assigned: felix)

Tracking

(Blocks: 1 bug)

Trunk
mozilla11
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 1 obsolete attachment)

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

8 years ago
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
Blocks: 408416
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
Whiteboard: [good first bug]
Bug 515496 could make JS_THIS_OBJECT infallible.
(Assignee)

Comment 8

6 years ago
Created attachment 562996 [details] [diff] [review]
Null-checking JS_THIS_OBJECT Results
Attachment #562996 - Flags: review?(jeff.walden)
(Assignee)

Updated

6 years ago
Assignee: general → ffung
Attachment #562996 - Flags: review?(jeff.walden) → review?(jwalden+bmo)
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.
Attachment #562996 - Flags: review?(jwalden+bmo) → review-
(Assignee)

Comment 10

6 years ago
Created attachment 575019 [details] [diff] [review]
Null-checking JS_THIS_OBJECT Results

- Updated patch
- Note: WorkerScope.cpp seems to have been taken care of...
Attachment #562996 - Attachment is obsolete: true
Attachment #575019 - Flags: review?(jwalden+bmo)
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.
Attachment #575019 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 12

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2e914523306
https://hg.mozilla.org/mozilla-central/rev/c2e914523306
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Why are some of these returning false and some true?
(Assignee)

Comment 15

6 years ago
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.
You need to log in before you can comment on or make changes to this bug.