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

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
5 years ago
3 months ago

People

(Reporter: bzbarsky, Assigned: jorendorff)

Tracking

unspecified
mozilla34
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

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...
Flags: needinfo?(jorendorff)
Assignee

Comment 1

5 years ago
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.
Flags: needinfo?(jorendorff)
Assignee

Comment 2

5 years ago
The most important changes in here are GC changes, so Terrence gets the review request.
Assignee: nobody → jorendorff
Attachment #8468870 - Flags: review?(terrence)
Assignee

Comment 3

5 years ago
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.
Attachment #8468871 - Flags: review?(jst)
Assignee

Comment 4

5 years ago
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.
Attachment #8468873 - Flags: review?(peterv)
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?
Attachment #8468878 - Flags: review?(bobbyholley) → review+
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
Attachment #8468871 - Flags: review?(jst) → review+
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.
Attachment #8468870 - Flags: review?(terrence) → review+
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?
Attachment #8468873 - Flags: review?(peterv) → review+
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)?
Attachment #8468875 - Flags: review?(peterv) → review+
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 :-).
Attachment #8468877 - Flags: review?(peterv) → review+
Assignee

Comment 14

5 years ago
(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, jst@mozilla.com) 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.
Assignee

Comment 15

5 years ago
(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.
Assignee

Comment 16

5 years ago
(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.
Depends on: 1058252
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.