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

+1000000

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.

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.

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.

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 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 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.

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

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

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