Closed
Bug 1113369
Opened 10 years ago
Closed 10 years ago
Change [[Set]] and other internal methods to support errors
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: jorendorff, Assigned: jorendorff)
References
(Blocks 4 open bugs)
Details
Attachments
(9 files, 7 obsolete files)
(deleted),
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
dvander
:
review+
bholley
:
review+
|
Details | Diff | Splinter Review |
ES6 specifies that 5 internal methods ([[SetPrototypeOf]], [[PreventExtensions]], [[DefineOwnProperty]], [[Set]], [[Delete]]) return boolean values.
This is done to avoid piping an explicit 'strict' parameter throw everything in the spec. Strict callers and nonstrict callers call these methods in just the same way, but strict callers check the return value and throw a TypeError if it's false.
Instead we pass a strict parameter all over the place. In our defense this does let us generate better error messages. But we have to change it in order to support Reflect.set() and friends, which expose the boolean return values.
To do this while keeping the good error messages will require changing the boolean return value to something like a 'Maybe<ErrorMessage>'.
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Comment 5•10 years ago
|
||
Assignee | ||
Comment 6•10 years ago
|
||
Assignee | ||
Comment 7•10 years ago
|
||
Assignee | ||
Comment 8•10 years ago
|
||
Assignee | ||
Comment 9•10 years ago
|
||
In general, jsobj.h will offer, for each standard internal method that returns a boolean value indicating success/failure, signatures with a JS::ObjectOpResult& out-parameter and signatures without it. The ones without will throw a TypeError on failure (that is, the behavior will be "strict").
Attachment #8564134 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8564135 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 11•10 years ago
|
||
Add an ObjectOpResult out-param for DefineProperty functions everywhere. We leave a few js::DefineProperty() convenience functions with no *result out-param. These have strict behavior: that is, they automatically check the result and throw if it is false. In bug 1125624 these strict signatures may end up being called DefinePropertyOrThrow, as that is what the spec calls it.
Attachment #8564138 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8564139 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8564140 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8564141 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8564142 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 16•10 years ago
|
||
By the way, if your impression after reading a bunch of this is that the many hunks like this are not helping:
> MOZ_ASSERT(!setter);
>- return CallAddPropertyHookDense(cx, obj, index, value);
>+ if (!CallAddPropertyHookDense(cx, obj, index, value))
>+ return false;
>+ return result.succeed();
> }
> }
...then I could be convinced to make OkCode the default and get rid of Uninitialized. In every case I found where I was using an uninitialized ObjectOpResult (and there were maybe 8-10 bugs like this during development) the cause was a missing result.succeed(), not a missing result.fail().
Assignee | ||
Comment 17•10 years ago
|
||
Attachment #8564249 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•10 years ago
|
Attachment #8564139 -
Attachment is obsolete: true
Attachment #8564139 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 18•10 years ago
|
||
Comment 19•10 years ago
|
||
Comment on attachment 8564134 [details] [diff] [review]
part 1 - Introduce JS::ObjectOpResult and use it in js::StandardDefineProperty
Review of attachment 8564134 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/public/Class.h
@@ +66,5 @@
> + *
> + * Typical usage:
> + *
> + * ObjctOpSuccess success;
> + * if (!DefineProperty(cx, obj, id, ..., &success))
The & shouldn't be there any more, right?
@@ +93,5 @@
> + return code_ == OkCode;
> + }
> +
> + explicit operator bool() const { return ok(); }
> + bool operator!() const { return !ok(); }
I don't believe you need this -- default operator! will invoke an explicit operator bool.
::: js/src/jsapi.cpp
@@ +145,5 @@
> + MOZ_ASSERT(!ok());
> + MOZ_ASSERT(code_ != Uninitialized);
> +
> + if (ErrorTakesIdArgument(code_)) {
> + RootedString str(cx, IdToString(cx, id));
This produces sane results if id is a symbol, correct? Don't want to be losing all sanity in errors if a symbol is a property name.
::: js/src/jsobj.cpp
@@ +781,5 @@
>
> + // XXX Temporarily break compatibility here. The right thing is to pass
> + // result on to ArraySetLength.
> + if (!ArraySetLength(cx, arr, id, attrs, v, false))
> + return false;
Gonna assume a test fails with this, a later patch fixes this, and that the two patches will land together.
Attachment #8564134 -
Flags: review?(jwalden+bmo) → review+
Comment 20•10 years ago
|
||
Comment on attachment 8564135 [details] [diff] [review]
part 2 - Use JS::ObjectOpResult in js::SetArrayLength
Review of attachment 8564135 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/public/Class.h
@@ +121,5 @@
>
> /*
> + * Report an error or warning if necessary; return true to proceed and
> + * false if an error was reported. Call this when failure should cause
> + * an warning if extraWarnings are enabled.
an warning
::: js/src/jsobj.cpp
@@ +778,5 @@
> if (desc.hasWritable() && !desc.writable())
> attrs = attrs | JSPROP_READONLY;
> }
>
> + return ArraySetLength(cx, arr, id, attrs, v, result);
That was quick. :-)
Attachment #8564135 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 21•10 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #19)
> > + if (ErrorTakesIdArgument(code_)) {
> > + RootedString str(cx, IdToString(cx, id));
>
> This produces sane results if id is a symbol, correct? Don't want to be
> losing all sanity in errors if a symbol is a property name.
Oh yuck, it doesn't. Good catch. I'll fix it before landing and add a test.
Also, I think I regressed JSMSG_OBJECT_NOT_EXTENSIBLE:
js> Object.defineProperty(Object.freeze({}), "prop", {})
typein:1:0 TypeError: prop is not extensible
D'oh. I'll fix that somehow.
All these error messages could be improved a great deal with a fairly small amount of effort: just including a brief side-effect-free description of the object, for example. Filed bug 1134253 against that.
> > + // XXX Temporarily break compatibility here. The right thing is to pass
> > + // result on to ArraySetLength.
>
> Gonna assume a test fails with this, a later patch fixes this, and that the
> two patches will land together.
Yep! The test is js/src/ecma_5/extensions/proxy-array-target-length-definition.js .
Comment 22•10 years ago
|
||
Comment on attachment 8564138 [details] [diff] [review]
part 3 - [[DefineOwnProperty]]
Review of attachment 8564138 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/bindings/DOMJSProxyHandler.cpp
@@ +192,5 @@
>
> bool
> DOMProxyHandler::defineProperty(JSContext* cx, JS::Handle<JSObject*> proxy, JS::Handle<jsid> id,
> + MutableHandle<JSPropertyDescriptor> desc,
> + JS::ObjectOpResult &result, bool *defined) const
Don't you need to |return result.fail()| or something in the return-true case in this method, when an Xray was passed in?
::: js/src/builtin/TypedObject.cpp
@@ +1771,2 @@
> {
> return ReportPropertyError(cx, JSMSG_UNDEFINED_PROP, id);
Talk to the typed-object people, make sure they want this to unconditionally (?) throw rather than just silently do nothing in strict mode. I would mildly expect the latter, but who knows.
::: js/src/jsarray.cpp
@@ +749,2 @@
> {
> + if (!obj->is<ArrayObject>())
Probably worth adding an assertion that obj isn't a proxy. I want to say the callers are in the native property-definition path such that this is the case, so this can be done, tho I'm not 100% certain.
::: js/src/proxy/ScriptedDirectProxyHandler.cpp
@@ +560,2 @@
> {
> + // steps 2-4
If all the step numbers are being touched, it'd be nice to make these // Step 1. and so on, fixing caps/punctuation to be consistent with step listings elsewhere.
@@ +572,3 @@
> RootedValue trap(cx);
> if (!GetProperty(cx, handler, handler, cx->names().defineProperty, &trap))
> return false;
The spec draft I'm looking at (20150220) says this should be GetMethod, which is GetProperty, then returning undefined if null/undefined was gotten, otherwise returning that after an is-callable check (which if it fails causes a TypeError to be thrown). Please post a followup patch to add a GetMethod method and use that here -- and if search results are predictive, probably this needs to happen for *every* trap-get.
I believe this is observable: something like Object.defineProperty(new Proxy({}, { defineProperty: null }), "foo", {}) shouldn't throw a TypeError.
@@ +610,5 @@
> + if (!IsExtensible(cx, target, &extensibleTarget))
> + return false;
> +
> + // step 17-18
> + bool settingConfigFalse = desc.isPermanent();
This isn't really right, is it? It looks like this bit is set if the field's present -- cool -- or absent -- not cool. At least comment this for fixing later?
::: js/src/vm/NativeObject.cpp
@@ +1158,5 @@
> + if (WouldDefinePastNonwritableLength(obj, index))
> + return result.fail(JSMSG_CANT_DEFINE_PAST_ARRAY_LENGTH);
> +
> + NativeObject::EnsureDenseResult edResult;
> + edResult = obj->ensureDenseElements(cx, index, 1);
Mild preference for
N::E edResult =
obj->ensure...;
@@ +1230,5 @@
> if (!cx->shouldBeJSContext())
> return false;
> RootedValue nvalue(cx, value);
> +
> + // FIXME: result should be passed to NativeSet.
Fixt later in these patches? Get the bug filed if not.
@@ +2061,5 @@
> return true;
> }
>
> + if (WouldDefinePastNonwritableLength(obj, index)) {
> + if (strict) {
Does some followup here propagate results into this, so this isn't handling this itself?
Attachment #8564138 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 23•10 years ago
|
||
Attachment #8568220 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 24•10 years ago
|
||
Attachment #8568221 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•10 years ago
|
Attachment #8564135 -
Attachment is obsolete: true
Assignee | ||
Comment 25•10 years ago
|
||
Comment on attachment 8568220 [details] [diff] [review]
part 1.5 - Avoid regressing error messages by adding obj to the ObjectOpResult methods that could throw a TypeError
s/1½/1.5/ for the benefit of hg bzexport (d'oh)
Attachment #8568220 -
Attachment description: part 1½ - Avoid regressing error messages by adding obj to the ObjectOpResult methods that could throw a TypeError → part 1.5 - Avoid regressing error messages by adding obj to the ObjectOpResult methods that could throw a TypeError
Assignee | ||
Comment 26•10 years ago
|
||
Add an ObjectOpResult out-param for DefineProperty functions everywhere. We leave a few js::DefineProperty() convenience functions with no *result out-param. These have strict behavior: that is, they automatically check the result and throw if it is false. In bug 1125624 these strict signatures may end up being called DefinePropertyOrThrow, as that is what the spec calls it.
Assignee | ||
Comment 27•10 years ago
|
||
Attachment #8568229 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•10 years ago
|
Attachment #8564138 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8564249 -
Attachment is obsolete: true
Attachment #8564249 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•10 years ago
|
Attachment #8568226 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 28•10 years ago
|
||
Attachment #8568231 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•10 years ago
|
Attachment #8564140 -
Attachment is obsolete: true
Attachment #8564140 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 29•10 years ago
|
||
Attachment #8568233 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•10 years ago
|
Attachment #8564141 -
Attachment is obsolete: true
Attachment #8564141 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 30•10 years ago
|
||
Attachment #8568234 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•10 years ago
|
Attachment #8564142 -
Attachment is obsolete: true
Attachment #8564142 -
Flags: review?(jwalden+bmo)
Comment 31•10 years ago
|
||
Comment on attachment 8568220 [details] [diff] [review]
part 1.5 - Avoid regressing error messages by adding obj to the ObjectOpResult methods that could throw a TypeError
Review of attachment 8568220 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/public/Class.h
@@ +122,5 @@
> * Convenience method. Return true if ok() or if strict is false; otherwise
> * throw a TypeError and return false.
> */
> + bool checkStrict(JSContext *cx, HandleObject obj, HandleId id) {
> + return ok() || reportError(cx, obj, id);
I'd mildly prefer
if (ok())
return true;
return reportError(...); here.
::: js/src/jsapi.cpp
@@ +146,5 @@
> +
> + if (code_ == JSMSG_OBJECT_NOT_EXTENSIBLE) {
> + RootedValue val(cx, ObjectValue(*obj));
> + js_ReportValueErrorFlags(cx, JSREPORT_ERROR, code_, JSDVG_IGNORE_STACK, val,
> + NullPtr(), nullptr, nullptr);
Ugh, this is turning into a bigger and bigger mess. :-(
::: js/src/tests/js1_5/extensions/regress-365869.js
@@ +32,5 @@
> print('test crash from bug 371292 Comment 9');
>
> try
> {
> + expect = "TypeError: can't redefine non-configurable property 5";
Hmm, the old error message seemed better to me. Not worth a strong complaint, tho.
Attachment #8568220 -
Flags: review?(jwalden+bmo) → review+
Comment 32•10 years ago
|
||
Comment on attachment 8568221 [details] [diff] [review]
part 2 - Use JS::ObjectOpResult in js::SetArrayLength
Review of attachment 8568221 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/public/Class.h
@@ +120,5 @@
>
> /*
> + * Report an error or warning if necessary; return true to proceed and
> + * false if an error was reported. Call this when failure should cause
> + * an warning if extraWarnings are enabled.
a warning
Attachment #8568221 -
Flags: review?(jwalden+bmo) → review+
Comment 33•10 years ago
|
||
Comment on attachment 8568231 [details] [diff] [review]
part 5 - [[Delete]]
Review of attachment 8568231 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/base/nsGlobalWindow.cpp
@@ +827,4 @@
> {
> if (nsCOMPtr<nsIDOMWindow> frame = GetSubframeWindow(cx, proxy, id)) {
> + // Fail (which means throw if strict, else return false).
> + return result.failCantDelete();
I'd prefer seeing this comment removed. We should get in the habit of only referring to things in result-ful terms, forcing people to understand them in our terms, and the spec's terms. That strict mode and non-strict behave certain ways is characteristic of *those* locations -- but not of the underlying operation that's used within both.
::: dom/bindings/Codegen.py
@@ +10294,5 @@
> def getDeleterBody(type, foundVar=None):
> """
> type should be "Named" or "Indexed"
> +
> + The possibile outcomes:
possible
::: js/src/builtin/TypedObject.cpp
@@ +2073,5 @@
> return ReportPropertyError(cx, JSMSG_CANT_DELETE, id);
>
> RootedObject proto(cx, obj->getProto());
> + if (!proto)
> + return result.succeed();
Add a test for this behavior change. Or possibly don't make it now, and fix it in a followup patch.
::: js/src/jit/BaselineCompiler.cpp
@@ +2044,5 @@
> typedef bool (*DeleteElementFn)(JSContext *, HandleValue, HandleValue, bool *);
> +static const VMFunction DeleteElementStrictInfo
> + = FunctionInfo<DeleteElementFn>(DeleteElementJit<true>);
> +static const VMFunction DeleteElementNonStrictInfo
> + = FunctionInfo<DeleteElementFn>(DeleteElementJit<false>);
I can't remember ever seeing the = on the next line in our code - bump to previous.
::: js/src/json.cpp
@@ +717,5 @@
> return false;
> } else {
> /* Step 2a(iii)(3). */
> // XXX This definition should ignore success/failure, when
> // our property-definition APIs indicate that.
The previous patch likely made this doable now, right? Should fix in a separate patch with a test.
@@ +745,5 @@
> return false;
> } else {
> /* Step 2b(ii)(3). */
> // XXX This definition should ignore success/failure, when
> // our property-definition APIs indicate that.
And here.
::: js/src/proxy/Proxy.cpp
@@ +602,2 @@
> return false;
> + return SuppressDeletedProperty(cx, obj, id); // XXX is this necessary?
...yeah, I'm not really sure either. Can't remember if "native iterator" also implies object nativeness or not, and skiming jsiter.cpp didn't quickly answer either.
::: js/src/proxy/ScriptedDirectProxyHandler.cpp
@@ +708,3 @@
> RootedValue trap(cx);
> if (!GetProperty(cx, handler, handler, cx->names().deleteProperty, &trap))
> return false;
GetMethod again.
::: js/src/vm/ArgumentsObject.cpp
@@ +357,1 @@
> NativeDefineProperty(cx, argsobj, id, vp, nullptr, nullptr, attrs, result);
Worth splitting this to add a MOZ_ASSERT(ignored.ok()) as well? I would.
@@ +468,5 @@
> * simple data property. Note that we rely on args_delProperty to clear the
> * corresponding reserved slot so the GC can collect its value.
> */
> + ObjectOpResult ignored;
> + return NativeDeleteProperty(cx, argsobj, id, ignored) &&
And here.
::: js/src/vm/Interpreter.cpp
@@ +2363,4 @@
> RootedId &id = rootId0;
> if (!ValueToId<CanGC>(cx, propval, &id))
> goto error;
> + if (!DeleteProperty(cx, obj, id, result))
Mild preference for result by the DeleteProperty call, because the result is the direct output of the method. The id is just an input, ergo less important, ergo should go further away.
Attachment #8568231 -
Flags: review?(jwalden+bmo) → review+
Comment 34•10 years ago
|
||
Comment on attachment 8568226 [details] [diff] [review]
part 3 - [[DefineOwnProperty]]
Review of attachment 8568226 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/base/nsGlobalWindow.cpp
@@ +789,5 @@
> // Spec says to Reject whether this is a supported index or not,
> // since we have no indexed setter or indexed creator. That means
> // throwing in strict mode (FIXME: Bug 828137), doing nothing in
> // non-strict mode.
> + return result.succeed();
Shouldn't this be a failure case, per the comment? Fix in a followup patch with a test.
::: dom/bindings/Codegen.py
@@ +10269,5 @@
>
> if (found) {
> + return js::IsInNonStrictPropertySet(cx)
> + ? opresult.succeed()
> + : ThrowErrorMessage(cx, MSG_NO_NAMED_SETTER, "${name}");
That presumable bytecode-sniffing is ugh. Surely we can get rid of it somehow in a followup? I hope?
::: dom/bindings/DOMJSProxyHandler.cpp
@@ +206,1 @@
> return true;
Don't you need to |return result.fail()| or something in the return-true case in this method, when an Xray was passed in?
::: js/src/builtin/TypedObject.cpp
@@ +1771,3 @@
> {
> Rooted<TypedObject *> typedObj(cx, &obj->as<TypedObject>());
> return ReportTypedObjTypeError(cx, JSMSG_OBJECT_NOT_EXTENSIBLE, typedObj);
Again not clear to me this should be unconditional error versus result.failCantDefine() or so.
::: js/src/jsarray.cpp
@@ +749,2 @@
> {
> + if (!obj->is<ArrayObject>())
Again, assert it's not a proxy?
::: js/src/proxy/ScriptedDirectProxyHandler.cpp
@@ +560,2 @@
> {
> + // steps 2-4
If all the step numbers are being touched, it'd be nice to make these // Step 1. and so on, fixing caps/punctuation to be consistent with step listings elsewhere.
@@ +572,3 @@
> RootedValue trap(cx);
> if (!GetProperty(cx, handler, handler, cx->names().defineProperty, &trap))
> return false;
The spec draft I'm looking at (20150220) says this should be GetMethod, which is GetProperty, then returning undefined if null/undefined was gotten, otherwise returning that after an is-callable check (which if it fails causes a TypeError to be thrown). Please post a followup patch to add a GetMethod method and use that here -- and if search results are predictive, probably this needs to happen for *every* trap-get.
I believe this is observable: something like Object.defineProperty(new Proxy({}, { defineProperty: null }), "foo", {}) shouldn't throw a TypeError.
@@ +610,5 @@
> + if (!IsExtensible(cx, target, &extensibleTarget))
> + return false;
> +
> + // step 17-18
> + bool settingConfigFalse = desc.isPermanent();
This isn't really right, is it? It looks like this bit is set if the field's present -- cool -- or absent -- not cool. At least comment this for fixing later?
::: js/src/vm/NativeObject.cpp
@@ +1155,5 @@
> + if (WouldDefinePastNonwritableLength(obj, index))
> + return result.fail(JSMSG_CANT_DEFINE_PAST_ARRAY_LENGTH);
> +
> + NativeObject::EnsureDenseResult edResult;
> + edResult = obj->ensureDenseElements(cx, index, 1);
Mild preference for
N::E edResult =
obj->ensure...;
@@ +1229,5 @@
> RootedValue nvalue(cx, value);
> +
> + // FIXME: result should be passed to NativeSet.
> + if (!NativeSet(cx->asJSContext(), obj, obj, shape, false, &nvalue))
> + return false;
Fixt later in these patches? Get the bug filed if not.
@@ +2139,5 @@
> return true;
> }
>
> + if (WouldDefinePastNonwritableLength(obj, index)) {
> + if (strict) {
Does some followup here propagate results into this, so this isn't handling this itself?
::: js/src/vm/ScopeObject.cpp
@@ +1634,5 @@
> bool found;
> if (!has(cx, proxy, id, &found))
> return false;
> if (found)
> + return result.fail(JSMSG_CANT_REDEFINE_PROP);
Add a test for this behavioral change? Or defer it to a followup and leave it as throwing. Whatever's faster progress.
::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +1958,5 @@
> if (existing_desc.hasGetterOrSetterObject() || desc.hasGetterOrSetterObject() ||
> existing_desc.isEnumerable() != desc.isEnumerable() ||
> (existing_desc.isReadonly() && !desc.isReadonly()))
> {
> + return result.failCantRedefineProp();
Add a test for the behavioral change? Or defer to followup.
@@ +1963,3 @@
> }
> + if (existing_desc.isReadonly())
> + return result.failReadOnly();
Same.
Attachment #8568226 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 35•10 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #22)
> ::: dom/bindings/DOMJSProxyHandler.cpp
> Don't you need to |return result.fail()| or something in the return-true
> case in this method, when an Xray was passed in?
Yes. Done. I also changed the JS_ReportErrorFlagsAndNumber call to result.fail(), since the flags were `JSREPORT_WARNING | JSREPORT_STRICT | JSREPORT_STRICT_MODE_ERROR` (I missed it before).
> Talk to the typed-object people, make sure they want this to unconditionally
> (?) throw rather than just silently do nothing in strict mode. I would
> mildly expect the latter, but who knows.
I'll ask. For now I just preserved the existing behavior: be strict regardless of the strict flag, even for set(). I wanted to minimize behavior changes for this stack; but I will gladly take a follow-up bug for this if the TypedObject folks tell me they want this change...
> ::: js/src/jsarray.cpp
> @@ +749,2 @@
> > {
> > + if (!obj->is<ArrayObject>())
>
> Probably worth adding an assertion that obj isn't a proxy.
It turns out all the call sites pass either an ArrayObject or a NativeObject, so I just changed the parameter type to HandleNativeObject. Yay types.
> ::: js/src/proxy/ScriptedDirectProxyHandler.cpp
> If all the step numbers are being touched, it'd be nice to make these //
> Step 1. and so on, fixing caps/punctuation to be consistent with step
> listings elsewhere.
OK, but the whole file should be changed at one go; I'll file a follow-up bug and patch.
> @@ +572,3 @@
> > RootedValue trap(cx);
> > if (!GetProperty(cx, handler, handler, cx->names().defineProperty, &trap))
> > return false;
>
> The spec draft I'm looking at (20150220) says this should be GetMethod,
> [...]I believe this is observable: something like Object.defineProperty(new
> Proxy({}, { defineProperty: null }), "foo", {}) shouldn't throw a TypeError.
Ugh. Yeah. This is one of those cases where the spec is wrong... in the moral sense. But yeah, I'll file a follow-up for that too..
> @@ +610,5 @@
> > + // step 17-18
> > + bool settingConfigFalse = desc.isPermanent();
>
> This isn't really right, is it? It looks like this bit is set if the
> field's present -- cool -- or absent -- not cool. At least comment this for
> fixing later?
Right, it's a bug. The good news is, I've already written a fix. Bug 1133081, part 5, has this:
>- bool settingConfigFalse = desc.isPermanent();
>+ bool settingConfigFalse = desc.hasConfigurable() && !desc.configurable();
For the moment, I'll just put in a comment. Ultimately we should add a dimension to the Object.defineProperty tests: try each combination on a null-handler direct proxy. I think bug 1133081 might even get us to where that would pass. Right now it would be failure city.
> Mild preference for
>
> N::E edResult =
> obj->ensure...;
It even fits on one line.
> > + // FIXME: result should be passed to NativeSet.
>
> Fixt later in these patches? Get the bug filed if not.
You know it. (Part 4.)
> > + if (WouldDefinePastNonwritableLength(obj, index)) {
> > + if (strict) {
>
> Does some followup here propagate results into this, so this isn't handling
> this itself?
Ditto.
There's more for me to do here. Tomorrow!
Assignee | ||
Comment 36•10 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #31)
> > + expect = "TypeError: can't redefine non-configurable property 5";
>
> Hmm, the old error message seemed better to me. Not worth a strong
> complaint, tho.
Yeah. Bug 1134253 asks for better error messages all around... If it's any consolation, I'm thinking Object.defineProperty on elements is not really a thing out there.
> > + * an warning if extraWarnings are enabled.
>
> a warning
Fixed.
Comment 37•10 years ago
|
||
Comment on attachment 8568229 [details] [diff] [review]
part 4 - [[Set]]
Review of attachment 8568229 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jit/IonCaches.cpp
@@ +1472,5 @@
> // Push stubCode for marking.
> attacher.pushStubCodePointer(masm);
>
> + // Unused space, to keep the same stack layout as Proxy::set frames.
> + static_assert(sizeof(ObjectOpResult) == 4,
sizeof(int32_t) might be slightly nicer.
@@ +2172,5 @@
> // Push stubCode for marking.
> attacher.pushStubCodePointer(masm);
>
> + // Allocate result out-param.
> + static_assert(sizeof(ObjectOpResult) == 4,
sizeof(int32_t)
@@ +2174,5 @@
>
> + // Allocate result out-param.
> + static_assert(sizeof(ObjectOpResult) == 4,
> + "ObjectOpResult size must match size reserved by masm.Push() here");
> + masm.Push(Imm32(ObjectOpResult::Uninitialized));
Might be nice in non-debug to not push a value, just bump the pointer. Micro-optimization.
@@ +2211,4 @@
> masm.branchIfFalseBool(ReturnReg, masm.exceptionLabel());
>
> + // Test for strict failure. We emit the check even in non-strict mode in
> + // order to pick up the warning if extraWarnings is enabled.
Ugh. BURN IT ALL TO THE GROUND. :-(
Maybe we can make extra warnings a property of the script and eliminate this at some point.
@@ +2437,5 @@
> + // the stubCode pointer in order to match the layout of
> + // IonOOLSetterOpExitFrameLayout.
> + static_assert(sizeof(ObjectOpResult) == 4,
> + "ObjectOpResult size must match size reserved by masm.Push() here");
> + masm.Push(Imm32(ObjectOpResult::Uninitialized));
So actually. Could we add a pushObjectOpResult method or something, so we don't have this assert combo repeated every time? This has to be combinable somehow.
Or, actually, maybe reserveObjectOpResult() or something like that, to encapsulate the uninitialized value being pushed here, that shouldn't be necessary except in debug builds.
::: js/src/jit/JitFrames.h
@@ +720,5 @@
> uint32_t vp0_;
> uint32_t vp1_;
>
> + // result out-parameter (unused for Proxy::get)
> + JS::ObjectOpResult result_;
Hm, at some point we should split these in two if the result's unused for some things and not others.
::: js/src/proxy/ScriptedDirectProxyHandler.cpp
@@ +949,5 @@
>
> // step 5-6
> RootedValue trap(cx);
> if (!GetProperty(cx, handler, handler, cx->names().set, &trap))
> return false;
Also GetMethod again.
@@ +951,5 @@
> RootedValue trap(cx);
> if (!GetProperty(cx, handler, handler, cx->names().set, &trap))
> return false;
>
> // step 7
Step numbering is off throughout this method again. Feel free to followup-fix.
@@ +1005,1 @@
> vp.setBoolean(success);
By wrong you mean unnecessary, right? Not clear to me how it's wrong, exactly -- there's no value being returned, so this is just setting a value into the ether, that the "other side" is looking at even when it shouldn't, right?
::: js/src/vm/GlobalObject.h
@@ +613,5 @@
> #endif
> RootedObject holder(cx, intrinsicsHolder());
> RootedValue valCopy(cx, value);
> + ObjectOpResult ignored;
> + return SetProperty(cx, holder, holder, name, &valCopy, ignored);
We can assert ok() on success, right?
::: js/src/vm/ScopeObject.cpp
@@ +1608,5 @@
> Rooted<DebugScopeObject*> debugScope(cx, &proxy->as<DebugScopeObject>());
> Rooted<ScopeObject*> scope(cx, &proxy->as<DebugScopeObject>().scope());
>
> if (debugScope->isOptimizedOut())
> + return result.fail(JSMSG_DEBUG_CANT_SET_OPT_ENV);
Test? Or defer the behavior change.
::: js/xpconnect/wrappers/AddonWrapper.cpp
@@ +140,1 @@
> return JS_CallFunctionValue(cx, receiver, fval, args, vp);
I might have
if (!JS_CallFunctionValue(...))
return false;
result.succeed();
return true;
but it's a nit.
Attachment #8568229 -
Flags: review?(jwalden+bmo) → review+
Comment 38•10 years ago
|
||
I'm cool with having the uninitialized initial state -- EIBTI and all. A little more trouble, but I think perfectly reasonable trouble to have to go to.
Assignee | ||
Comment 39•10 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #34)
My comment 35 was in response to your first review of this patch (comment 22); here I'll just reply to the new comments in your second review (comment 34).
> ::: dom/base/nsGlobalWindow.cpp
> > // Spec says to Reject whether this is a supported index or not,
> > // since we have no indexed setter or indexed creator. That means
> > // throwing in strict mode (FIXME: Bug 828137), doing nothing in
> > // non-strict mode.
> > + return result.succeed();
>
> Shouldn't this be a failure case, per the comment? Fix in a followup patch
> with a test.
Already done and reviewed in the cited bug.
> ::: dom/bindings/Codegen.py
> > + return js::IsInNonStrictPropertySet(cx) [...]
>
> That presumable bytecode-sniffing is ugh. Surely we can get rid of it
> somehow in a followup? I hope?
Yeah. Filed bug 1135993.
> ::: dom/bindings/DOMJSProxyHandler.cpp
> @@ +206,1 @@
> > return true;
>
> Don't you need to |return result.fail()| or something in the return-true
> case in this method, when an Xray was passed in?
Yes. But it should be result.succeed(), because we don't want the caller to treat it as failure. Fixed.
> ::: js/src/vm/ScopeObject.cpp
> > if (found)
> > + return result.fail(JSMSG_CANT_REDEFINE_PROP);
>
> Add a test for this behavioral change? Or defer it to a followup and leave
> it as throwing. Whatever's faster progress.
Huh. Well, I just changed it back. I don't think the change is observable. IIUC, DebugScopeProxy objects are never directly exposed to script, and its sole user (Debugger.Environment) never calls DefineProperty on it.
> ::: js/xpconnect/wrappers/XrayWrapper.cpp
> > + return result.failCantRedefineProp();
>
> Add a test for the behavioral change? Or defer to followup.
>
> > + if (existing_desc.isReadonly())
> > + return result.failReadOnly();
>
> Same.
These are testable. Deferred to bug 1135997.
Assignee | ||
Comment 40•10 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #37)
> ::: js/src/jit/IonCaches.cpp
> > + // Unused space, to keep the same stack layout as Proxy::set frames.
> > + static_assert(sizeof(ObjectOpResult) == 4,
>
> sizeof(int32_t) might be slightly nicer.
done
> @@ +2211,4 @@
> > masm.branchIfFalseBool(ReturnReg, masm.exceptionLabel());
> >
> > + // Test for strict failure. We emit the check even in non-strict mode in
> > + // order to pick up the warning if extraWarnings is enabled.
>
> Ugh. BURN IT ALL TO THE GROUND. :-(
Yeah. I think we can afford the extra branch. If it regresses proxy accesses, I'll take it back out and we'll just fail to generate warnings sometimes.
> Maybe we can make extra warnings a property of the script and eliminate this
> at some point.
That's a good idea.
> So actually. Could we add a pushObjectOpResult method or something, so we
> don't have this assert combo repeated every time? This has to be combinable
> somehow.
done
> ::: js/src/proxy/ScriptedDirectProxyHandler.cpp
> Step numbering is off throughout this method again. Feel free to
> followup-fix.
see next comment
> > // XXX FIXME - This use of vp is wrong. Bug 1132522 can clean it up.
> > vp.setBoolean(success);
>
> By wrong you mean unnecessary, right? Not clear to me how it's wrong,
> exactly -- there's no value being returned, so this is just setting a value
> into the ether, that the "other side" is looking at even when it shouldn't,
> right?
Right. We just have different definitions of "wrong". To me, deluded is wrong.
In any case the comment and the bogus vp.setBoolean() are both deleted in patches already r+'d in bug 1132522 (they fix the step numbers too).
> ::: js/src/vm/GlobalObject.h
> @@ +613,5 @@
> > #endif
> > RootedObject holder(cx, intrinsicsHolder());
> > RootedValue valCopy(cx, value);
> > + ObjectOpResult ignored;
> > + return SetProperty(cx, holder, holder, name, &valCopy, ignored);
>
> We can assert ok() on success, right?
Sure.
> > if (debugScope->isOptimizedOut())
> > + return result.fail(JSMSG_DEBUG_CANT_SET_OPT_ENV);
>
> Test? Or defer the behavior change.
Backed out for the same reason as the other DebugScopeObject change.
> > return JS_CallFunctionValue(cx, receiver, fval, args, vp);
>
> I might have
>
> if (!JS_CallFunctionValue(...))
> return false;
> result.succeed();
> return true;
Agreed.
Comment 41•10 years ago
|
||
(In reply to Jason Orendorff [:jorendorff] from comment #40)
> Right. We just have different definitions of "wrong". To me, deluded is
> wrong.
Hm, true, true. Well -- they're both definitions, I just seized upon the slightly wrong one when reading. I might have used "nonsensical" or "abominable" or some other word or phrase more clearly distinguishing "incorrect" from "this is stupid why would we ever do this ahghgngh". :-)
Assignee | ||
Comment 42•10 years ago
|
||
Assignee | ||
Comment 43•10 years ago
|
||
Assignee | ||
Comment 44•10 years ago
|
||
Comment 45•10 years ago
|
||
Comment on attachment 8568233 [details] [diff] [review]
part 6 - [[PreventExtensions]]
Review of attachment 8568233 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jsapi.cpp
@@ +172,5 @@
> +JS::ObjectOpResult::reportStrictErrorOrWarning(JSContext *cx, HandleObject obj, bool strict)
> +{
> + MOZ_ASSERT(!ErrorTakesIdArgument(code_));
> + RootedId id(cx, JSID_EMPTY);
> + return reportStrictErrorOrWarning(cx, obj, id, strict);
Frankly, I think I'd rather see two reporting methods, with different code in each, than pass in a bogus id and have to worry whether the downstream code ever actually uses it in a bad way.
Attachment #8568233 -
Flags: review?(jwalden+bmo) → review+
Updated•10 years ago
|
Attachment #8568234 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 46•10 years ago
|
||
Assignee | ||
Comment 47•10 years ago
|
||
Assignee | ||
Comment 48•10 years ago
|
||
Assignee | ||
Comment 49•10 years ago
|
||
Assignee | ||
Comment 50•10 years ago
|
||
Assignee | ||
Comment 51•10 years ago
|
||
Assignee | ||
Comment 52•10 years ago
|
||
Found the bug finally. Part 3 contains this gem:
>- bool defined = false;
>+ bool defined;
It's then being used as an outparam, but the callee expects it to be pre-initialized to false. Filed bug 1137899 to make this less time consuming to track down next time.
Assignee | ||
Comment 53•10 years ago
|
||
Attachment #8570698 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 54•10 years ago
|
||
Comment on attachment 8570698 [details] [diff] [review]
All-in-one patch
Review of attachment 8570698 [details] [diff] [review]:
-----------------------------------------------------------------
Hey dvander, could you take a look at the js/ipc parts here?
Attachment #8570698 -
Flags: review?(dvander)
Assignee | ||
Comment 55•10 years ago
|
||
dvander: Comment 0 and the comment I'm adding in js/public/Class.h are probably the best guide to what's going on here.
Assignee | ||
Comment 56•10 years ago
|
||
Comment on attachment 8570698 [details] [diff] [review]
All-in-one patch
Review of attachment 8570698 [details] [diff] [review]:
-----------------------------------------------------------------
bholley: See previous comment. This patch adds a new outparam to 5 ProxyHandler methods, which necessarily touches every silly thing in js/xpconnect, but in a pretty shallow way.
Attachment #8570698 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 57•10 years ago
|
||
Comment 58•10 years ago
|
||
Comment on attachment 8564134 [details] [diff] [review]
part 1 - Introduce JS::ObjectOpResult and use it in js::StandardDefineProperty
Review of attachment 8564134 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/public/Class.h
@@ +65,5 @@
> + * it also stores an error code.
> + *
> + * Typical usage:
> + *
> + * ObjctOpSuccess success;
"Objct"
Assignee | ||
Comment 59•10 years ago
|
||
Assignee | ||
Comment 60•10 years ago
|
||
(In reply to :Ms2ger from comment #58)
> "Objct"
Fixed already -- you can see what it looks like now in the all-in-one patch.
Try server is finally green. I had to apply one more patch, now awaiting review in bug 1138059.
Comment on attachment 8570698 [details] [diff] [review]
All-in-one patch
Review of attachment 8570698 [details] [diff] [review]:
-----------------------------------------------------------------
js/ipc changes look good
Attachment #8570698 -
Flags: review?(dvander) → review+
Comment 62•10 years ago
|
||
Comment on attachment 8570698 [details] [diff] [review]
All-in-one patch
> WindowNamedPropertiesHandler::delete_(JSContext* aCx,
The message for JSMSG_CANT_DELETE (from failCantDelete) is a bit of an odd fit here, since it claims the prop is non-configurable, but the properties on WindowNamedPropertiesHandler are in fact very much configurable.... Not sure whether we want a different error message here or what.
This applies to the failCantRedefineProp bug 828137 adds to nsOuterWindowProxy::defineProperty too: that claims the prop is non-configurable, which is not the case.
> nsOuterWindowProxy::delete_(JSContext *cx, JS::Handle<JSObject*> proxy,
Same issue here.
>+++ b/dom/bindings/BindingUtils.h
>+ * result is the out-parameter indicating success (read it only if
>+ * this returns true and also sets *result to true).
You mean sets *defined to true?
>+++ b/dom/bindings/Codegen.py
>@@ -10200,140 +10197,157 @@ class CGDOMJSProxyHandler_defineProperty
>+ return js::IsInNonStrictPropertySet(cx)
>+ ? opresult.succeed()
>+ : ThrowErrorMessage(cx, MSG_NO_INDEXED_SETTER, "${name}");
Shouldn't this turn into a "return opresult.fail(something)" instead of the manual js::IsInNonStrictPropertySet check? Followup bug is ok here.
My apologies for the merge issues around the unforgeable bits in this function. :( The good news is that the merge should be simple: the UseHolderForUnforgeable(self.descriptor) block is just gone.
>+ return js::IsInNonStrictPropertySet(cx)
>+ ? opresult.succeed()
>+ : ThrowErrorMessage(cx, MSG_NO_NAMED_SETTER, "${name}");
Again, this seems like it should become an opresult.fail(), in a followup.
> class CGDOMJSProxyHandler_delete(ClassMethod):
>+ The possibile outcomes:
"possible"
Again, the uses of failCantDelete here in the "indexedBody" and "namedBody" bits will lead to misleading error messages. Followup to fix that.
> DOMProxyHandler::defineProperty(JSContext* cx, JS::Handle<JSObject*> proxy, JS::Handle<jsid> id,
The failGetterOnly bit here is a bit weird, but it's preexisting-weird. Please file a followup on figuring out what it's trying to do and documenting it?
r=me with the above on the DOM bits.
Attachment #8570698 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 63•10 years ago
|
||
Updated•10 years ago
|
Attachment #8570698 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Updated•10 years ago
|
Blocks: es6internalmethods
Assignee | ||
Comment 64•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f6b1ead121a
https://hg.mozilla.org/integration/mozilla-inbound/rev/48c2332309a0
https://hg.mozilla.org/integration/mozilla-inbound/rev/da286f0f7a49
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b18c04de86c
https://hg.mozilla.org/integration/mozilla-inbound/rev/0712a3d4b79c
https://hg.mozilla.org/integration/mozilla-inbound/rev/35f7c0795116
https://hg.mozilla.org/integration/mozilla-inbound/rev/e85721e91692
https://hg.mozilla.org/integration/mozilla-inbound/rev/41df9affe00f
Comment 65•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3f6b1ead121a
https://hg.mozilla.org/mozilla-central/rev/48c2332309a0
https://hg.mozilla.org/mozilla-central/rev/da286f0f7a49
https://hg.mozilla.org/mozilla-central/rev/2b18c04de86c
https://hg.mozilla.org/mozilla-central/rev/0712a3d4b79c
https://hg.mozilla.org/mozilla-central/rev/35f7c0795116
https://hg.mozilla.org/mozilla-central/rev/e85721e91692
https://hg.mozilla.org/mozilla-central/rev/41df9affe00f
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in
before you can comment on or make changes to this bug.
Description
•