Last Comment Bug 741040 - Make an ArrayBufferObject subclass of JSOBject
: Make an ArrayBufferObject subclass of JSOBject
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Steve Fink [:sfink] [:s:]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 741041
  Show dependency treegraph
 
Reported: 2012-03-30 21:31 PDT by Steve Fink [:sfink] [:s:]
Modified: 2012-07-19 21:55 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Make an ArrayBufferObject subclass of JSOBject (53.24 KB, patch)
2012-03-30 21:31 PDT, Steve Fink [:sfink] [:s:]
jwalden+bmo: review+
Details | Diff | Splinter Review

Description Steve Fink [:sfink] [:s:] 2012-03-30 21:31:15 PDT
Change ArrayBuffer to ArrayBufferObject
Comment 1 Steve Fink [:sfink] [:s:] 2012-03-30 21:31:23 PDT
Created attachment 611123 [details] [diff] [review]
Make an ArrayBufferObject subclass of JSOBject
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2012-04-03 19:22:26 PDT
Comment on attachment 611123 [details] [diff] [review]
Make an ArrayBufferObject subclass of JSOBject

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

::: js/src/jsclone.cpp
@@ +467,5 @@
>  JSStructuredCloneWriter::writeArrayBuffer(JSObject *obj)
>  {
> +    ArrayBufferObject &buffer = obj->asArrayBuffer();
> +    return out.writePair(SCTAG_ARRAY_BUFFER_OBJECT, buffer.byteLength()) &&
> +           out.writeBytes(buffer.dataPointer(), buffer.byteLength());

Could make it data() rather than dataPointer() if you want to be a little shorter, don't see particular win with either name tho.

@@ +707,5 @@
>  
>  bool
>  JSStructuredCloneReader::readArrayBuffer(uint32_t nbytes, Value *vp)
>  {
> +    JSObject *obj = js::ArrayBufferObject::create(context(), nbytes);

Heh, js:: appears.

::: js/src/jstypedarray.cpp
@@ +201,5 @@
>  /*
>   * new ArrayBuffer(byteLength)
>   */
>  JSBool
> +ArrayBufferObject::class_constructor(JSContext *cx, unsigned argc, Value *vp)

I might rename this to just construct, if I were touching this.  Shorter, as clear or clearer, seems to me.

@@ +268,5 @@
>      return static_cast<JSObject*>(obj->getPrivate());
>  }
>  
>  JSObject *
> +ArrayBufferObject::create(JSContext *cx, int32_t nbytes, uint8_t *contents)

How easy is it to change nbytes to be uint32_t?  Please do so if it's not too big.

@@ +273,2 @@
>  {
> +    JSObject *obj = NewBuiltinClassInstance(cx, &slowClass);

Here you just use |&slowClass|.  Below, you qualify it.  How about we qualify both of them for consistency and explicitness?

@@ +323,5 @@
>  
>      return create(cx, 0);
>  }
>  
> +ArrayBufferObject::~ArrayBufferObject()

Erm.  It really shouldn't be necessary for any JSObject subclass to have a destructor!  Please remove it, and the constructor as well, I think.

@@ +473,5 @@
>      obj = getArrayBuffer(obj);
>      JS_ASSERT(obj);
>      if (JSID_IS_ATOM(id, cx->runtime->atomState.byteLengthAtom)) {
> +        ArrayBufferObject &buffer = obj->asArrayBuffer();
> +        vp->setInt32(buffer.byteLength());

I'd one-line this, myself.

@@ +491,5 @@
>      obj = getArrayBuffer(obj);
>      JS_ASSERT(obj);
>      if (name == cx->runtime->atomState.byteLengthAtom) {
> +        ArrayBufferObject &buffer = obj->asArrayBuffer();
> +        vp->setInt32(buffer.byteLength());

And here too.

@@ -1447,5 @@
>  
>      static JSObject *
>      createTypedArray(JSContext *cx, JSObject *bufobj, uint32_t byteOffset, uint32_t len)
>      {
> -        JS_ASSERT(bufobj->isArrayBuffer());

Why not make the argument type ArrayBufferObject?

@@ +1479,5 @@
>          obj->setSlot(FIELD_TYPE, Int32Value(ArrayTypeID()));
>          obj->setSlot(FIELD_BUFFER, ObjectValue(*bufobj));
>  
> +        JS_ASSERT(bufobj->isArrayBuffer());
> +        ArrayBufferObject &buffer = bufobj->asArrayBuffer();

Presumably the as* asserts, so no need for the assert here?  (If there's some reason that |bufobj| isn't an ArrayBufferObject.)

@@ +1710,5 @@
>      }
>  
>    public:
>      static JSObject *
> +    createTypedArrayWithBuffer(JSContext *cx, JSObject *bufobj,

Seems like this should be ArrayBufferObject too.

@@ +3358,5 @@
>  JS_GetArrayBufferByteLengthUnchecked(JSObject *obj)
>  {
>      obj = UnwrapObject(obj);
>      JS_ASSERT(obj->isArrayBuffer());
> +    return obj->asArrayBuffer().byteLength();

The assert's redundant now, right?

@@ +3366,5 @@
>  JS_GetArrayBufferByteLength(JSContext *cx, JSObject *obj, uint32_t *byteLength)
>  {
>      obj = UnwrapObject(obj);
>      JS_ASSERT(obj->isArrayBuffer());
> +    *byteLength = obj->asArrayBuffer().byteLength();

Another redundant assert.

@@ +3375,5 @@
>  JS_GetArrayBufferData(JSContext *cx, JSObject *obj, uint8_t **data)
>  {
>      obj = UnwrapObject(obj);
>      JS_ASSERT(obj->isArrayBuffer());
> +    *data = obj->asArrayBuffer().dataPointer();

Another.

@@ +3384,5 @@
>  JS_GetArrayBufferDataUnchecked(JSObject *obj)
>  {
>      obj = UnwrapObject(obj);
>      JS_ASSERT(obj->isArrayBuffer());
> +    return obj->asArrayBuffer().dataPointer();

Another.

::: js/src/jstypedarray.h
@@ +175,5 @@
> +    inline uint32_t
> +    byteLength() const;
> +
> +    inline uint8_t *
> +    dataPointer() const;

I think we'd make these three declarations be one-liners.

@@ +182,5 @@
>       * Check if the arrayBuffer contains any data. This will return false for
>       * ArrayBuffer.prototype and neutered ArrayBuffers.
>       */
> +    inline bool
> +    hasData() const;

And this.
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2012-04-24 16:17:12 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/8535dc5b590a

...to make ArrayBufferObject be a class, consistently.  (We should privatize some methods/fields in that at some point, but for now I just smacked a public: at the top of it and let sleeping dragons lie.)
Comment 4 Steve Fink [:sfink] [:s:] 2012-04-24 17:03:38 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/a04734d243c8
Comment 5 Ed Morley [:emorley] 2012-04-25 02:02:11 PDT
(In reply to Jeff Walden [:Waldo] (busy, try to prefer other reviewers if possible) from comment #3)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/8535dc5b590a
> 
> ...to make ArrayBufferObject be a class, consistently.  (We should privatize
> some methods/fields in that at some point, but for now I just smacked a
> public: at the top of it and let sleeping dragons lie.)

Followup part backed out along with the rest of Waldo's push, for win debug bustage:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=718ced982de1

https://hg.mozilla.org/integration/mozilla-inbound/rev/cdb6904fa2cf
Comment 8 :Ehsan Akhgari 2012-04-25 07:48:20 PDT
Backed out: https://hg.mozilla.org/mozilla-central/rev/cdb6904fa2cf
Comment 9 Shane Caraveo (:mixedpuppy) 2012-06-07 16:26:21 PDT
I'm not sure if this bug is our issue or not, I got here following the dup/dependency tree.  Our original bug 734215 is duped to  741041 which is closed, but we still have a non-working UInt8Array problem in sandboxes.
Comment 10 Steve Fink [:sfink] [:s:] 2012-07-19 21:55:07 PDT
I can't figure out the change graph for this, but it's certainly in the tree now, so I'm going to resolve it.

(In reply to Shane Caraveo (:mixedpuppy) from comment #9)
> I'm not sure if this bug is our issue or not, I got here following the
> dup/dependency tree.  Our original bug 734215 is duped to  741041 which is
> closed, but we still have a non-working UInt8Array problem in sandboxes.

I'm guessing that your issue has now been resolved (at least on trunk; I still need to land one thing in Aurora since I missed the cutoff.) But if now, file a new bug; this bug isn't really the root cause of anything, and it's too messy already.

Note You need to log in before you can comment on or make changes to this bug.