Closed
Bug 1071177
Opened 10 years ago
Closed 10 years ago
Make CPOWs support symbol ids
Categories
(Core :: IPC, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: jorendorff, Assigned: evilpie)
References
Details
Attachments
(4 files, 1 obsolete file)
37.21 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
6.50 KB,
patch
|
billm
:
review+
jorendorff
:
review+
|
Details | Diff | Splinter Review |
3.02 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
14.51 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•10 years ago
|
Assignee: nobody → evilpies
Assignee | ||
Comment 1•10 years ago
|
||
Let's start by introducing a JSIDVariant type that we are going to extend with union value for "SymbolVariant".
Attachment #8493324 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 2•10 years ago
|
||
First the two easy cases.
Attachment #8493410 -
Flags: review?(wmccloskey)
Attachment #8493410 -
Flags: review?(jorendorff)
Assignee | ||
Comment 3•10 years ago
|
||
Allows us to pass around symbol typed values.
Attachment #8493411 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 4•10 years ago
|
||
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.
Reporter | ||
Comment 5•10 years ago
|
||
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+
Reporter | ||
Comment 10•10 years ago
|
||
Lots of superficial conflicts with bug 1065811.
Reporter | ||
Comment 11•10 years ago
|
||
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.
Reporter | ||
Comment 12•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=a5414c47b651
Assignee | ||
Comment 13•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/765ab5eaab5b https://hg.mozilla.org/integration/mozilla-inbound/rev/6e6f184e5928 https://hg.mozilla.org/integration/mozilla-inbound/rev/2279791b5c6e https://hg.mozilla.org/integration/mozilla-inbound/rev/435e1a7b1a34
Keywords: leave-open
Comment 14•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/765ab5eaab5b https://hg.mozilla.org/mozilla-central/rev/6e6f184e5928 https://hg.mozilla.org/mozilla-central/rev/2279791b5c6e https://hg.mozilla.org/mozilla-central/rev/435e1a7b1a34
Flags: in-testsuite+
Assignee | ||
Updated•10 years ago
|
Attachment #8493431 -
Attachment is obsolete: true
Assignee | ||
Comment 15•10 years ago
|
||
Turns out the patch I landed was wrong and never ran...
Attachment #8502132 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 16•10 years ago
|
||
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+
Assignee | ||
Comment 18•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5b05c63480d
Keywords: leave-open
Comment 19•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f5b05c63480d
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•