Closed
Bug 1140482
Opened 9 years ago
Closed 9 years ago
Remove hack in NativeDefineProperty to avoid resolve hook recursion
Categories
(Core :: JavaScript Engine, defect)
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.
Assignee | ||
Comment 1•9 years ago
|
||
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 | ||
Comment 2•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e302e746001
Assignee | ||
Comment 3•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a29aa0b4c115
Assignee | ||
Comment 4•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9de9a2168b55
Assignee | ||
Comment 5•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=efc8e129c36f
Assignee | ||
Comment 6•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f431e9e76c02
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8601026 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
Comment 8•9 years ago
|
||
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+
Assignee | ||
Comment 9•9 years ago
|
||
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).
Assignee | ||
Comment 10•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fe2d349950e
Assignee | ||
Comment 11•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6af9d10a242
Assignee | ||
Comment 12•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5232dd059c11
Comment 13•9 years ago
|
||
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?
Updated•9 years ago
|
Flags: needinfo?(jorendorff)
https://hg.mozilla.org/mozilla-central/rev/5232dd059c11
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment 15•9 years ago
|
||
Landed followup to address comment 13: https://hg.mozilla.org/integration/mozilla-inbound/rev/ed98e1b9168d
Updated•9 years ago
|
Flags: needinfo?(jorendorff)
You need to log in
before you can comment on or make changes to this bug.
Description
•