Closed
Bug 1082672
Opened 10 years ago
Closed 10 years ago
Add miscellaneous symbol support (preliminary work to bug 918828)
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: jorendorff, Assigned: jorendorff)
Details
Attachments
(4 files)
24.07 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
942 bytes,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
24.06 KB,
patch
|
bholley
:
review+
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
2.33 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
Bug 918828 is already too big, so I'm splitting out the preliminary parts.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8504825 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•10 years ago
|
||
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+
Assignee | ||
Comment 3•10 years ago
|
||
(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.)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8504881 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•10 years ago
|
Attachment #8504825 -
Attachment is obsolete: true
Attachment #8504825 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 5•10 years ago
|
||
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;
Assignee | ||
Comment 6•10 years ago
|
||
(sorry, premature Tab+Enter) It should be like this: uint32_t index; if (!ArrayIndexFromId(cx, id, &index)) return false; if (IsArrayIndex... ... ...
Assignee | ||
Updated•10 years ago
|
Attachment #8504825 -
Attachment is obsolete: false
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8504892 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8504895 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 9•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=6e06f52c6fbf
Comment 10•10 years ago
|
||
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 11•10 years ago
|
||
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 12•10 years ago
|
||
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 13•10 years ago
|
||
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+
Assignee | ||
Comment 14•10 years ago
|
||
(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);
Assignee | ||
Comment 15•10 years ago
|
||
(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.
Assignee | ||
Comment 16•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=f78cbb83f63d
Assignee | ||
Comment 17•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac0d55e594f1 https://hg.mozilla.org/integration/mozilla-inbound/rev/bff9837442af https://hg.mozilla.org/integration/mozilla-inbound/rev/f2214b9e3333 https://hg.mozilla.org/integration/mozilla-inbound/rev/9a7fd8fd00b3
Comment 18•10 years ago
|
||
(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.
Comment 19•10 years ago
|
||
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!
Comment 20•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ac0d55e594f1 https://hg.mozilla.org/mozilla-central/rev/bff9837442af https://hg.mozilla.org/mozilla-central/rev/f2214b9e3333 https://hg.mozilla.org/mozilla-central/rev/9a7fd8fd00b3
Status: ASSIGNED → 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
•