Closed Bug 1094176 Opened 5 years ago Closed 5 years ago

Remove lookup API

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: evilpie, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

The idea of the whole lookup API is fundamentally flawed in the light of ES5/6.
+1000000
Assignee: nobody → evilpies
Attached patch Remove JS_Lookup* from js (obsolete) — Splinter Review
For own uses of JS_Lookup it's not important whether we invoked getters or not.
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 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.
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.
Attachment #8517471 - Attachment is obsolete: true
Attachment #8526720 - Flags: review?(jorendorff)
Attached patch v1 Remove browser usage (obsolete) — Splinter Review
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.
Attachment #8526768 - Flags: review?(bobbyholley)
What are the difference between the two APIs?
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 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.
Attachment #8526768 - Flags: review?(bobbyholley) → review-
Attached patch v2 Remove browser usage (obsolete) — Splinter Review
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.
Attachment #8526768 - Attachment is obsolete: true
Attachment #8527359 - Flags: review?(bobbyholley)
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());
Attachment #8527359 - Flags: review?(bobbyholley)
Attachment #8526720 - Flags: review?(jorendorff) → review+
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.
Attached file stack (obsolete) —
MOZ_ASSERT_IF(hasGetterOrSetter(), desc()->value.isUndefined()); doesn't always hold apparently when defining a property.
(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.
(As in, we want to keep returning false without throwing for non-existent exposedProps).
So, I am now explicitly checking hasGetterOrSetter
Attachment #8527359 - Attachment is obsolete: true
Attachment #8535580 - Attachment is obsolete: true
Attachment #8536028 - Flags: review?(bobbyholley)
Attachment #8536028 - Flags: review?(bobbyholley) → review+
https://hg.mozilla.org/mozilla-central/rev/7075d6399f43
https://hg.mozilla.org/mozilla-central/rev/ae868f859721
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Blocks: 1122293
Depends on: 1247924
You need to log in before you can comment on or make changes to this bug.