ArrayBufferObjects can no longer store their data inline, so this API and ArrayBufferObject::ensureNonInline are no longer relevant. Moreover, the name of this function is pretty bad because array buffers aren't stable --- their data pointer can change through neutering and other operations.

(In reply to Brian Hackett (:bhackett) from comment #0) > ArrayBufferObjects can no longer store their data inline, so this API and > ArrayBufferObject::ensureNonInline are no longer relevant. Moreover, the > name of this function is pretty bad because array buffers aren't stable --- > their data pointer can change through neutering and other operations. Actually, this description is wrong. ArrayBufferObjects *can* store their data inline, but they also can't be nursery allocated, which is I think what this API was originally meant to guard against. So the data pointer can move during a compacting GC, but even when malloc'ed the data can be moved by things like neutering, so I still think this API should be removed.

Comment on attachment 8482372 [details] [diff] [review] patch Review of attachment 8482372 [details] [diff] [review]: ----------------------------------------------------------------- After seeing these, it's tempting to create an UnrootedArrayBufferContents RAII type. But it's probably not worth the bother, since it'd be to easy to accidentally avoid (since it needs to convert to a pointer.) Unless we wanted to go whole-hog: make it a handle and give it memcpy and memset methods, since that's just about all it's safe to do. ::: dom/workers/FileReaderSync.cpp @@ +75,5 @@ > return; > } > > uint32_t bufferLength = JS_GetArrayBufferByteLength(jsArrayBuffer); > + uint8_t* arrayBuffer = JS_GetArrayBufferData(jsArrayBuffer); I agree that JS_GetStableArrayBufferData is bad API. But just removing it seems kind of unsafe, since it's removing the only marker we have that a particular use is sensitive to the contents moving. This usage is a good example case, though easily fixable. GetInternalStream can GC. If that GC could be a compacting GC, arrayBuffer can be invalidated. There seems to be no reason whatsoever that the JS_GetArrayBufferData couldn't be moved down to just before stream->Read, though I'd need to read the body of Read to know if that's safe or not. The right fix to this code is to take advantage of the fact that ArrayBuffers can now hold foreign-malloced memory, and stop pre-creating the ArrayBuffer. ::: js/src/ctypes/CTypes.cpp @@ +2457,5 @@ > } else if (val.isObject() && JS_IsArrayBufferObject(valObj)) { > // Convert ArrayBuffer to pointer without any copy. > // Just as with C arrays, we make no effort to > // keep the ArrayBuffer alive. > + void* p = JS_GetArrayBufferData(valObj); This one is just unsafe no matter what. I think this should probably steal the data and neuter the ArrayBuffer. It's either that or implement pinning in the compacting GC. ::: js/src/jsapi-tests/testArrayBuffer.cpp @@ +43,5 @@ > JS_GetProperty(cx, view, "byteLength", &v); > CHECK_SAME(v, INT_TO_JSVAL(size)); > > // Modifying the underlying data should update the value returned through the view > + uint8_t *data = JS_GetArrayBufferData(obj); I haven't looked closely at this file, but I'm guessing this just shouldn't hang onto these pointers. ::: js/src/jsapi-tests/testTypedArrays.cpp @@ +36,5 @@ > RootedObject dummy(cx, JS_GetParent(proto)); > CHECK(!JS_IsArrayBufferObject(dummy)); > > CHECK_EQUAL(JS_GetArrayBufferByteLength(buffer), nbytes); > + memset(JS_GetArrayBufferData(buffer), 1, nbytes); This one seems like it shouldn't have been using Stable in the first place. @@ +101,1 @@ > memset(bufdata, 1, nbytes); This shouldn't store the pointer. Let it crash on a NULL deref. @@ +112,5 @@ > CHECK_EQUAL(JS_GetArrayBufferViewBuffer(cx, array), (JSObject*) buffer); > > Element *data; > CHECK(data = GetData(array)); > + CHECK(bufdata = JS_GetArrayBufferData(buffer)); This is fine, it never needed to use Stable. ::: netwerk/base/src/ArrayBufferInputStream.cpp @@ +38,5 @@ > > uint32_t buflen = JS_GetArrayBufferByteLength(arrayBuffer); > mOffset = std::min(buflen, aByteOffset); > mBufferLength = std::min(buflen - mOffset, aLength); > + mBuffer = JS_GetArrayBufferData(arrayBuffer); This class should not store mBuffer. It should store the original buffer. The mArrayBuffer field, which is a Value, already uses Maybe<PersistentRooted<Value>> to allow either stack or heap allocation. That would work for a JSObject* too, it seems to me. So I think the right fix is to eliminate mBuffer and change mArrayBuffer to Maybe<PersistentRooted<JSObject*>>. ::: xpcom/io/nsBinaryStream.cpp @@ +840,5 @@ > if (bufferLength < aLength) { > return NS_ERROR_FAILURE; > } > > + char* data = reinterpret_cast<char*>(JS_GetArrayBufferData(buffer)); This one should re-fetch the pointer every time it writes in some data.

(In reply to Steve Fink [:sfink] from comment #2) > I agree that JS_GetStableArrayBufferData is bad API. But just removing it > seems kind of unsafe, since it's removing the only marker we have that a > particular use is sensitive to the contents moving. Sorry, but I think you're off base. Right now, JS_GetStableArrayBufferData has no difference in observable behavior with JS_GetArrayBufferData, and the two are totally interchangeable. That might change with compacting GC, but compacting GC isn't here yet and when it is there needs to be a better mechanism in place for making sure people use array buffers correctly than crossing our fingers and hoping they use the right API. (The rooting analysis won't help with finding unrooted internal array buffer pointers.) What I think we need is an ArrayBuffer contents API similar to what we now do with strings. Have an RAII class named something like AutoStableArrayBufferData which (a) prevents the buffer from being collected, and (b) prevents its contents from being neutered or otherwise changed, i.e. do the things that JS_GetStableArrayBufferData ought to do but doesn't. Then we would change things so that it is impossible to get an array buffer's data in API clients without ensuring the pointer doesn't change: uint8_t * JS_GetArrayBufferData(HandleObject, AutoStableArrayBufferData &); Now, constructing AutoStableArrayBufferData would have a cost and if there was a hot use of JS_GetArrayBufferData (is there?) it would be nice to avoid this cost. So maybe have an AutoAssertStableArrayBufferData RAII class too which just asserts the buffer's contents don't change during its lifetime but has no release mode cost. If we have such a class though it should be used rarely, and the rooting analysis could understand it and make sure there are no calls which can GC or neuter() within its lifetime. This isn't perfect --- the pointer from JS_GetArrayBufferData can escape the lifetime of the AutoStableArrayBufferData, which seems like something that needs to be caught by reviewers as I don't see a good fix --- but it's a lot better than what we have now. But, this proposal is still not relevant to this bug, which is just about removing this bad JS_GetStableArrayBufferData API.

(In reply to Brian Hackett (:bhackett) from comment #3) > (In reply to Steve Fink [:sfink] from comment #2) > > I agree that JS_GetStableArrayBufferData is bad API. But just removing it > > seems kind of unsafe, since it's removing the only marker we have that a > > particular use is sensitive to the contents moving. > > Sorry, but I think you're off base. Right now, JS_GetStableArrayBufferData > has no difference in observable behavior with JS_GetArrayBufferData, and the > two are totally interchangeable. That might change with compacting GC, but > compacting GC isn't here yet and when it is there needs to be a better > mechanism in place for making sure people use array buffers correctly than > crossing our fingers and hoping they use the right API. (The rooting > analysis won't help with finding unrooted internal array buffer pointers.) Er... I think we're in violent agreement. I am not opposed to removing JS_GetStableArrayBufferData. It's a bad API. I suppose we could salvage it by making it permanently pin the ArrayBuffer (until a separate call releases it), but even that is way too dangerous without at least an RAII class. What I *am* opposed to is just removing it without fixing up the problems that it is highlighting. If you simply switch JS_GetStableArrayBufferData -> JS_GetArrayBufferData as in the patch, we'll end up finding these places again when debugging compacting GC. I understand that currently there is no behavioral difference (ever since ArrayBuffers ceased being nursery-allocatable, though I'm also thinking we might want to reverse that too). > What I think we need is an ArrayBuffer contents API similar to what we now > do with strings. Have an RAII class named something like > AutoStableArrayBufferData which (a) prevents the buffer from being > collected, and (b) prevents its contents from being neutered or otherwise > changed, i.e. do the things that JS_GetStableArrayBufferData ought to do but > doesn't. Then we would change things so that it is impossible to get an > array buffer's data in API clients without ensuring the pointer doesn't > change: > > uint8_t * > JS_GetArrayBufferData(HandleObject, AutoStableArrayBufferData &); > > Now, constructing AutoStableArrayBufferData would have a cost and if there > was a hot use of JS_GetArrayBufferData (is there?) it would be nice to avoid > this cost. So maybe have an AutoAssertStableArrayBufferData RAII class too > which just asserts the buffer's contents don't change during its lifetime > but has no release mode cost. If we have such a class though it should be > used rarely, and the rooting analysis could understand it and make sure > there are no calls which can GC or neuter() within its lifetime. Sure, I described some of these possibilities in my review. And I do think that we'll need something like this for safety. But after having gone through the patch, I feel like at least the existing users of JS_GetStableArrayBufferData all have fairly simple solutions (other than maybe the ctypes one), so we can fix them up and land the JS_GetStableArrayBufferData removal first and worry about a nicer mechanism afterwards. > This isn't perfect --- the pointer from JS_GetArrayBufferData can escape the > lifetime of the AutoStableArrayBufferData, which seems like something that > needs to be caught by reviewers as I don't see a good fix --- but it's a lot > better than what we have now. I'm pretty skeptical about this approach, even though I suggested it too. I think API users will see that they can get a pointer out, and will forget all about where the pointer came from. Right now, I'm leaning more towards a handle-based approach, where you do not get the pointer, you get an indirect pointer instead that will go through the ArrayBuffer to the current location of the data. I think that, plus some .memset and .memcpy methods on the handle, plus neutering when you want full read access to the data, plus passing in an external pointer when creating ArrayBuffers, should be enough. Or, to lay it out: write only (creating a new AB): use JS_NewArrayBufferWithContents(ptr) read only (consuming an AB): JS_StealArrayBufferContents read/write: ArrayBufferContentsHandle::{memset,memcpy,etc.} I'm sure *somebody* needs to do more than that, but hopefully we can just bloat up the Handle interface. > But, this proposal is still not relevant to > this bug, which is just about removing this bad JS_GetStableArrayBufferData > API. I hope I explained myself better above. Simply removing it means leaving compacting GC broken, and it's getting ready to land.

(In reply to Steve Fink [:sfink] from comment #4) > I hope I explained myself better above. Simply removing it means leaving > compacting GC broken, and it's getting ready to land. Ah, I didn't realize compacting GC was so close. I'm just going to WONTFIX this then. Even though this API needs to be removed the problems with the ArrayBuffer API run deep and removing this API can be done as part of a broader fix.

We can't just ignore this, as it's a blocker for compacting GC. I'll take it.

Posting current WIP. Many test failures still, and the ctypes stuff is very much up in the air.

I'm not sure I have everything cleared up quite yet, but I'll start requesting reviews anyway. I'll add a comment here to describe what is going on for later reviewers. The idea here is that this is dangerous: void* p = JS_GetArrayBufferData(obj); ...stuff... memcpy(..., p, size); // or memcpy(p, ..., size) because (1) compacting might move inlined data, invalidating 'p'; and (2) if any JS code runs in "...stuff...", it could neuter/detach the ArrayBuffer and invalidate 'p'. (1) is actually not a huge deal, because we have JS_GetStableArrayBufferData that moves the data out of line before returning it. But (2) *is* a big deal, a neverending source of security bugs, so solving it via JS_GetStableArrayBufferData is an antipattern. This patch removes that API entirely to avoid the attractive nuisance. Instead, you are now required to furnish proof while retrieving data pointers that nothing weird will happen while you're holding it. That is done by requiring a const JS::AutoCheckCannotGC& argument. Running JS code can always GC, so this covers both the compacting GC and neutering hazards. Note that this is easily subvertible: void *p; { JS::AutoCheckCannotGC nogc; p = JS_GetArrayBufferData(obj, nogc); } but at least this forces callers to be aware of the restriction, and is a bright red flag for reviewers.

This code held onto a data pointer for a long, long time. Now that we can construct ArrayBufferObjects out of any old malloced data pointer, it's better to construct the data and then create an ABO at the very end.

A variety of fixes for smaug review. The weirdest one is nsDOMFileReader.cpp one because it initializes an AutoJSAPI in a very odd way. It should be commented, but I don't understand it well enough to comment. Olli does, however. :-)

Mostly straightforward -- delay fetching the data until a point at which we can prove we won't GC. This one actually required an analysis annotation from back in the first patch in this series, because of some GL function pointer. Note that the analysis suspects that readback->GetData() can GC, which is why the code restructuring is a little weird.

I was a little surprised that mccr8 was marked as the reviewer for this one, but what do I know? Mostly this just juggles things around to grab out the data and copy it away before GC stuff can run. It doesn't add any new copies to the ones already there. The first change to TCPSocketChild.cpp is a little weird, though -- I see nothing that needs to GC there. But maybe it's better to stick to the external data pattern when possible.

Finally, a simple one. Stop holding an unreliable pointer and switch to building an ABO out of external data instead.

Ugh. This one stinks. It totally looks like it's subverting the safety mechanism. It only works because UnixSocketRawData's constructor does an immediate memcpy.

Comment on attachment 8499218 [details] [diff] [review] ABO safety fixes for TCP sockets The file moved recently, and I've just reviewed a few tiny fixes since that happened. I don't actually know anything about this code. Maybe jdm can review, or at least figure out who can review it.

Re-fetch the data pointer on every iteration. This one is only fixing the inline data case and would actually be a valid use of JS_GetStableArrayBufferData, since it's already guarded against neutering. But this way is safer.

Of the good, the bad, and the ugly, this qualifies for the latter two. So we really want IO.File to be able to read/write ArrayBuffers (and typed arrays) with zero copying. But the IO.File code extracts a data pointer and reads/writes using it, running all kinds of JS code that could conceivably neuter the buffer. I've been racking my brains to figure out the best approach here, and ended up just kind of picking the one that felt least wrong. I initially thought that I could change ctypes to neuter ArrayBuffers when it converted them to char*. It would make them much less useful, but at least they'd be safe and if you're just going to write the data to a file descriptor, then the caller probably doesn't need to use the AB contents afterwards anyway. There are two problems with that. First, ownership is an issue. The stolen pointer needs to be freed, and unless you're doing the conversion as part of a ctypes function call, I don't know of a good way to make sure the pointer gets freed. Second, I ran into callers that didn't want their data to go away (though in retrospect, the one I ran into could have been handled differently.) The ownership problem would exist even if we were willing to pay a memcpy. Perhaps jorendorff knows whether CData or something could take ownership? I didn't see a straightforward way -- something close, but not quite there. So what I ended up doing is saying that you can convert ABs and TAs to char*, but only when calling a ctypes function. I figured that such a function can already mess you up by just freeing the pointer you give it or something, so calling back from C to JS is just a Don't Do That thing. This still required changes to OS.File, because it expected to be able to convert to a char* and pass the pointer around. An alternative I was considering was exposing functions to write out portions of ABs. read(fd, AB, offset, size) and write(fd, AB, offset, size) or something. But that splits platform-dependent code between OS.File and Components.utils, and makes separate paths for IO on ABs vs other sources of data, all of which seems pretty messy. Reviewers' opinions may vary. I may be missing a reason why this scheme won't work. We may end up having to add a flag to ABs to label them as immobile and un-neuterable. It would be unfortunate if ctypes were the only reason for that, though.

Waldo, I'd feel safer if you took a quick look at these.

Yoric, jorendorff: Note that since this undoes most of bug 795505, I would need to remove the tests in that bug too. Although if there's an easy way to call memcmp from that test, I would prefer to switch all the tests to doing that.

Comment on attachment 8499234 [details] [diff] [review] ABO safety non-fix for ctypes Review of attachment 8499234 [details] [diff] [review]: ----------------------------------------------------------------- I am quite fine with the idea of converting ArrayBuffer to C pointer only for calls to C functions. However, the OS.File patch here introduces copies (of possibly Mb-long buffers, on the main thread), so it cannot be used as such. I have filed bug 1077354 to handle the refactoring required for OS.File. I will try and work on it soon. Do you have an ETA?

Comment on attachment 8499208 [details] [diff] [review] Make it harder for ArrayBuffer data pointers to be held across invalidations >+ case FILE_AS_ARRAYBUFFER: { >+ AutoJSAPI jsapi; >+ if (NS_WARN_IF(!jsapi.Init(this->mozilla::DOMEventTargetHelper::GetParentObject()))) { Why you need this-> here? >- if (mDataFormat == FILE_AS_ARRAYBUFFER) { >- RootResultArrayBuffer(); >- mResultArrayBuffer = JS_NewArrayBuffer(aCx, mTotal); >- if (!mResultArrayBuffer) { >- NS_WARNING("Failed to create JS array buffer"); >- aRv.Throw(NS_ERROR_FAILURE); >- } >- } So the old implementation just creates right size arraybuffer and reads the data to it. No extra malloc churn. You end up relying on mFileData which is created using moz_realloc, so we possibly end up calling it quite a few times. I'd prefer to not lose the optimization for arraybuffer. >+ for (uint32_t i = 0; i < aDataLen; i++) { >+ frames[i] = aData[i]; >+ } (Hmm, you just moved this code, but why it isn't using memcpy. No need to fix in this bug) >+nsSpeechTask::SendAudioImpl(nsRefPtr<mozilla::SharedBuffer>& samples, uint32_t aDataLen) aSamples

Comment on attachment 8499226 [details] [diff] [review] ABO safety fixes for nsBinaryStream I'm not the right person to review this. hg log says waldo should look this over.

(In reply to Olli Pettay [:smaug] from comment #21) > Comment on attachment 8499208 [details] [diff] [review] > Make it harder for ArrayBuffer data pointers to be held across invalidations > > >+ case FILE_AS_ARRAYBUFFER: { > >+ AutoJSAPI jsapi; > >+ if (NS_WARN_IF(!jsapi.Init(this->mozilla::DOMEventTargetHelper::GetParentObject()))) { > Why you need this-> here? I just liked the full explicitness when specifying an alternate base class's implementation. But it's no big deal. I removed it. > >- if (mDataFormat == FILE_AS_ARRAYBUFFER) { > >- RootResultArrayBuffer(); > >- mResultArrayBuffer = JS_NewArrayBuffer(aCx, mTotal); > >- if (!mResultArrayBuffer) { > >- NS_WARNING("Failed to create JS array buffer"); > >- aRv.Throw(NS_ERROR_FAILURE); > >- } > >- } > So the old implementation just creates right size arraybuffer and reads the > data to it. No extra malloc churn. > You end up relying on mFileData which is created using moz_realloc, so we > possibly end up calling it quite a few times. > I'd prefer to not lose the optimization for arraybuffer. Ok, fixed. But why is this optimization only used for ArrayBuffer? FILE_AS_BINARY looks like it could preallocate mTotal*2 bytes. Even FILE_AS_TEXT could optimistically preallocate (assuming ASCII), reallocing only if needed. > >+ for (uint32_t i = 0; i < aDataLen; i++) { > >+ frames[i] = aData[i]; > >+ } > (Hmm, you just moved this code, but why it isn't using memcpy. No need to > fix in this bug) I noticed that, and stopped myself from changing it.

FILE_AS_BINARY is a deprecated thing, and FILE_AS_TEXT... well, it might be worth to preallocate. But that fix should happen in a different bug.

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #20) > I am quite fine with the idea of converting ArrayBuffer to C pointer only > for calls to C functions. > However, the OS.File patch here introduces copies (of possibly Mb-long > buffers, on the main thread), so it cannot be used as such. I have filed bug > 1077354 to handle the refactoring required for OS.File. I will try and work > on it soon. Do you have an ETA? Where does it introduce a copy? It shouldn't; that was a large part of the reason for choosing this approach. But maybe I'm overlooking something.

At the moment, for historical reasons, we use toMsg() to convert `ArrayBuffer` instances into `void*` before sending them from the main thread to the worker. This avoids a copy. Now that `postMessage` implements transferable `ArrayBuffer`, we need to fix OS.File to take advantage of this instead. Not a huge fix, but it needs to tweak a few hard-to-discover places.

Comment on attachment 8499224 [details] [diff] [review] ABO safety fixes for Ril (I don't think cjones does reviewing too often these days)

I literally just mid-aired you saying, > Hi, sorry, I don't feel comfortable reviewing this diff. If I r+'d JSAPI code before, it's because I buddy-reviewed it with someone. It's better for both of us if someone else looks this over ;) (recommend mrbkap or bent). So nm!

Comment on attachment 8499686 [details] [diff] [review] Make it harder for ArrayBuffer data pointers to be held across invalidations >+ if (mDataFormat != FILE_AS_ARRAYBUFFER) { >+ mFileData = (char *) moz_realloc(mFileData, mDataLen + aCount); >+ NS_ENSURE_TRUE(mFileData, NS_ERROR_OUT_OF_MEMORY); >+ } I guess we don't need the if (mDataFormat != FILE_AS_ARRAYBUFFER) since moz_realloc shouldn't just do anything. >+nsSpeechTask::SendAudioImpl(nsRefPtr<mozilla::SharedBuffer>& samples, uint32_t aDataLen) aSamples >- void SendAudioImpl(int16_t* aData, uint32_t aDataLen); >+ void SendAudioImpl(nsRefPtr<mozilla::SharedBuffer>& samples, uint32_t aDataLen); ditto

(In reply to Olli Pettay [:smaug] from comment #30) > Comment on attachment 8499686 [details] [diff] [review] > Make it harder for ArrayBuffer data pointers to be held across invalidations > > >+ if (mDataFormat != FILE_AS_ARRAYBUFFER) { > >+ mFileData = (char *) moz_realloc(mFileData, mDataLen + aCount); > >+ NS_ENSURE_TRUE(mFileData, NS_ERROR_OUT_OF_MEMORY); > >+ } > I guess we don't need the if (mDataFormat != FILE_AS_ARRAYBUFFER) > since moz_realloc shouldn't just do anything. Wouldn't that shrink the allocation if DoReadData received part of the file? > >+nsSpeechTask::SendAudioImpl(nsRefPtr<mozilla::SharedBuffer>& samples, uint32_t aDataLen) > aSamples > > >- void SendAudioImpl(int16_t* aData, uint32_t aDataLen); > >+ void SendAudioImpl(nsRefPtr<mozilla::SharedBuffer>& samples, uint32_t aDataLen); > ditto Ah, right, sorry.

Silly me. that if-check for realloc is needed as you pointed on irc.

Ok, some important updates for jorendorff's review. Note that this patch is not valid without Yoric's IO.File Transferable stuff. Still 2 errors remaining -- a bizarre Windows compilation error, and an M-4 timeout that I haven't managed to reproduce. Ooh! And a new M-oth failure. Ugh. https://tbpl.mozilla.org/?tree=Try&rev=59bfa69cf3f7

Comment on attachment 8499207 [details] [diff] [review] ABO safety changes for jdm review Review of attachment 8499207 [details] [diff] [review]: ----------------------------------------------------------------- This was fun. Let's do it again sometime. ::: netwerk/base/src/ArrayBufferInputStream.cpp @@ +90,3 @@ > > + char buffer[8192]; > + uint32_t count = std::min(std::min(aCount, remaining), uint32_t(sizeof(buffer))); mozilla::ArrayLength instead? @@ +90,4 @@ > > + char buffer[8192]; > + uint32_t count = std::min(std::min(aCount, remaining), uint32_t(sizeof(buffer))); > + if (count == 0) {} please. @@ +95,5 @@ > + char* src; > + { > + JS::AutoCheckCannotGC nogc; > + src = (char*) JS_GetArrayBufferData(mArrayBuffer->get(), nogc) + mOffset + mPos; > + memcpy(buffer, src, count); We copy src into buffer but later copy from src. Something is wrong here. @@ +101,5 @@ > > + // It is just barely possible that writer() will detach the ArrayBuffer's > + // data, setting its length to zero. Or move the data to a different memory > + // area. (This would only happen in a subclass that passed something other > + // than NS_CopySegmentToBuffer as 'writer'). This comment feels unfinished, in that it describes a problem, but the code underneath it doesn't clearly relate to or solve said problem.

(In reply to Josh Matthews [:jdm] from comment #34) > @@ +95,5 @@ > > + char* src; > > + { > > + JS::AutoCheckCannotGC nogc; > > + src = (char*) JS_GetArrayBufferData(mArrayBuffer->get(), nogc) + mOffset + mPos; > > + memcpy(buffer, src, count); > > We copy src into buffer but later copy from src. Something is wrong here. Er, uh, yeah. Good point. :-) > @@ +101,5 @@ > > > > + // It is just barely possible that writer() will detach the ArrayBuffer's > > + // data, setting its length to zero. Or move the data to a different memory > > + // area. (This would only happen in a subclass that passed something other > > + // than NS_CopySegmentToBuffer as 'writer'). > > This comment feels unfinished, in that it describes a problem, but the code > underneath it doesn't clearly relate to or solve said problem. Well, it would have made a little more sense if the code underneath it were actually correct (as in, copying from buffer instead of src). But I moved and expanded the comment.

Comment on attachment 8499218 [details] [diff] [review] ABO safety fixes for TCP sockets Review of attachment 8499218 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/network/TCPSocketChild.cpp @@ +28,5 @@ > mozilla::AutoSafeJSContext cx; > JSAutoCompartment ac(cx, aObj); > > + mozilla::UniquePtr<uint8_t> data((uint8_t*) malloc(aBuffer.Length())); > + if (!data) I'm 99% sure that malloc is infallible. Double check with glandium.

Comment on attachment 8500667 [details] [diff] [review] ABO safety fixes for ArrayBufferInputStream Review of attachment 8500667 [details] [diff] [review]: ----------------------------------------------------------------- Much better.

Comment on attachment 8499222 [details] [diff] [review] ABO safety fixes for FileReaderSync Review of attachment 8499222 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/FileReaderSync.cpp @@ +90,2 @@ > > + JSObject *arrayBuffer = JS_NewArrayBufferWithContents(aCx, blobSize, bufferData.get()); stars on the left is general Gecko style. @@ +93,5 @@ > + aRv.Throw(NS_ERROR_OUT_OF_MEMORY); > + return; > + } > + > + bufferData.release(); I assume this transfers ownership, rather than copying.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #38) > @@ +93,5 @@ > > + aRv.Throw(NS_ERROR_OUT_OF_MEMORY); > > + return; > > + } > > + > > + bufferData.release(); > > I assume this transfers ownership, rather than copying. Yes. I should perhaps change these APIs to accept UniquePtrs so I don't have to do this dance. But I don't want any more risk here.

Comment on attachment 8499218 [details] [diff] [review] ABO safety fixes for TCP sockets Review of attachment 8499218 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/network/TCPSocketChild.cpp @@ +27,5 @@ > { > mozilla::AutoSafeJSContext cx; > JSAutoCompartment ac(cx, aObj); > > + mozilla::UniquePtr<uint8_t> data((uint8_t*) malloc(aBuffer.Length())); This is an allocator mismatch: pairs a malloc with a delete. You want auto data = MakeUnique<uint8_t[]>(aBuffer.Length()); instead. (Technically MakeUnique<T[]> zeroes out the array it creates. But I would be rather astonished if any compiler we use, actually zeroed out the array, when immediately after this we memcpy over the entire array.) @@ +37,3 @@ > if (!obj) > + return false; > + data.release(); We should add an adopting ArrayBuffer creation method at some point, that could take a UniquePtr directly. ::: dom/network/TCPSocketParent.cpp @@ +240,5 @@ > JS::Rooted<JSObject *> obj(aCx, &aDataVal.toObject()); > if (JS_IsArrayBufferObject(obj)) { > + FallibleTArray<uint8_t> fallibleArr; > + uint32_t errLine = 0; > + do { Unusual (if justified) structure here.

Comment on attachment 8499218 [details] [diff] [review] ABO safety fixes for TCP sockets Review of attachment 8499218 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/network/TCPSocketChild.cpp @@ +27,5 @@ > { > mozilla::AutoSafeJSContext cx; > JSAutoCompartment ac(cx, aObj); > > + mozilla::UniquePtr<uint8_t> data((uint8_t*) malloc(aBuffer.Length())); ...wait. This is still an allocator mismatch. The contents of an ArrayBuffer are managed using the JS heap, i.e. js_malloc and js_free. This is not guaranteed to be the same thing as malloc. And indeed, this is exactly the sort of thing we've hit when trying to make the JS allocator use a different heap from the main one, for security purposes, somewhere or other recently. So you *really* want: mozilla::UniquePtr<uint8_t[], JS::FreePolicy> data(js_pod_malloc<uint8_t>(aBuffer.Length()));

Comment on attachment 8499222 [details] [diff] [review] ABO safety fixes for FileReaderSync Review of attachment 8499222 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/FileReaderSync.cpp @@ +73,5 @@ > aRv.Throw(rv); > return; > } > > + UniquePtr<char> bufferData((char*) malloc(blobSize)); UniquePtr<char[], JS::FreePolicy> bufferData(js_pod_malloc<char>(blobSize)); @@ +85,5 @@ > if (NS_FAILED(rv)) { > aRv.Throw(rv); > return; > } > + NS_ASSERTION(numRead == blobSize, "failed to read data"); In passing I note it's very dodgy assuming reading N bytes from a stream will read all N bytes, unless the exact implementation of the stream is known. I assume we actually do know this here, but I didn't check.

Comment on attachment 8499678 [details] [diff] [review] Make the compiler check for holding ABO data when it could move Review of attachment 8499678 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webaudio/AudioBuffer.cpp @@ +116,5 @@ > if (!array) { > return false; > } > + JS::AutoCheckCannotGC nogc; > + memcpy(JS_GetFloat32ArrayData(array, nogc), data, sizeof(float)*mLength); mozilla::PodCopy(JS_GetFloat32ArrayData(array, nogc), data, mLength); ::: js/src/jsapi-tests/testTypedArrays.cpp @@ +161,5 @@ > CHECK_SAME(v, v2); > + { > + JS::AutoCheckCannotGC nogc; > + Element *data; > + CHECK(data = GetData(array, nogc)); Soooooooo much repetition of this.

I'm splitting off the ctypes stuff into a separate bug, since everything else is landable and the ctypes stuff depends on IO.File changes. And the ctypes stuff is at this point only for detached buffers, and chrome code just shouldn't do that. (ctypes is only exposed to chrome.)