Closed
Bug 741041
Opened 10 years ago
Closed 10 years ago
Wrapper hygiene for typed arrays and friends
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: sfink, Assigned: sfink)
References
Details
Attachments
(3 files, 4 obsolete files)
2.17 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
21.84 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
16.21 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
Use UnwrapObjectChecked where possible, and ensure that array buffers and their views are in the same compartment: when creating a view, it should be created in the same compartment as the array buffer and then wrapped for the calling compartment
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #611128 -
Flags: review?(luke)
Assignee | ||
Comment 2•10 years ago
|
||
Shell test function for checking if the fully unwrapped form of two objects are in the same compartment
Attachment #611129 -
Flags: review?(luke)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #611130 -
Flags: review?(luke)
Assignee | ||
Comment 4•10 years ago
|
||
I'm not sure if it should be isProxy or isWrapper.
Depends on: 741040
![]() |
||
Comment 5•10 years ago
|
||
Comment on attachment 611128 [details] [diff] [review] Implement isProxy testing command for JS shell Review of attachment 611128 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/TestingFunctions.cpp @@ +134,5 @@ > + JS_ReportError(cx, "the function takes exactly one argument"); > + return false; > + } > + if (!args[0].isObject()) { > + args.rval().setBoolean(true); Perhaps setBoolean(false) ?
Attachment #611128 -
Flags: review?(luke) → review+
![]() |
||
Comment 6•10 years ago
|
||
Comment on attachment 611129 [details] [diff] [review] Implement isSameCompartment JS shell command for tests The semantics of this function are a bit twitchy. What about instead phrasing the test in terms of globals (which will hopefully soon have the same meaning as "compartment") by adding a "getGlobalForObject(o)" which returns JS_GetGlobalForObject? Then you could assert things in terms of globals. This just made me realize something about these typed array changes, though: they change visible semantics by changing the global of the typed array. Conceivably, this breaks 'instanceof' and I think implicit 'this'? I suppose this could work by wrapping the correct prototype (from the correct global).
![]() |
||
Updated•10 years ago
|
Attachment #611129 -
Flags: review?(luke)
![]() |
||
Updated•10 years ago
|
Attachment #611130 -
Flags: review?(luke)
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #6) > Comment on attachment 611129 [details] [diff] [review] > Implement isSameCompartment JS shell command for tests > > The semantics of this function are a bit twitchy. What about instead > phrasing the test in terms of globals (which will hopefully soon have the > same meaning as "compartment") by adding a "getGlobalForObject(o)" which > returns JS_GetGlobalForObject? Then you could assert things in terms of > globals. That's a good idea. On further thought, though, I don't think this test is necessary. I have asserts in the code that would trigger if it were possible to create a cross-compartment typed array. So I think I'll just scrap this. > This just made me realize something about these typed array changes, though: > they change visible semantics by changing the global of the typed array. > Conceivably, this breaks 'instanceof' and I think implicit 'this'? I > suppose this could work by wrapping the correct prototype (from the correct > global). Uploading a new pair of patches, one to do everything but the cross-compartment case, and one to set the proto to a wrapper of the proto from the calling compartment. I won't request review until I land some other patches, though.
Assignee: sphink → nobody
Component: JavaScript Engine → Keyboard: Navigation
QA Contact: general → keyboard.navigation
Target Milestone: --- → mozilla14
Version: unspecified → Trunk
Assignee | ||
Comment 8•10 years ago
|
||
Assignee: nobody → sphink
Attachment #611129 -
Attachment is obsolete: true
Attachment #611130 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Assignee | ||
Comment 9•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Assignee: sphink → general
Component: Keyboard: Navigation → JavaScript Engine
QA Contact: keyboard.navigation → general
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #623306 -
Flags: review?(luke)
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #623309 -
Flags: review?(luke)
![]() |
||
Comment 12•10 years ago
|
||
Comment on attachment 623306 [details] [diff] [review] Use UnwrapObjectChecked instead of UnwrapObject wherever applicable >+js_NewTypedArray(JSContext *cx, uint32_t atype, int32_t nelements); I don't see any use/definition of this...
Attachment #623306 -
Flags: review?(luke) → review+
![]() |
||
Comment 13•10 years ago
|
||
Comment on attachment 623309 [details] [diff] [review] Ensure ArrayBufferViews and their ArrayBuffers are in the same compartment Review of attachment 623309 [details] [diff] [review]: ----------------------------------------------------------------- This is really pretty clean, nice job. ::: js/src/jstypedarray.cpp @@ +1537,5 @@ > + // compartment. constructWrapped() is invoked in the ArrayBuffer's > + // compartment, and is passed in a (wrapped) prototype object from the > + // origin compartment to use as the newly-created view's proto. > + static JSBool > + constructWrapped(JSContext *cx, unsigned argc, Value *vp) How about just naming this fromBuffer (overload resolution should pick the right one for Proxy::nativeCall) and dropping all mention of wrappers since, technically, this is just a native that calls fromBuffer ; nothing wrapper-y about it. Instead, the comment can go next to... @@ +1726,5 @@ > + if (bufobj->isWrapper()) { > + JSObject *wrapped = UnwrapObjectChecked(cx, bufobj); > + if (!wrapped) > + return NULL; > + if (wrapped->isArrayBuffer()) { ... this piece of code. Now, instead of explicit wrapper testing, could you add a new ESClass so that this could be written: /* * Normally, NonGenericMethodGuard handles the case of transparent wrappers. * However, we have a peculiar situation: we want to construct the new typed * array in the compartment of the buffer. This is necessary since typed arrays * need to point directly at their buffer's data. Instead, we use the underlying * machinery directly to proxy the native call. * if (!ObjectClassIs(cx, *bufobj, ESClass_ArrayBuffer, cx)) { JS_ReportErrorNumber(cx, ...); return NULL; } JS_ASSERT(bufobj->isArrayBuffer() || bufobj->isProxy()); if (bufobj->isProxy()) { ... Proxy::nativeCall ... } ... normal path @@ +1741,5 @@ > + > + CallArgs args = CallArgsFromVp(argc, argv); > + if (!Proxy::nativeCall(cx, bufobj, &ArrayBufferClass, constructWrapped, args)) > + return NULL; > + return args.rval().toObjectOrNull(); Can this be &args.rval().toObject()? @@ +2301,5 @@ > return true; > } > > JSBool > +DataViewObject::constructWrapped(JSContext *cx, unsigned argc, Value *vp) Same comments as above, mutatis mutandis.
![]() |
||
Comment 14•10 years ago
|
||
Comment on attachment 623309 [details] [diff] [review] Ensure ArrayBufferViews and their ArrayBuffers are in the same compartment r+ with the UnwrapObject's replaced with ObjectClassIs as mentioned above.
Attachment #623309 -
Flags: review?(luke) → review+
Assignee | ||
Updated•10 years ago
|
Attachment #618474 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #618475 -
Attachment is obsolete: true
Assignee | ||
Comment 16•10 years ago
|
||
Requested changes made. I landed the two non-test patches merged because it was easier to do the update for RootedVarObject&friends that way. https://hg.mozilla.org/integration/mozilla-inbound/rev/df84190b1c0a https://hg.mozilla.org/integration/mozilla-inbound/rev/73b380d3edd8
Target Milestone: mozilla14 → mozilla15
Comment 17•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/df84190b1c0a https://hg.mozilla.org/mozilla-central/rev/73b380d3edd8 https://hg.mozilla.org/mozilla-central/rev/a9f05eabf20b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•