WebIDL Dictionary ToObject assert that |parentObject| is non-null

NEW
Unassigned

Status

()

Core
DOM
5 years ago
5 years ago

People

(Reporter: bholley, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

5 years ago
Looking at the CodeGen for, say, AsyncScrollEventDetail::ToObject, it looks like the outparam is a MutableHandleValue, and we don't even use |parentObject|, because we're always converting to a primitive. This is problematic in a number of ways:

1 - ToObject is, in general, a misnomer. And it's a linguistic mismatch with SpiderMonkey, where |toObject| means "I know this thing is most definitely an object - assert if it isn't".

2 - It encourages people to pull tricks like this:

298   // We can get away with a null global here because
299   // AsyncScrollEventDetail only contains numeric values.
300   if (!detail.ToObject(cx, JS::NullPtr(), &val)) {
301     MOZ_CRASH("Failed to convert dictionary to JS::Value due to OOM.");
302     return NS_ERROR_FAILURE;
303   }

(From BrowserElementParent.cpp).

This is a footgun, because if anyone ever alters the semantics of the dictionary to (perhaps very rarely) create an object, we're not likely to fix the callsite until someone notices the crash.

IMO, this function should be renamed "ToJSVal", and should assert that the global is non-null. If we wanted, we could optionally add "ToPrimitiveJSVal", which wouldn't require a parent, for use by BrowserElementParent.cpp and friends.
> This is a footgun

How would being called ToJSVal make this less of a footgun?

> and should assert that the global is non-null.

That's an option, yes.

> If we wanted, we could optionally add "ToPrimitiveJSVal"

I'm not sure that naming makes sense.  The return value of ToObject is a JSObject* stored in a JS::Value.  The only reason a global is not needed is that none of the _properties_ of that object need a global for wrapping.  It's called ToObject because it always produces a value that tests isObject() if it doesn't throw.

Now what we could do is require a global but have a 2-argument signature of ToObject/ToJSValue only on dictionaries which have primitive-only members, to reduce the footgunness...
(Reporter)

Comment 2

5 years ago
(In reply to Boris Zbarsky [:bz] from comment #1)
> > This is a footgun
> 
> How would being called ToJSVal make this less of a footgun?

The naming issue is separate from the issue of allowing people to pass null. The former is confusing, the latter is a footgun.

> > If we wanted, we could optionally add "ToPrimitiveJSVal"
> 
> I'm not sure that naming makes sense.  The return value of ToObject is a
> JSObject* stored in a JS::Value.

Oh, I see. If that's the case, then the function should take a MutableHandleObject rather than a MutableHandleValue. Is there any reason not to do that?
 
> Now what we could do is require a global but have a 2-argument signature of
> ToObject/ToJSValue only on dictionaries which have primitive-only members,
> to reduce the footgunness...

Ok, that sounds good. Altering the bug title to fix the two issues that are bonafide bugs IMO. We can do the last part as an enhancement if we want.
Summary: WebIDL Dictionary jsval conversion routine shouldn't be called 'ToObject' → WebIDL Dictionary ToObject should take a MutableHandleObject and assert that |parentObject| is non-null
> Is there any reason not to do that?

Yes.  For the common/designed use case (codegen) it would require an extra RootedObject or something.  Codegen always wraps into a MutableHandleValue, since that's what args.rval() is.
(Reporter)

Comment 4

5 years ago
(In reply to Boris Zbarsky [:bz] from comment #3)
> > Is there any reason not to do that?
> 
> Yes.  For the common/designed use case (codegen) it would require an extra
> RootedObject or something.  Codegen always wraps into a MutableHandleValue,
> since that's what args.rval() is.

Oh. :-(
Summary: WebIDL Dictionary ToObject should take a MutableHandleObject and assert that |parentObject| is non-null → WebIDL Dictionary ToObject assert that |parentObject| is non-null
You need to log in before you can comment on or make changes to this bug.