1041261 - Need to audit all DOM code that uses JSID_IS_* to make sure it can deal with symbols

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

And probably the Web IDL spec too. For example, right now getOwnPropertyDescriptor for things with named getters does:

    binding_detail::FakeString name;
    if (MOZ_LIKELY(JSID_IS_STRING(id))) {
      if (!AssignJSString(cx, name, JSID_TO_STRING(id))) {
        return false;
      }
    } else {
      nameVal = js::IdToValue(id);
      if (!ConvertJSValueToString(cx, nameVal, eStringify, eStringify,
                                  name)) {
        return false;
      }
    }

which will basically stringify a symbol id.  That made sense when jsid was a string or integer, but I'm not sure that makes sense in the symbol world.

Relatedly, the algorithm at http://heycam.github.io/webidl/#getownproperty definitely seems to assume that all property names are strings (which they were in JS-the-spec until symbols came along...

Jason, I assume this needs to happen for the same release that will actually enable shipped symbols?  Please set tracking flags as needed...

Comment 1

[Jason Orendorff [:jorendorff]]

I'm doing this now. The audit turned up several things; I re-audited everything inside the JS engine and found some stuff in there, too.

Status: Already audited about 200 hits; Codegen.py and js/xpconnect/wrappers remain to be audited (just 8 hits but probably about 50% of the work). Should have a patch or three up for review here tomorrow.

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: Part 1 - Misc. engine changes

The most important changes in here are GC changes, so Terrence gets the review request.

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: Part 2 - Plugins

jst: ES6 introduces a new kind of property id, called Symbols. So in addition to JSID_IS_STRING and JSID_IS_INT, we now have JSID_IS_SYMBOL. This patch changes some code in dom/plugins to just ignore symbols.

The first change is in a resolve hook; there it's always OK to do nothing and return true.

The second change, I'm not so sure. There are no calls to this method RecvRetain in the codebase. Symbols don't have anything analogous to interning a string, so maybe it's right to do nothing here as well? I would love to know how this is called.

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: Part 3 - Codegen.py scoping changes

This is the first of three patches to Codegen.py.

The end goal of these three patches is to change some proxy handlers from doing

    <<convert a jsid to a string>>
    <<call a method, passing the string>>

to be more like this:

    if (<<the jsid is NOT a symbol>>) {
      <<convert a jsid to a string>>
      <<call a method, passing the string>>
    }

The problem is that currently, the <<call a method>> part declares two local variables, `found` and `result`, which may be used in later code. The if-block causes those variables to go out of scope; we end up with:

    if (<<the jsid is NOT a symbol>>) {
      <<convert a jsid to a string>>
      bool found;
      bool result;
      result = <<call a method, passing the string>>
    }
    <<...code that uses found and result>>

So here's what I did:

  - Part 3 makes it possible for Python code to declare a `found` variable
    and/or a `result` variable and tell the CGProxySpecialWhatever class
    to assign to the existing variable instead of declaring its own.
    This part does not change the $OBJDIR/dom/bindings output at all.
    It's just laying the groundwork.

  - Part 4 makes some callers actually do that, declaring `found` or `bool`
    up front.

  - Part 5 introduces the new if-block, which now works because part 4
    moved the bool variable declarations up outside of the if-statement.

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: Part 4 - Codegen.py: Move some boolean variable declarations

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: Part 5 - Codegen.py: Change named/indexed ops to cope with symbols

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: Part 6 - Change a few comments in XPConnect to accept the possibility of properties with symbol ids.

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 8468878 [details] [diff] [review]
Part 6 - Change a few comments in XPConnect to accept the possibility of properties with symbol ids.

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

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +798,5 @@
>      if (js::StandardClassIsDependent(protoKey))
>          return true;
>  
>      // Compute the property name we're looking for. We'll handle indexed
> +    // properties when we start supporting arrays. We'll handle well-known

This stuff about arrays is now bogus, because we support them - can you change the comment to indicate that indexed array properties are handled above?

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 8468871 [details] [diff] [review]
Part 2 - Plugins

A plugin can only get an NPIdentifier through the NPAPI which only ever hands out strings, ints, or undefined, so this change looks good. FWIW, PluginIdentifierParent::RecvRetain() is called by the IPDL generated code, from PluginIdentifierChild::MakePermanent().

r=jst

Comment 10

[Terrence Cole [:terrence]]

Comment on attachment 8468870 [details] [diff] [review]
Part 1 - Misc. engine changes

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

::: js/src/jsinfer.cpp
@@ +4219,2 @@
>                  }
> +                

You have trailing whitespace on this line. Please remove.

Comment 11

[Peter Van der Beken [:peterv]]

