785483 - No JSVAL_IS_OBJECT function exists

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[Josh Matthews [:jdm]]

This should be corrected in a similar fashion to the other JSVAL_IS_foo functions ins jsapi.h (http://mxr.mozilla.org/mozilla-central/source/js/src/jsapi.h).

Comment 1

[Tom S. (please needinfo tschuster)]

Is this really so common that we need a function for this? I would rather introduce JSVAL_IS_OBJECT again, which this time really only is Value::isObject and also change OBJECT_TO_JSVAL to not accept null.

Comment 2

[Josh Matthews [:jdm]]

A JSVAL_IS_OBJECT would be fine with me. I just want some kind of parity with what existed a few months ago for C API consumers like the Servo Rust bindings.

Comment 3

[Bhimsen]

I would like to work on this bug. Can you please tell me what should be the outcome this function.

Comment 4

[Josh Matthews [:jdm]]

If you look at the JSVAL_IS_foo functions in jsapi.h, you'll see a pattern. The functions return whether a given jsval has a certain type. In this case, we want to follow Tom's suggestions from comment 1.

Comment 5

[Bhimsen]

Attachment: This patch includes the function definition JSVAL_IS_OBJECT.

This bug required to implement JSVAL_IS_OBJECT method. 
The attachment includes a patch implementing that method.

Comment 6

[Josh Matthews [:jdm]]

Comment on attachment 657254 [details] [diff] [review]
This patch includes the function definition JSVAL_IS_OBJECT.

Thanks Bhimsen! However, Tom suggested that we should change OBJECT_TO_JSVAL to not accept null jsvals as well. Could you make that change?

Also, for future reference, you should set the flags to ? and give a name. + is used by the person giving feedback/review to indicate a positive result.

Also _also_, if you could follow the steps at https://developer.mozilla.org/en/Creating_a_patch_that_can_be_checked_in when generating your patches, it would make them easier to check in (particularly the author and commit message are appreciated).

Comment 7

[Bhimsen]

(In reply to Josh Matthews [:jdm] from comment #6)
> Comment on attachment 657254 [details] [diff] [review]
> This patch includes the function definition JSVAL_IS_OBJECT.
> 
> Thanks Bhimsen! However, Tom suggested that we should change OBJECT_TO_JSVAL
> to not accept null jsvals as well. Could you make that change?
> 
> Also, for future reference, you should set the flags to ? and give a name. +
> is used by the person giving feedback/review to indicate a positive result.
> 
> Also _also_, if you could follow the steps at
> https://developer.mozilla.org/en/Creating_a_patch_that_can_be_checked_in
> when generating your patches, it would make them easier to check in
> (particularly the author and commit message are appreciated).

I am bit confused. Can you please tell me more about the second task. Because the OBJECT_TO_JSVAL takes pointer to a JSObject and not to jsval. So there would be no point of not accepting null jsvals.

Comment 8

[Josh Matthews [:jdm]]

My mistake, I misspoke. I believe Tom wants to make OBJECT_TO_JSVAL only handle non-null JSObject* values, but I think he should clarify.

Comment 9

[Josh Matthews [:jdm]]

Resetting the owner of this task that hasn't seen any activity in a while.

Comment 10

[joseph]

Attachment: Implementation of JSVAL_IS_OBJECT and changing OBJECT_TO_JSVAL to handle non-null JSObject*

Please add any documentation if needed.

Comment 11

[Josh Matthews [:jdm]]

Comment on attachment 680489 [details] [diff] [review]
Implementation of JSVAL_IS_OBJECT and changing OBJECT_TO_JSVAL to handle non-null JSObject*

Thanks, Joseph. Let's see what the JS team thinks.

Comment 12

[Jeff Walden [:Waldo]]

Comment on attachment 680489 [details] [diff] [review]
Implementation of JSVAL_IS_OBJECT and changing OBJECT_TO_JSVAL to handle non-null JSObject*

Review of attachment 680489 [details] [diff] [review]:
-----------------------------------------------------------------

This isn't something we should resolve here, probably.  But now that SpiderMonkey is C++, I think we should consider removing all these macro-style functions.  JS::Value exposes all these things in a much nicer, C++ way.  I think we should make that the only way to access this stuff, in the interests of having only one way to do things, and in the interest of simplifying all the IMPL guts that currently underlie all this (and do their best to make this much harder to follow, I believe).

In the meantime, til we can get people using JS::Value, this is probably not unreasonable.  Although I worry that, by providing this, people will use it in preference to JS::Value, which gets in the way of convincing people to move to the C++ API...

Anyway.  All that to the side, the JSVAL_TO_OBJECT note definitely needs to be resolved before this can move along.  And since this is JSAPI stuff, it needs dmandelin's owner approval of it -- please also flag :dmandelin for review on the next patch.

::: js/src/jsapi.h
@@ +2140,5 @@
>  
> +static JS_ALWAYS_INLINE JSBool
> +JSVAL_IS_OBJECT(jsval v)
> +{
> +    return JSVAL_IS_OBJECT_IMPL( JSVAL_TO_IMPL( v ) );

SpiderMonkey code style wouldn't add spaces in this:

    return JSVAL_IS_OBJECT_IMPL(JSVAL_TO_IMPL(v));

@@ +2148,5 @@
>  JSVAL_TO_OBJECT(jsval v)
>  {
>      JS_ASSERT(JSVAL_IS_OBJECT_OR_NULL_IMPL(JSVAL_TO_IMPL(v)));
>      return JSVAL_TO_OBJECT_IMPL(JSVAL_TO_IMPL(v));
>  }

It seems to me that if we're going to have this, JSVAL_IS_OBJECT, JSVAL_TO_OBJECT, and OBJECT_TO_JSVAL *must* be consistent.  Right now, it looks like JSVAL_TO_OBJECT will perfectly happily accept JSVAL_NULL and give you back a null pointer.  This needs to be resolved before this goes anywhere.

Comment 13

[joseph]

Attachment: JSVAL_TO_OBJECT doesn't accept JSVAL_NULL anymore.

Comment 14

[Jeff Walden [:Waldo]]

Comment on attachment 680860 [details] [diff] [review]
JSVAL_TO_OBJECT doesn't accept JSVAL_NULL anymore.

Review of attachment 680860 [details] [diff] [review]:
-----------------------------------------------------------------

So, this looks fine as far as it goes.

But it belatedly occurs to me that really, this is a crazy huge change for anyone not already assuming the patch's semantics.  Will every other embedder be okay with this change?

Even supposing they're all okay, Gecko itself still has to deal with this change.  We have three hundred-odd calls to JSVAL_TO_OBJECT in the tree; at a very brief glance at the MXR page, many appear to assume non-nullness, but some may not.  The best thing would be to audit every location to make sure it's okay with this semantics change...which is kind of a huge request.  But I think it'd need to happen if we were to take this patch.

http://mxr.mozilla.org/mozilla-central/search?string=jsval_to_object

And honestly, if we were to audit everything, I think I'd want us to take the opportunity and convert them all to using JS::Value's toObject() method, which has since its introduction always and only worked on non-null object values.

But I guess this conundrum -- that we probably need to audit every caller in the tree -- doesn't need to be addressed until dmandelin says he's okay with this functionality change.  And he might even refer it to the js-engine list/newsgroup to be sure, anyway.  So I'm happy enough with the patch, but we'll likely need to do more work than this if we want to land it -- which fact is still yet to be determined, to be sure.

dmandelin, what's your opinion here?

Comment 15

[David Mandelin [:dmandelin]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #14)
> Comment on attachment 680860 [details] [diff] [review]
> JSVAL_TO_OBJECT doesn't accept JSVAL_NULL anymore.
> 
> Review of attachment 680860 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So, this looks fine as far as it goes.

Thanks for the careful analysis, Jeff.

> But it belatedly occurs to me that really, this is a crazy huge change for
> anyone not already assuming the patch's semantics.  Will every other
> embedder be okay with this change?
> 
> Even supposing they're all okay, Gecko itself still has to deal with this
> change.  We have three hundred-odd calls to JSVAL_TO_OBJECT in the tree; at
> a very brief glance at the MXR page, many appear to assume non-nullness, but
> some may not.  The best thing would be to audit every location to make sure
> it's okay with this semantics change...which is kind of a huge request.  But
> I think it'd need to happen if we were to take this patch.

I think you're right. Unless I'm missing something, it's risky to land this patch without ensuring that the Gecko calls still make sense.

> http://mxr.mozilla.org/mozilla-central/search?string=jsval_to_object
> 
> And honestly, if we were to audit everything, I think I'd want us to take
> the opportunity and convert them all to using JS::Value's toObject() method,
> which has since its introduction always and only worked on non-null object
> values.
> 
> But I guess this conundrum -- that we probably need to audit every caller in
> the tree -- doesn't need to be addressed until dmandelin says he's okay with
> this functionality change.  And he might even refer it to the js-engine
> list/newsgroup to be sure, anyway.  So I'm happy enough with the patch, but
> we'll likely need to do more work than this if we want to land it -- which
> fact is still yet to be determined, to be sure.
> 
> dmandelin, what's your opinion here?

It depends on what we're trying to accomplish here:

1. If it's to add a C API for Rust, then the answer is that SpiderMonkey doesn't have a C API, and Rust users should feel free to implement such a function in their own code. If someone out there really does need a C API, I think it's a fine idea to create an (unsupported, for now) C wrapper API that can live in SpiderMonkey. Anyway, if that's the goal, there is no reason to change Gecko.

2. If it's also to make JSAPI more consistent, then we can do that, but we won't risk creating bugs at hundreds of sites in Gecko for it. The patch does the right thing with these APIs to make them consistent. I think the right thing for Gecko is to make a JSVAL_TO_OBJECT_OR_NULL and have Gecko call that, or use Value::toObject. It should be possible to do that mostly with python/sed/whatever.

I'm assuming it's #2, so to land, this should change Gecko to call an API with the old semantics. A green try server run would be great if we can arrange that without too much inconvenience. I'll mark the patch r- to record the fact that we don't want to land it in this form, but it's without prejudice.

Comment 16

[joseph]

> I'm assuming it's #2, so to land, this should change Gecko to call an API
> with the old semantics. A green try server run would be great if we can
> arrange that without too much inconvenience. I'll mark the patch r- to
> record the fact that we don't want to land it in this form, but it's without
> prejudice.

Okay, that is not a problem at all. But I did not notice what is final decision. Is there anything I can do now or it depends on some internal affair and we will wait until is resolved?

Cheers,

Comment 17

[Tom S. (please needinfo tschuster)]

We completely switched to the C++ JS::Value methods.