Forked from bug 993768 comment 18. (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #18) > Steve, the current code in the bitrotted part looks like this: > > if (buffer->isMappedArrayBuffer()) > buffer->changeContents(cx, nullptr); > else if (newData != buffer->dataPointer()) > buffer->changeContents(cx, newData); > > Why isn't the mapped-array-buffer arm doing the same thing the other arm is > doing, in terms of allocating an equal-sized backing buffer to avoid the > out-of-bounds accesses that were an issue before? Because memory? If so, > we should back out (yet again, sigh) this XHR fix until we have the > neutering issue *actually* fixed, with no one using stale pointers or > lengths to do any indexing. In theory that's relatively soon -- patches to > fix all the typed array/arraybuffer methods are in bug 991981, with only > JSAPI users remaining to be fixed. So it wouldn't be so bad, I think. No good reason. This is wrong.

Still running this through try. Will request review once I get that green.

It would be good to have tests for this type of ArrayBuffer. I'm not sure that they really belong in this bug. The test can land before any of the other patches here, and doesn't need to be obscured.

This extends the test to hit the bug 982974 problem, and so probably shouldn't land until all of the tests can land (even though the exact feature being tested here is nightly only.)

Comment on attachment 8410383 [details] [diff] [review] Implement createMappedArrayBuffer for testing Review of attachment 8410383 [details] [diff] [review]: ----------------------------------------------------------------- r+, I guess, because the needed changes are pretty minimal, even if the mistakes are big. Or something. ::: js/src/shell/js.cpp @@ +715,5 @@ > + // I need a file at a known location, and the only good way I know of to do > + // that right now is to include it in the repo alongside the test script. > + // Bug 944164 would introduce an alternative. > + JSString *filenameStr = ResolvePath(cx, rawFilenameStr, ScriptRelative); > + JSAutoByteString filename(cx, filenameStr); Don't you need |if (!filename) return false;| or so here, and on fileNameStr too? @@ +755,5 @@ > + else > + size = 0; > + } > + > + JS_ASSERT(size >= offset); Erm. If I have createMappedArrayBuffer("/path/to/file.txt", 999999) and the file has zero length, doesn't this assertion fail? @@ +764,5 @@ > + } > + > + RootedObject obj(cx, JS_NewMappedArrayBufferWithContents(cx, size, contents)); > + if (!obj) > + return false; Does the failure case leak contents, or does this method free contents appropriately on failure? (We really need UniquePtr here so that we can pass a move reference in here and let that method steal/not steal as appropriate. Soon, I will return to bug 953296!) ::: js/src/tests/js1_8_5/extensions/file-mapped-arraybuffers.js @@ +6,5 @@ > +{ > + var s = ""; > + for (var c of view) > + s += String.fromCharCode(c); > + return s; Could also do |return String.fromCharCode.apply(null, view);|, for data that's this short. (Once you get to 500K+ data you start running out of stack space and throwing.) @@ +23,5 @@ > + var buffer3 = createMappedArrayBuffer(filename, 0, 8); > + view = new Uint8Array(buffer3); > + assertEq(viewToString(view), "01234567"); > +} > + Test could use more exercising of the error cases, particularly the assertion mistake.

Comment on attachment 8410385 [details] [diff] [review] imported patch FMAB-failing-test Review of attachment 8410385 [details] [diff] [review]: ----------------------------------------------------------------- Hm, I just r+'d a bitrotting of my own patches. Smooth.

Comment on attachment 8410382 [details] [diff] [review] Mapped array buffers need a safety buffer Review of attachment 8410382 [details] [diff] [review]: ----------------------------------------------------------------- I assume you want an r+ on this, now that that try run's done?

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #6) > Comment on attachment 8410383 [details] [diff] [review] > Implement createMappedArrayBuffer for testing > > Review of attachment 8410383 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+, I guess, because the needed changes are pretty minimal, even if the > mistakes are big. Or something. > > ::: js/src/shell/js.cpp > @@ +715,5 @@ > > + // I need a file at a known location, and the only good way I know of to do > > + // that right now is to include it in the repo alongside the test script. > > + // Bug 944164 would introduce an alternative. > > + JSString *filenameStr = ResolvePath(cx, rawFilenameStr, ScriptRelative); > > + JSAutoByteString filename(cx, filenameStr); > > Don't you need |if (!filename) return false;| or so here, and on fileNameStr > too? Yeah, sorry. Forgot to return to this after my "just get it to work!" phase. > @@ +755,5 @@ > > + else > > + size = 0; > > + } > > + > > + JS_ASSERT(size >= offset); > > Erm. If I have createMappedArrayBuffer("/path/to/file.txt", 999999) and the > file has zero length, doesn't this assertion fail? Ouch. I think I got tangled in the two meanings of size. (st.st_size is the size of the file, aka the size of the data available. size is supposed to be the size of the view you are creating.) I changed it to throw if you requested an offset past the end of the actual data. There's a little bit of a race condition (if the size changes between the stat and mmap calls), which from my testing results in either NULs after the actual data, an EADDR failure, or both, depending on the kernel's opinions about where you're putting the data. (Which is weird. I was testing with a write(1, addr, length), and the failure mode depended on whether stdout was a real file or /dev/null.) > @@ +764,5 @@ > > + } > > + > > + RootedObject obj(cx, JS_NewMappedArrayBufferWithContents(cx, size, contents)); > > + if (!obj) > > + return false; > > Does the failure case leak contents, or does this method free contents > appropriately on failure? (We really need UniquePtr here so that we can > pass a move reference in here and let that method steal/not steal as > appropriate. Soon, I will return to bug 953296!) Depends on when bug 995278 lands. With the current code, there are various double-frees and mismatched mmap/free pairs on failure. But yes, you're right, this should really free instead of depending on it getting freed on failure. That was my preferred direction rather than unconditionally taking ownership, which seems to have been borne out given what I've seen in the callers. (As in, there are currently more double-frees than leaks.) > ::: js/src/tests/js1_8_5/extensions/file-mapped-arraybuffers.js > @@ +6,5 @@ > > +{ > > + var s = ""; > > + for (var c of view) > > + s += String.fromCharCode(c); > > + return s; > > Could also do |return String.fromCharCode.apply(null, view);|, for data > that's this short. (Once you get to 500K+ data you start running out of > stack space and throwing.) Ooh, it takes a list. That's cleaner. > @@ +23,5 @@ > > + var buffer3 = createMappedArrayBuffer(filename, 0, 8); > > + view = new Uint8Array(buffer3); > > + assertEq(viewToString(view), "01234567"); > > +} > > + > > Test could use more exercising of the error cases, particularly the > assertion mistake. Ok.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #8) > I assume you want an r+ on this, now that that try run's done? Heh. Thanks. Though I can't really do much with it anytime soon. And I think you meant "r-, use both neuter variants."

This is only relevant since part 1 of bug 945152 landed, which is nightly only. This functionality is not yet called by anything but test code. Still, this bug should remain s-s since it points to the same flaw as bug 982974 (I could mark individual patches and comments to remove the problematic parts, but it doesn't seem worth it.) I'm going to land the fix and the safe test parts. I will not land the test that points out bug 982974.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f0714c1413b https://hg.mozilla.org/integration/mozilla-inbound/rev/b51cc5e640ec Marking [leave-open] for the remaining test.

https://hg.mozilla.org/mozilla-central/rev/2f0714c1413b https://hg.mozilla.org/mozilla-central/rev/b51cc5e640ec

Marking [qa-] for verification purposes. Feel free to add STR or other info if you feel you need QA to test this. Thank you.