Closed Bug 1140482 Opened 5 years ago Closed 4 years ago

Remove hack in NativeDefineProperty to avoid resolve hook recursion

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: jorendorff, Assigned: jorendorff)

References

Details

Attachments

(1 file)

js::NativeDefineProperty doesn't trigger resolve hooks in certain cases. We have a separate js::StandardDefineProperty that does always call the resolve hook. Bug 1125624 seeks to unify the two. Which behavior should we keep?

1.  Always calling the resolve hook causes problems. A comment in js::NativeDefineProperty explains:

    > If we did a normal lookup here, it would cause resolve hook recursion in
    > the following case. Suppose the first script we run in a lazy global is
    > |parseInt()|.
    >
    > - js::InitNumberClass is called to resolve parseInt.
    > - js::InitNumberClass tries to define the Number constructor on the
    >     global.
    > - We end up here.
    > - This lookup for 'Number' triggers the global resolve hook.
    > - js::InitNumberClass is called again, this time to resolve Number.
    > - It creates a second Number constructor, which trips an assertion.

2.  Always skipping the resolve hook gives nonstandard behavior. Consider:

        Object.defineProperty(function f(){}, "length", {value: 3});

    f.length is initially unresolved. What happens if we skip the resolve
    hook? Then there is no existing property; the omitted descriptor fields
    to default to {configurable: false, writable: false}. This is wrong; the
    existing property is configurable and writable and we shouldn't be
    changing it.

Plan: in bug 1125624, add a `callResolveHook` boolean parameter to NativeDefineProperty. Default to false, but explicitly pass true at call sites where standard behavior is wanted. This follow-up bug is to fix it so that we can default to true (standard behavior) everywhere, except when defining properties from a resolve hook, the specific case where we need to avoid recursion.

If all goes well we can also remove the recursion detection stuff.
At first I had

    extern bool
    NativeDefinePropertyResolving(JSContext* cx, HandleNativeObject obj, HandleId id,
                                  HandleValue value, unsigned attrs,
                                  JSGetterOp getter=nullptr, JSSetterOp setter=nullptr);

and

    namespace JS {
    extern JS_PUBLIC_API(bool)
    DefinePropertyResolving(JSContext* cx, HandleObject obj, HandleId id,
                            Handle<JSPropertyDescriptor> desc);

    extern JS_PUBLIC_API(bool)
    DefinePropertyResolving(JSContext* cx, HandleObject obj, HandleId id, HandleValue value,
                            unsigned attrs);
    }

and Waldo liked this approach... but that patch was about twice as big as the one I'm going to post, and mostly busywork. :-P

With separate APIs for resolving:
 18 files changed, 192 insertions(+), 109 deletions(-)
With an attribute, JSPROP_RESOLVING:
 11 files changed, 74 insertions(+), 59 deletions(-)
Blocks: 1125624
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
Comment on attachment 8601026 [details] [diff] [review]
Add JSPROP_RESOLVING. Give NativeDefineProperty standard behavior in cases where a non-resolving define needs to trigger a resolve hook

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

::: js/src/jsapi-tests/testResolveRecursion.cpp
@@ +11,5 @@
>   * Test that resolve hook recursion for the same object and property is
>   * prevented.
>   */
> +BEGIN_TEST(testResolveRecursion)
> +    {

This extra level of indentation isn't consistent with any other JSAPI test I'm aware of.  We might argue it should be, though I'm not sure about that, but at the absolute minimum reindent in another patch.

@@ +150,3 @@
>      }
>  
> +    const JSClass *getGlobalClass() override {

* in the wrong place now.

::: js/src/jsfun.cpp
@@ +409,5 @@
> +    // Per ES5 15.3.5.2 a user-defined function's .prototype property is
> +    // initially non-configurable, non-enumerable, and writable.
> +    RootedValue protoVal(cx, ObjectValue(*proto));
> +    return DefineProperty(cx, fun, id, protoVal, nullptr, nullptr,
> +                          JSPROP_PERMANENT | JSPROP_RESOLVING);

Assert id is "prototype"?
Attachment #8601026 - Flags: review?(jwalden+bmo) → review+
Reverted the indentation thing, fixed the wayward *, added the assertion (at the top of the function, since it's really part of this function's contract).
Comment on attachment 8601026 [details] [diff] [review]
Add JSPROP_RESOLVING. Give NativeDefineProperty standard behavior in cases where a non-resolving define needs to trigger a resolve hook

>+++ b/js/src/jsfun.cpp

>-static JSObject*
>-ResolveInterpretedFunctionPrototype(JSContext* cx, HandleObject obj)
>+static bool
>+ResolveInterpretedFunctionPrototype(JSContext* cx, HandleFunction fun, HandleId id)
> {
[...]
>     if (isStarGenerator)
>         objProto = GlobalObject::getOrCreateStarGeneratorObjectPrototype(cx, global);
>     else
>-        objProto = obj->global().getOrCreateObjectPrototype(cx);
>+        objProto = fun->global().getOrCreateObjectPrototype(cx);
>     if (!objProto)
>         return nullptr;


You changed the return type here, but didn't update the return statements (at least, not the 'return nullptr' ones).

This caused these build warnings (treated as error) for me, with clang 3.6 on Linux:
{
js/src/jsfun.cpp:391:16: error: implicit conversion of nullptr constant to 'bool' [-Werror,-Wnull-conversion]
        return nullptr;
        ~~~~~~ ^~~~~~~
               false
js/src/jsfun.cpp:396:16: error: implicit conversion of nullptr constant to 'bool' [-Werror,-Wnull-conversion]
        return nullptr;
        ~~~~~~ ^~~~~~~
               false
js/src/jsfun.cpp:405:20: error: implicit conversion of nullptr constant to 'bool' [-Werror,-Wnull-conversion]
            return nullptr;
            ~~~~~~ ^~~~~~~
                   false
}

jorendorff, could you push a followup to fix the return statements?
Flags: needinfo?(jorendorff)
https://hg.mozilla.org/mozilla-central/rev/5232dd059c11
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Flags: needinfo?(jorendorff)
You need to log in before you can comment on or make changes to this bug.