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

RESOLVED FIXED in Firefox 39

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: bz, Assigned: jorendorff)

Tracking

(Blocks: 1 bug, {dev-doc-complete, site-compat})

unspecified
mozilla39
dev-doc-complete, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(2 attachments)

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.  :(
Oh, and the point is that in the set() proxy hook I really do want the reportReadOnly thing.

Updated

5 years ago
Depends on: 803157
Assignee: general → nobody
(Assignee)

Comment 2

3 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);
>  }
Most excellent.  Once that lands, I'll reevaluate what, if anything, is left in this bug.
Depends on: 1113369
(Assignee)

Comment 4

3 years ago
Created attachment 8564259 [details] [diff] [review]
part 1 - Make Object.defineProperty fail on window elements
Attachment #8564259 - Flags: review?(bzbarsky)
(Assignee)

Updated

3 years ago
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
(Assignee)

Comment 5

3 years ago
Created 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
Attachment #8564260 - Flags: review?(bzbarsky)
Comment on attachment 8564259 [details] [diff] [review]
part 1 - Make Object.defineProperty fail on window elements

r=me
Attachment #8564259 - Flags: review?(bzbarsky) → review+
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?
Attachment #8564260 - Flags: review?(bzbarsky) → review+
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 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 ->?
(Assignee)

Comment 10

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

Updated

3 years ago
Blocks: 1139700
(Assignee)

Comment 15

3 years ago
Fix for the Jetpack issue is happening in bug 1132522. Will reland soon.
Flags: needinfo?(jorendorff)

Updated

3 years ago
Keywords: dev-doc-needed, site-compat
OS: Mac OS X → All
Hardware: x86 → All
https://hg.mozilla.org/mozilla-central/rev/1d5e0ae90498
https://hg.mozilla.org/mozilla-central/rev/68f9f9c4868b
Status: ASSIGNED → RESOLVED
Last Resolved: 3 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.