1094176 - Remove lookup API

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tom S [:evilpie]]

The idea of the whole lookup API is fundamentally flawed in the light of ES5/6.

Comment 1

[Jason Orendorff [:jorendorff]]

+1000000

Comment 2

[Tom S [:evilpie]]

Attachment: Remove JS_Lookup* from js

For own uses of JS_Lookup it's not important whether we invoked getters or not.

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 8517471 [details] [diff] [review]
Remove JS_Lookup* from js

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

::: js/src/jsapi.cpp
@@ +2644,5 @@
>      AssertHeapIsIdle(cx);
>      CHECK_REQUEST(cx);
>      assertSameCompartment(cx, obj, id);
>  
> +    return js::HasOwnProperty(cx, obj, id, foundp);

I would expect this to break things. The "Be careful not to call any lookup or resolve hooks" in the original is part of this function's contract.

Comment 4

[Tom S [:evilpie]]

Comment on attachment 8517471 [details] [diff] [review]
Remove JS_Lookup* from js

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

::: js/src/jsapi.cpp
@@ +2644,5 @@
>      AssertHeapIsIdle(cx);
>      CHECK_REQUEST(cx);
>      assertSameCompartment(cx, obj, id);
>  
> +    return js::HasOwnProperty(cx, obj, id, foundp);

Oh you are right. I guess the only change for native objects is that we invoke the resolve hook. We should probably check if that is to important to any caller. Otherwise we could just keep the old code for the native case and call HasOwnProperty in the other.

Comment 5

[Tom S [:evilpie]]

So I think we can simply replace most uses in the browser with JS_GetProperty. There are however three uses in nsDOMClassInfo that try to resolve the constructor property by looking at window[classname], this could potentially execute content code.

Comment 6

[Tom S [:evilpie]]

Attachment: v2 Remove JS_Lookup* from js

Comment 7

[Tom S [:evilpie]]

Attachment: v1 Remove browser usage

Sorry Bobby, I guess the hard part on deciding when this ok, is actually on you. Just replacing JS_LookupProperty with JS_HasProperty when possible, is a not a big deal, because it basically does the same thing under the hood.

Comment 8

[Bobby Holley (:bholley)]

What are the difference between the two APIs?

Comment 9

[Tom S [:evilpie]]

So LookupProperty, allows you to get a property's value, without invoking getters. So when you only use LookupProperty to trigger resolve hooks and don't actually use the value of the property, then it's basically equal to HasProperty. If you actually need the value, then closest thing would be to use something like GetPropertyDescriptor, but I just assumed that for these cases GetProperty is probably good enough.

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8526768 [details] [diff] [review]
v1 Remove browser usage

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

In general, I think it's safer to avoid invoking scripted getters in places where we didn't before. So we should change the JS_GetProperty instances to JS_GetPropertyDescriptor.

Comment 11

[Tom S [:evilpie]]

Attachment: v2 Remove browser usage

So I changed the case in DOMClassInfo and AccessCheck, because these are actually critical. The cases for postRILMessage and CPOWs are just unnecessary from my point of view.

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 8527359 [details] [diff] [review]
v2 Remove browser usage

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

::: dom/base/nsDOMClassInfo.cpp
@@ +1047,5 @@
>  
>    JS::Rooted<JSObject*> global(cx, ::JS_GetGlobalForObject(cx, obj));
>  
>    JS::Rooted<JS::Value> val(cx);
> +  if (!GetValueProperty(cx, global, mData->mName, &val)) {

I don't think we need the helper. I think this above should just be JS_GetPropertyDescriptor...

@@ +1052,4 @@
>      return NS_ERROR_UNEXPECTED;
>    }
>  
>    if (!val.isPrimitive()) {

...and this should be if (desc.object() && !desc.value().isObject())

And then add an assert to the value() getter:

MOZ_ASSERT_IF(!rv.isUndefined(), !rv.hasGetterOrSetter());

Comment 13

[Tom S [:evilpie]]

Comment on attachment 8527359 [details] [diff] [review]
v2 Remove browser usage

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

::: js/xpconnect/wrappers/AccessCheck.cpp
@@ +333,3 @@
>          return false;
>  
> +    if (!desc.obj() || desc.hasGetterOrSetter()) {

Should I drop the check for desc.obj()? This is a change in behavior from, non-existing __exposedProps__ returns false to throws.

Comment 14

[Tom S [:evilpie]]

Attachment: stack

MOZ_ASSERT_IF(hasGetterOrSetter(), desc()->value.isUndefined()); doesn't always hold apparently when defining a property.

Comment 15

[Bobby Holley (:bholley)]

(In reply to Tom Schuster [:evilpie] from comment #13)
> Comment on attachment 8527359 [details] [diff] [review]
> v2 Remove browser usage
> 
> Review of attachment 8527359 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/xpconnect/wrappers/AccessCheck.cpp
> @@ +333,3 @@
> >          return false;
> >  
> > +    if (!desc.obj() || desc.hasGetterOrSetter()) {
> 
> Should I drop the check for desc.obj()? This is a change in behavior from,
> non-existing __exposedProps__ returns false to throws.

Yes, please do that.

Comment 16

[Bobby Holley (:bholley)]

(As in, we want to keep returning false without throwing for non-existent exposedProps).

Comment 17

[Tom S [:evilpie]]

Attachment: v3 Remove browser usage

So, I am now explicitly checking hasGetterOrSetter

Comment 18

[Tom S [:evilpie]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7075d6399f43
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae868f859721

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7075d6399f43
https://hg.mozilla.org/mozilla-central/rev/ae868f859721