Closed Bug 1071177 Opened 5 years ago Closed 5 years ago

Make CPOWs support symbol ids

Categories

(Core :: IPC, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jorendorff, Assigned: evilpie)

References

Details

Attachments

(4 files, 1 obsolete file)

Bug 918828 requires support for at least calling the Symbol.iterator method via a CPOW. Supporting symbol ids across all operations is the Right Thing.

This involves, particularly, changing all the 'nsString id' parameters throughout js/ipc/PJavaScript.ipdl to JSVariants, and making sure that the various types of symbols are correctly smuggled across the process boundary.

Or, serialized. Whatever they're calling it these days.
Assignee: nobody → evilpies
Let's start by introducing a JSIDVariant type that we are going to extend with union value for "SymbolVariant".
Attachment #8493324 - Flags: review?(wmccloskey)
First the two easy cases.
Attachment #8493410 - Flags: review?(wmccloskey)
Attachment #8493410 - Flags: review?(jorendorff)
Attached patch SymbolValueSplinter Review
Allows us to pass around symbol typed values.
Attachment #8493411 - Flags: review?(wmccloskey)
Attached patch unique-symbol-cpow (obsolete) — Splinter Review
The tracing bits here aren't set up yet. I am not even sure what kind of tracing is required here? We don't seem to have JS_CallSymbolTracer.

It would be good if somebody would double check the logic of all those wicked tables. I think I've got that right now, i.e. passing a symbol to the child, and then reading it back, gives you the same symbol.
Comment on attachment 8493410 [details] [diff] [review]
WellKnown and RegisteredSymbols

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

r=me on the js/src-related bits, with a few comments.

This doesn't have tests which would ordinarily make it an automatic r- for me, but I'll leave that to billm.

::: js/ipc/JavaScriptLogging.h
@@ +192,5 @@
>  
>      void format(const Identifier &id, nsCString &out) {
>          switch (id.variant.type()) {
> +          case JSIDVariant::TSymbolVariant: {
> +              out = "<Symbol>";

Huh. That's kind of lame. Why not include the description?

jsstr.cpp:SymbolToSource (used by uneval(sym)) does something nice.

::: js/ipc/WrapperOwner.cpp
@@ +936,5 @@
> +        *symVarp = RegisteredSymbol(autoStr);
> +        return true;
> +    }
> +
> +    return false;

Can you return false here without setting an error message or something?

@@ +946,5 @@
> +    switch (symVar.type()) {
> +      case SymbolVariant::TWellKnownSymbol:
> +      {
> +        uint32_t which = symVar.get_WellKnownSymbol().which();
> +        return GetWellKnownSymbol(cx, static_cast<SymbolCode>(which));

Is it OK for the remote side to be able to force the server to fail an assertion by sending bad data?

Assuming that's not OK, this must check that 'which' is in range and fail (safely, rather than proceed) if it's not.

@@ +957,5 @@
> +            return nullptr;
> +        return GetSymbolFor(cx, str);
> +      }
> +      default:
> +        return nullptr;

Same question here.
Attachment #8493410 - Flags: review?(jorendorff) → review+
Comment on attachment 8493324 [details] [diff] [review]
Introduce a JSIDVariant

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

Can we get rid of JavaScriptShared::convertIdToGeckoString and JavaScriptShared::convertGeckoStringToId?

::: js/ipc/WrapperAnswer.cpp
@@ +87,5 @@
>  }
>  
>  bool
> +WrapperAnswer::AnswerGetPropertyDescriptor(const ObjectId &objId, const JSIDVariant &idVar,
> +					                       ReturnStatus *rs, PPropertyDescriptor *out)

Somehow the indent got screwed up here.

@@ +121,5 @@
>  }
>  
>  bool
> +WrapperAnswer::AnswerGetOwnPropertyDescriptor(const ObjectId &objId, const JSIDVariant &idVar,
> +					                          ReturnStatus *rs, PPropertyDescriptor *out)

Actually, in all of these.
Attachment #8493324 - Flags: review?(wmccloskey) → review+
Comment on attachment 8493324 [details] [diff] [review]
Introduce a JSIDVariant

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

::: js/ipc/JavaScriptShared.cpp
@@ +349,5 @@
> +    if (JSID_IS_INT(from)) {
> +        *to = JSID_TO_INT(from);
> +        return true;
> +    }
> +    return false;

Actually, we should MOZ_CRASH here.

@@ +364,5 @@
> +          to.set(INT_TO_JSID(from.get_int32_t()));
> +          return true;
> +
> +        default:
> +            return false;

And here.
Comment on attachment 8493410 [details] [diff] [review]
WellKnown and RegisteredSymbols

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

Please do add some tests. It should be easy.

::: js/ipc/WrapperOwner.cpp
@@ +919,5 @@
>      return obj;
>  }
> +
> +bool
> +WrapperOwner::toSymbolVariant(JSContext *cx, JS::Symbol *symArg, SymbolVariant *symVarp)

Why are these in WrapperOwner? They belong in JavaScriptShared.

@@ +936,5 @@
> +        *symVarp = RegisteredSymbol(autoStr);
> +        return true;
> +    }
> +
> +    return false;

This seems like a good place to MOZ_CRASH.

@@ +957,5 @@
> +            return nullptr;
> +        return GetSymbolFor(cx, str);
> +      }
> +      default:
> +        return nullptr;

Same here.
Attachment #8493410 - Flags: review?(wmccloskey) → review+
Comment on attachment 8493411 [details] [diff] [review]
SymbolValue

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

We should have a test for this too.
Attachment #8493411 - Flags: review?(wmccloskey) → review+
Lots of superficial conflicts with bug 1065811.
These patches don't make any changes to CallGetPropertyNames, probably because it has a confusing name that makes it look as though we don't need to worry about symbols.

But we do -- Bug 1026918 will fix the naming and make this clearer, I think.
Attachment #8493431 - Attachment is obsolete: true
Turns out the patch I landed was wrong and never ran...
Attachment #8502132 - Flags: review?(wmccloskey)
Not the patch never ran, but the test that I wrote never ran.
We also throw an exception on unique symbols now, because support them over CPOWs is most likely not worth the effort. Only new code will use symbols and in that case CPOWs should be avoided anyway.
Comment on attachment 8502132 [details] [diff] [review]
Support symbol keys and throw exception on unique symbols

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

Thanks!
Attachment #8502132 - Flags: review?(wmccloskey) → review+
https://hg.mozilla.org/mozilla-central/rev/f5b05c63480d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.