Closed Bug 1082672 Opened 5 years ago Closed 5 years ago

Add miscellaneous symbol support (preliminary work to bug 918828)

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jorendorff, Assigned: jorendorff)

Details

Attachments

(4 files)

Bug 918828 is already too big, so I'm splitting out the preliminary parts.
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
Comment on attachment 8504825 [details] [diff] [review]
part 1 - Add JSAPI macros JS_SYM_FN etc. to support defining functions with well-known symbol keys

Carrying forward Waldo's review of this patch on the assumption he has no particular desire to review it again. It's the same patch except for the fix shown in bug 918828 comment 39 (and lots of rebasing).
Attachment #8504825 - Flags: review+
(bz: The review request for you in comment 1 is for the first hunk, mainly --- and really more to make sure you're aware of the change.)
Attachment #8504825 - Attachment is obsolete: true
Attachment #8504825 - Flags: review?(bzbarsky)
mozilla::dom::GetArrayIndexFromId is weird because it returns the same error value (-1) whether an actual exception occurred or not. This seems bad to me.

Generated code like this seems really wrong:

    int32_t index = GetArrayIndexFromId(cx, id);
    if (IsArrayIndex(index)) {
      mozilla::dom::AudioTrackList* self = UnwrapProxy(proxy);
      ...
    }

    JS::Rooted<JSObject*> expando(cx);
    if (!isXray && (expando = GetExpandoObject(proxy))) {
      ...
    }

    desc.object().set(nullptr);
    return true;

Code shouldn't proceed after an error. It should be like this:

    uint32_t index;
(sorry, premature Tab+Enter) It should be like this:

    uint32_t index;
    if (!ArrayIndexFromId(cx, id, &index))
      return false;
    if (IsArrayIndex...
      ...
    ...
Attachment #8504825 - Attachment is obsolete: false
Comment on attachment 8504881 [details] [diff] [review]
part 2 - Change mozilla::dom::GetArrayIndexFromId to cope with symbols

> This seems bad to me.

The only possibly-throwing thing in there is JS::ToNumber on the result of JS_IdToValue, right?

Can that actually ever fail for string or int ids?

I agree that we want to explicitly check for symbol, since it can't be ToNumbered.

Should there be a testcase added here, by the way?

r=me
Attachment #8504881 - Flags: review?(bzbarsky) → review+
Comment on attachment 8504892 [details] [diff] [review]
part 4 - Change XrayWrapper code to be able to resolve symbol-keyed methods

The JS_HAS_SYMBOLS usage here is kinda inconsistent.  Doesn't Object.getOwnPropertySymbols depend on it?  What about Symbol.for?

I'd like Bobby to look at the StandardClassIsDependent bit.

r=me on the rest, with the JS_HAS_SYMBOLS stuff double-checked
Attachment #8504892 - Flags: review?(bzbarsky)
Attachment #8504892 - Flags: review?(bobbyholley)
Attachment #8504892 - Flags: review+
Comment on attachment 8504895 [details] [diff] [review]
part 3 - Add some more symbol support for DOM bindings

r=me
Attachment #8504895 - Flags: review?(bzbarsky) → review+
Comment on attachment 8504892 [details] [diff] [review]
part 4 - Change XrayWrapper code to be able to resolve symbol-keyed methods

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

Thanks for handling the Xray stuff here. r=me modulo the below.

::: js/xpconnect/tests/chrome/test_xrayToJS.xul
@@ +23,5 @@
>    const Cc = Components.classes;
>    const Ci = Components.interfaces;
>    const Cu = Components.utils;
>    let global = Cu.getGlobalForObject.bind(Cu);
> +  const JS_HAS_SYMBOLS = typeof Symbol === "function";

Please put this with the other feature detection code.

@@ +338,2 @@
>      var trickyObject =
> +      iwin.eval(`new Object({

Is the switch to backticks here intentional? Some new ES feature I'm not aware of?

::: js/xpconnect/wrappers/FilteringWrapper.cpp
@@ +215,5 @@
>  {
>      // All properties on cross-origin objects are supposed |own|, despite what
>      // the underlying native object may report. Override the inherited trap to
>      // avoid passing JSITER_OWNONLY as a flag.
> +    return SecurityXrayDOM::enumerate(cx, wrapper, JSITER_HIDDEN | JSITER_SYMBOLS, props);

Wait, why do we want to iterate symbols on XOWs? XOWs only allow access to a whitelisted set of properties, none of which are symbols. so while this code isn't harmful, it's confusing. Please remove it unless I'm missing something.

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +459,5 @@
>      }
>  
> +    // Bail out for dependent classes, since all the rest of the properties we'll
> +    // resolve here will live on the parent prototype.
> +    if (js::StandardClassIsDependent(key))

Yeah I don't know what this is about. I'm guessing it's a rebasing error. Please remove it unless I'm missing something.
Attachment #8504892 - Flags: review?(bobbyholley) → review+
(In reply to Boris Zbarsky [:bz] from comment #10)
> Comment on attachment 8504881 [details] [diff] [review]
> part 2 - Change mozilla::dom::GetArrayIndexFromId to cope with symbols
> 
> > This seems bad to me.
> 
> The only possibly-throwing thing in there is JS::ToNumber on the result of
> JS_IdToValue, right?
> 
> Can that actually ever fail for string or int ids?

I think this is correct, and it *can't* fail. Sorry for the noise!

> I agree that we want to explicitly check for symbol, since it can't be
> ToNumbered.
> 
> Should there be a testcase added here, by the way?

Yes, I'm trying one a simple mochitest right now that just does

    var sym = Symbol("ponies");
    Object.defineProperty(window, sym, {configurable: true, value: 3});
    is(window[sym], 3);
(In reply to Bobby Holley (:bholley) from comment #13)
> @@ +338,2 @@
> >      var trickyObject =
> > +      iwin.eval(`new Object({
> 
> Is the switch to backticks here intentional? Some new ES feature I'm not
> aware of?

Yeah, template strings! They're pretty swell.

They support interpolation, `like this: ${your.expression.here()}`

> Wait, why do we want to iterate symbols on XOWs? XOWs only allow access to a
> whitelisted set of properties, none of which are symbols. so while this code
> isn't harmful, it's confusing. Please remove it unless I'm missing something.

Removed.

> > +    // Bail out for dependent classes, since all the rest of the properties we'll
> > +    // resolve here will live on the parent prototype.
> > +    if (js::StandardClassIsDependent(key))
> 
> Yeah I don't know what this is about. I'm guessing it's a rebasing error.
> Please remove it unless I'm missing something.

Urgh, good catch. Removed.
(In reply to Jason Orendorff [:jorendorff] from comment #14)
> > The only possibly-throwing thing in there is JS::ToNumber on the result of
> > JS_IdToValue, right?
> > 
> > Can that actually ever fail for string or int ids?
> 
> I think this is correct, and it *can't* fail. Sorry for the noise!

I'm not so sure about that.  If the string id is a very long string, then ToNumber -> NonObjectToNumberSlow -> StringToNumber -> CharsToNumber -> js_strtod can fail if we fail to grow that method's chars vector.

Now, that chars vector seems to have no reason to exist for 8-bit strings, given the contents are never overwritten, or so it seems to me.  But for 16-bit strings it needs to exist because js_strtod_harder isn't written to work on 16-bit strings, only 8-bit strings, so a copy is needed.  And so it seems to me ToNumber on atoms has to remain fallible.
Hmm.  So in that case, we could have an exception on cx, but only if reportAllocOverflow() does that.

Which mostly it seems not to...

In any case, I would totally welcome a better "ID is array index" thing!
You need to log in before you can comment on or make changes to this bug.