Comment on attachment 8468873 [details] [diff] [review]
Part 3 - Codegen.py scoping changes

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

::: dom/bindings/Codegen.py
@@ +9378,5 @@
>  
>      If checkFound is False, will just assert that the prop is found instead of
>      checking that it is before wrapping the value.
> +
> +    foundVar: For getters and deleters, the generated code can also set a bool

Can you also document resultVar?

@@ +9385,5 @@
> +    pass the name of the bool variable as the foundVar keyword argument to the
> +    constructor. The caller is responsible for declaring the variable.
> +
> +    (Currently, the generated code always declares a boolean variable named
> +    "found". The next patch in this stack will change this.)

You should remove this last paragraph.

@@ +9458,5 @@
>      If checkFound is False, will just assert that the prop is found instead of
>      checking that it is before wrapping the value.
>      """
>      def __init__(self, descriptor, name, doUnwrap=True, checkFound=True,
> +                 argumentMutableValue=None, resultVar=None, foundVar=None):

Maybe refer to the docs in CGProxySpecialOperation for resultVar and foundVar, here and in the others below?

Comment 12

[Peter Van der Beken [:peterv]]

Comment on attachment 8468875 [details] [diff] [review]
Part 4 - Codegen.py: Move some boolean variable declarations

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

::: dom/bindings/Codegen.py
@@ +10109,5 @@
>                    return true;
>                  }
>  
>                  """,
> +                presenceChecker=CGProxyIndexedPresenceChecker(self.descriptor, foundVar="found").define())

Just to make sure I understand, this isn't strictly needed (since it's an indexed operation)?

Comment 13

[Peter Van der Beken [:peterv]]

Comment on attachment 8468877 [details] [diff] [review]
Part 5 - Codegen.py: Change named/indexed ops to cope with symbols

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

::: dom/bindings/Codegen.py
@@ +4007,5 @@
>              JS::Rooted<jsid> curId(cx);
>              for (size_t i = 0; i < ids.length(); ++i) {
>                // Make sure we get the value before converting the name, since
>                // getting the value can trigger GC but our name is a dependent
>                // string.

Let's remove the comment since you make it false :-).

Comment 14

[Jason Orendorff [:jorendorff]]

(In reply to Bobby Holley (:bholley) from comment #8)
> ::: js/xpconnect/wrappers/XrayWrapper.cpp
> >      // Compute the property name we're looking for. We'll handle indexed
> > +    // properties when we start supporting arrays. We'll handle well-known
> 
> This stuff about arrays is now bogus, because we support them - can you
> change the comment to indicate that indexed array properties are handled
> above?

Done.

(In reply to Johnny Stenback  (:jst, [e-mail]) from comment #9)
> PluginIdentifierParent::RecvRetain() is called by the IPDL generated code,
> from PluginIdentifierChild::MakePermanent().

Ah, thanks.

(In reply to Terrence Cole [:terrence] from comment #10)
> You have trailing whitespace on this line. Please remove.

Done, thanks.

Comment 15

[Jason Orendorff [:jorendorff]]

(In reply to Peter Van der Beken [:peterv] from comment #11)
> ::: dom/bindings/Codegen.py
> Can you also document resultVar?

I added a pointer to the documentation (on class CGCallGenerator).

> You should remove this last paragraph.

Done.

> Maybe refer to the docs in CGProxySpecialOperation for resultVar and
> foundVar, here and in the others below?

I added pointers to the documentation everywhere.

Comment 16

[Jason Orendorff [:jorendorff]]

(In reply to Peter Van der Beken [:peterv] from comment #12)
> ::: dom/bindings/Codegen.py
> > +                presenceChecker=CGProxyIndexedPresenceChecker(self.descriptor, foundVar="found").define())
> 
> Just to make sure I understand, this isn't strictly needed (since it's an
> indexed operation)?

Right. I just changed it for consistency.

Comment 17

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8e3511f49f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/26f6fc591b4c
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb1eefe3c485
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fda269ec423
https://hg.mozilla.org/integration/mozilla-inbound/rev/822527b6f13b

Comment 18

[Jason Orendorff [:jorendorff]]

and https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-inbound&revision=f552c962fea3

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d8e3511f49f4
https://hg.mozilla.org/mozilla-central/rev/26f6fc591b4c
https://hg.mozilla.org/mozilla-central/rev/fb1eefe3c485
https://hg.mozilla.org/mozilla-central/rev/8fda269ec423
https://hg.mozilla.org/mozilla-central/rev/822527b6f13b

Comment 20

[Ryan VanderMeulen [:RyanVM]]

adn https://hg.mozilla.org/mozilla-central/rev/f552c962fea3