I ran into this while cleaning up the mapped array buffer implementation for another patch. When we detach an ArrayBufferObject we set its byteLength() to 0 [1]. When we do this to a mapped ABO and then go to free it, we run into problems as we no longer have the allocated length [2]. On Windows this is fine as the length isn't used, but passing a length of 0 to munmap probably means we leak the mapping (munmap doesn't seem to be failing, apparently it doesn't care). I fixed this by simply exempting mapped ABOs from having their size cleared. This doesn't seem to cause any problems, but I don't know if it's the right solution or if it could lead to us trying to free the same object twice. [1] https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/js/src/vm/ArrayBufferObject.cpp#541 [2] https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/js/src/vm/ArrayBufferObject.cpp#1011

Comment on attachment 9020492 [details] [diff] [review] Keep the byte length when detaching mapped array buffers so we can free them properly. Review of attachment 9020492 [details] [diff] [review]: ----------------------------------------------------------------- Do you have a scenario where this happens? What I'm seeing is that if you have a mapped arraybuffer, then it's either a wasm buffer (which cannot be detached) or some other mapped buffer like that created by the test function createMappedArrayBuffer. For the latter, when you detach the ABO it does the munmap on the old contents, then switches it to hold a zero-length regular memory allocation (so the ABO's changes from BufferKind::MAPPED to BufferKind::PLAIN). I couldn't get it to give me an munmap system call with length 0. If there is such a situation, we'll have to do some more thinking, because this patch would change observable semantics: after detaching, buffer.byteLength should return 0. https://tc39.github.io/ecma262/#sec-detacharraybuffer is where it sets the byteLength to 0, though really I should also find the part where it says that the byteLength accessor won't throw on a detached buffer; I think it may have gone back and forth.

This ABO code is all still pretty new to me, so bear with me. For some reason I can only get this to fail on Windows! What seems to happen is that we call JS_StealArrayBufferContents() from [1]. This detaches the buffer, setting its byteLength() to 0 but does *not* free the old data. I think this is supposed to happen at [2], but we don't hit that line because ArrayBufferObject::stealContents() passes the same buffer for both arguments [3] (is that a bug? Should we be passing contentsCopy instead of oldContents?). Then finally we hit the GC at [4], which tries to free the object with byteLength() set to 0 (during background finalization). I don't understand why this only happens on Windows though. [1] https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/js/src/jsapi-tests/testMappedArrayBuffer.cpp#153 [2] https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/js/src/vm/ArrayBufferObject.cpp#538 [3] https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/js/src/vm/ArrayBufferObject.cpp#1399 [4] https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/js/src/jsapi-tests/testMappedArrayBuffer.cpp#190

Ugh, ok. The idea here probably makes the most sense when stealing AB contents. If it's regular malloc memory, you want to return the pointer held in the ABO and give the ABO an unowned null pointer. (It could be an owned null pointer too, I suppose. It doesn't matter.) Some ABOs cannot have their contents changed, for various reasons. For those, we still allow stealing their contents, but now we make a copy of the data and return that. The original ABO hangs onto its pointer but sets its byteLength to zero. That seems to be the path you're seeing? The two cases are managed by passing in a newContents parameter to detach(), which says what the donor ABO should end up holding. For the mapped case I was looking at on Linux, we follow the first path -- a mapped array buffer is regarded as having stealable contents, so it gets transferred to the new owner and eventually freed in its finalizer (with the correct length.) But I see you're running a jsapi-test testMappedArrayBuffer, let me try that... ooh, that return false! And tracing through, it seems to have an unaligned pointer 0x7fd0c78cc008 and it has zeroed out the length, so it ends up calling munmap(0x7fd0c78cc000, 8). Which strangely does not match what I see with strace, where the smallest munmap argument is 12. At any rate, the behavior with this jsapi-test is different from eg: { var bbb = createMappedArrayBuffer("/etc/passwd"); print(bbb.byteLength); //detachArrayBuffer(bbb); s = serialize(bbb, [bbb]); print(bbb.byteLength); bbb = null; gc(); } print("Bye!"); This makes a mapped buffer with hasStealableContents() == true. I guess I need to debug -- you're right, we're losing information here, and it's reachable through the JSAPI.

(But my current thought is that at the point of detaching -- when byteLength is set to zero -- we should be munmapping. But that feels dangerous; I'm not sure why some paths are !hasStealableContents and whether they'd break if we munmap.)

> And tracing through, it seems to have an unaligned pointer 0x7fd0c78cc008 and it has zeroed out the length, so it ends up calling munmap(0x7fd0c78cc000, 8). Aah, right of course, so it doesn't call munmap with a value of 0 because the actual underlying buffer is page-aligned, and the pointer we were using was offset 8 bytes into that buffer. FWIW, the length parameter in this test is set to 12, so the total size is 20 bytes (though we allocate an entire page for it). It's curious that the value of hasStealableContents changes based on the path, I didn't think to check that at all. > (But my current thought is that at the point of detaching -- when byteLength is set to zero -- we should be munmapping. But that feels dangerous; I'm not sure why some paths are !hasStealableContents and whether they'd break if we munmap.) I guess there's 3 options: 1) Allow the buffer to be stolen and set the pointer of the original to nullptr along with its length; in this case the new holder would have to ensure that the buffer is freed the correct way (since on Windows we don't use VirtualFree but UnmapViewOfFile - maybe that's why it's not stealable on Windows?). 2) Copy the data and free the original buffer. 3) Copy the data and leave the original buffer intact. Then both objects would still work, but it wouldn't really be 'stealing' anymore. Options 1 and 2 would make the original object unusable - does that risk trying to access freed memory or dereferencing a null pointer?

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #6) > (since on Windows we don't use VirtualFree but UnmapViewOfFile - maybe that's why it's not stealable on Windows?) Oh, but either way you'd have to take care to remove the offset before freeing the buffer. So that's not really a reason to make it not stealable unless you just blanket mask out the lower bits of every buffer before freeing it.

Ok, JS_StealArrayBufferContents is explicit about this: https://searchfox.org/mozilla-central/source/js/src/vm/ArrayBufferObject.cpp#1869-1872 It's a limitation of an interface that only return a pointer to memory with no indication of what the recipient should do with it to free it. Not a good API now that mapped array buffers exist. I thought this was just a performance problem, in that we're creating a copy, but this is also a leak. For this API, we should be making a copy and then discarding the original completely, instead of just forgetting something important about it. I'm starting to think that we should distinguish "can't steal" from "can't release". Now I just need to re-figure out what the "can't steal" cases are. Again. (Weirdness here is usually asm.js or wasm related, or SharedArrayBuffers.)

(I had life happening in between, so my comment was made before reading either of your last two.) I don't think it's path or Windows related, it's just the API that insists on returning a pointer and nothing else, nothing to say how to dispose of it when done. The big question is why we can't pass in BufferContents::createPlain(nullptr) in place of oldContents at https://searchfox.org/mozilla-central/source/js/src/vm/ArrayBufferObject.cpp#1394 and I haven't figured that out or dug it out of my memory. (This would be your option #2.)

Something like this? I tried passing BufferContents::createPlain(nullptr) for every kind of object and got crashes, but I think this is probably the right solution for mapped buffers.

Comment on attachment 9020975 [details] [diff] [review] Allow mapped array buffer to be freed when its contents are stolen. Review of attachment 9020975 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, I didn't get to looking at this in detail. Looks like I'll need to redirect to Waldo while I'm on PTO.