828137 - Need APIs that would allow proxies to implement Reject in spec terms

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

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

Comment 2

[Jason Orendorff [:jorendorff]]

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);
>  }

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: part 1 - Make Object.defineProperty fail on window elements

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: part 2 - Make [[Set]] always fail on window elements. With this change, `window[0] = null;` is a TypeError in strict mode code

Comment 6

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 7

[Boris Zbarsky [:bzbarsky]]

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?

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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"

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 10

[Jason Orendorff [:jorendorff]]

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

Comment 11

[Jason Orendorff [:jorendorff]]

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

Comment 12

[Jason Orendorff [:jorendorff]]

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

Comment 13

[Jason Orendorff [:jorendorff]]

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

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

[Jason Orendorff [:jorendorff]]

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

Comment 16

[Jason Orendorff [:jorendorff]]

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

Comment 17

[Carsten Book [:Tomcat]]

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

Comment 18

[Florian Scholz (Open Web Docs)]

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