Boris Zbarsky [:bzbarsky] Reporter
	Description • 11 years ago

This is needed for [[Delete]] and [[DefineOwnProperty]].  They need to be able to Reject (basically by pretending that the relevant properties are write-only and non-configurable).

For [[DefineOwnProperty]] maybe JSObject::reportReadOnly(cx, id, JSREPORT_ERROR) is the right thing for the "strict" case and we just need public API for it?  Hard to tell.

For delete_, of course, we don't even know whether we're strict.  :(

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 1 • 11 years ago

Oh, and the point is that in the set() proxy hook I really do want the reportReadOnly thing.

	Jason Orendorff [:jorendorff] Assignee
	Comment 2 • 9 years ago

Meanwhile in bug 1113369...

>  bool
>  nsOuterWindowProxy::set(JSContext *cx, JS::Handle<JSObject*> proxy,
>                          JS::Handle<JSObject*> receiver,
>                          JS::Handle<jsid> id,
> -                        bool strict,
> -                        JS::MutableHandle<JS::Value> vp) const
> +                        JS::MutableHandle<JS::Value> vp,
> +                        JS::ObjectOpResult *result) const
>  {
>    int32_t index = GetArrayIndexFromId(cx, id);
>    if (IsArrayIndex(index)) {
>      // Reject (which means throw if and only if strict) the set.
> -    if (strict) {
> -      // XXXbz This needs to throw, but see bug 828137.
> -    }
> -    return true;
> +    return result->failReadOnly();
>    }
>  
> -  return js::Wrapper::set(cx, proxy, receiver, id, strict, vp);
> +  return js::Wrapper::set(cx, proxy, receiver, id, vp, result);
>  }

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 3 • 9 years ago

Most excellent.  Once that lands, I'll reevaluate what, if anything, is left in this bug.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 6 • 9 years ago

Comment on attachment 8564259 [details] [diff] [review]
part 1 - Make Object.defineProperty fail on window elements

r=me

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 7 • 9 years ago

Comment on attachment 8564260 [details] [diff] [review]
part 2 - Make [[Set]] always fail on window elements. With this change, `window[0] = null;` is a TypeError in strict mode code

r=me, but you also need to hg rm testing/web-platform/meta/html/browsers/the-window-object/window-indexed-properties-strict.html.ini, right?

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 8 • 9 years ago

Comment on attachment 8564259 [details] [diff] [review]
part 1 - Make Object.defineProperty fail on window elements

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

::: dom/base/test/test_window_indexing.html
@@ +57,5 @@
> +      if (!(exc instanceof errorCtor))
> +        throw exc;
> +      return;
> +    }
> +    throw new Error("expected TypeError, no exception thrown: " + f);

Hardcoded "TypeError"

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 9 • 9 years ago

Comment on attachment 8564260 [details] [diff] [review]
part 2 - Make [[Set]] always fail on window elements. With this change, `window[0] = null;` is a TypeError in strict mode code

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

::: dom/base/nsGlobalWindow.cpp
@@ +916,5 @@
>    int32_t index = GetArrayIndexFromId(cx, id);
>    if (IsArrayIndex(index)) {
> +    // Reject the set.  It's up to the caller to decide whether to throw a
> +    // TypeError.  If the caller is strict mode JS code, it'll throw.
> +    return result->failReadOnly();

... why is this ->?

	Jason Orendorff [:jorendorff] Assignee
	Comment 10 • 9 years ago

(In reply to :Ms2ger from comment #9)
> > +    return result->failReadOnly();
> 
> ... why is this ->?

Sorry. Leftovers from a previous revision in which the outparam was a pointer. Fixed locally, along with all the other differences. This is now waiting for bug 1113369 (and a "-b o" try server run).

	Jason Orendorff [:jorendorff] Assignee
	Comment 11 • 9 years ago

https://tbpl.mozilla.org/?tree=Try&rev=615f23ada38a

	Jason Orendorff [:jorendorff] Assignee
	Comment 12 • 9 years ago

https://tbpl.mozilla.org/?tree=Try&rev=fd8244ca07f3

	Jason Orendorff [:jorendorff] Assignee
	Comment 13 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/df26246112ad
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2ffd91dee7c

	Carsten Book [:Tomcat]
	Comment 14 • 9 years ago

sorry had to back this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=306ea1a7975d for Jetpack Test failures like :

https://treeherder.mozilla.org/logviewer.html#?job_id=7371703&repo=mozilla-inbound

	Jason Orendorff [:jorendorff] Assignee
	Comment 15 • 9 years ago

Fix for the Jetpack issue is happening in bug 1132522. Will reland soon.

	Jason Orendorff [:jorendorff] Assignee
	Comment 16 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d5e0ae90498
https://hg.mozilla.org/integration/mozilla-inbound/rev/68f9f9c4868b

	Carsten Book [:Tomcat]
	Comment 17 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/1d5e0ae90498
https://hg.mozilla.org/mozilla-central/rev/68f9f9c4868b

	Florian Scholz (Open Web Docs)
	Comment 18 • 9 years ago

https://developer.mozilla.org/en-US/Firefox/Releases/39#JavaScript
https://developer.mozilla.org/en-US/Firefox/Releases/39/Site_Compatibility#Proxy_handers_may_throw_under_specific_conditions