1037358 - Virtual memory leakage from array buffer transfer

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Shian-Yow Wu [:swu]]

One way to reproduced is to repeatedly read array buffer by XHR in a worker.
If we check the "heap-mapped" in about:memory, it keeps increasing and never get decreased.

This can eventually exhaust the VM address space.

Comment 1

[Shian-Yow Wu [:swu]]

Attachment: Test case

This is a test case to reproduce.

Comment 2

[Benjamin Smedberg]

Not a security bug. Are you sure this is a JS bug and not a worker bug?

Comment 3

[Shian-Yow Wu [:swu]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> Not a security bug. Are you sure this is a JS bug and not a worker bug?

To my current knowledge, array buffer handling code in JS contributes to the number of "heap-mapped" when doing such test.  Without further analysis, I am not sure whether it could come from other components, so marked it to JavaScript:GC that best fits for now.

Comment 4

[Jeff Walden [:Waldo]]

Given the particular JAR opened, this looks a lot like a probable dup of bug 1023585.

Comment 5

[Steve Fink [:sfink] [:s:]]

Hm, I'm seeing ambiguous results here. Doing a ps shows a gradual climb during the main run, followed by a drop, ending up at generally a bit higher than it started. Dumping about:memory reports, I sometimes see a net *decline* in the vmsize from the start of the run to the end of it, and sometimes a modest climb.

My latest run:

first dump during the test: 1,223.71 MB ── vsize
second mid-test dump: 1,315.52 MB ── vsize
third mid-test dump: 1,384.88 MB ── vsize
after the test finishes: 1,275.31 MB ── vsize
after letting it idle for a few minutes: 1,283.50 MB ── vsize

What do you see? I'll kick off a run with lots more iterations.

Comment 6

[Steve Fink [:sfink] [:s:]]

You're right. I ran out of memory at count=64671.

Comment 7

[Shian-Yow Wu [:swu]]

I probably got the idea of the root cause, but I don't have the environment to try it for the time being.

In currently m-c code, during an array buffer object transfer, a safety buffer is allocated in ArrayBufferObject::stealContents() before the object been neutered, and such buffer consumes VM space.  The safety buffer is expected to be freed by the finalizer when the object been GCed.

For memory-mapped array buffer, instead of freeing the buffer, its finalizer in ArrayBufferObject::releaseData() unmaps the memory-mapped pointer, which leaves the safety buffer unfreed.

If that's the case, my proposed solution is:
When a safety buffer is exchanged with the previously memory-mapped pointer of a mapped array buffer, we should make it a normal array buffer object by removing the MAPPED_BUFFER flag, then the safety buffer can be freed by the default FreeOp.

Does this make sense?

Comment 8

[Shian-Yow Wu [:swu]]

Attachment: Patch: Remove the MAPPED_BUFFER flag of the array buffer object if its data pointer is no longer memory-mapped.

The patch realized the proposed solution in comment 7.

Comment 9

[Shian-Yow Wu [:swu]]

This is the about:memory result that tested with non-mapped/mapped array buffer transfer, with/without the patch of comment 8.  The result is collected by clicking "Minimize memory usage" followed by "Measure" in about:memory.

1. Non-mapped array buffer transfer: (dom.mapped_arraybuffer.enabled=false)

Without patch:

  Initial:
    105.78 MB (100.0%) -- explicit
    99.00 MB ── heap-mapped
    1,001.09 MB ── vsize
  After 10000 iterations:
    122.54 MB (100.0%) -- explicit
    259.00 MB ── heap-mapped
    1,152.45 MB ── vsize
  After 20000 iterations:
    141.40 MB (100.0%) -- explicit
    261.00 MB ── heap-mapped
    1,237.99 MB ── vsize

With patch:

  Initial:
    105.26 MB (100.0%) -- explicit
    109.00 MB ── heap-mapped
    1,006.96 MB ── vsize
  After 10000 iterations:
    126.14 MB (100.0%) -- explicit
    265.00 MB ── heap-mapped
    1,160.20 MB ── vsize
  After 20000 iterations:
    144.40 MB (100.0%) -- explicit
    277.00 MB ── heap-mapped
    1,253.87 MB ── vsize

2. Mapped array buffer transfer: (dom.mapped_arraybuffer.enabled=true)

Without patch:

  Initial:
    102.32 MB (100.0%) -- explicit
    82.00 MB ── heap-mapped
    970.07 MB ── vsize
  After 10000 iterations:
    249.29 MB (100.0%) -- explicit
    226.00 MB ── heap-mapped
    1,120.75 MB ── vsize
  After 20000 iterations:
    383.35 MB (100.0%) -- explicit
    365.00 MB ── heap-mapped
    1,342.95 MB ── vsize

With patch:

  Initial:
    115.77 MB (100.0%) -- explicit
    80.00 MB ── heap-mapped
    983.53 MB ── vsize
  After 10000 iterations:
    121.50 MB (100.0%) -- explicit
    152.00 MB ── heap-mapped
    1,047.08 MB ── vsize
  After 20000 iterations:
    152.47 MB (100.0%) -- explicit
    196.00 MB ── heap-mapped
    1,173.11 MB ── vsize


Note:
The result shows that the major memory leakage in mapped array buffer transfer can be fixed by the patch, but it seems there are still other kinds of memory leakage.

Comment 10

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8455168 [details] [diff] [review]
Patch: Remove the MAPPED_BUFFER flag of the array buffer object if its data pointer is no longer memory-mapped.

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

Oh, nice catch. For some reason, I was assuming that it already worked this way.

But I think you should make it a little harder to make this mistake, especially since I think we'll be seeing new types of ArrayBuffers before long. It just seems dangerous to have a setNewOwnedData that doesn't know what kind of data it is accepting, which means the caller is responsible for fixing up a possibly invalid temporary buffer.

Can you add a parameter to setNewOwnedData that describes the data that is being owned? The simple way would be to pass in ArrayBufferFlags::MAPPED_BUFFER or not and use it to set just that flag bit (or perhaps any of the *_BUFFER bits?). The other way, which I'm not sure how would work out, would be to eliminate the OwnsState enum and replace its use everywhere with a full ArrayBufferFlags value (asserting that only the relevant bits are set.) That makes more sense if you say that knowing whether a pointer is owned or not isn't meaningful without knowing *how* it is owned (as in, what you need to do to release the data.) But it might be weird in places, because really owning vs not owning *is* meaningful in its own right (it just says whether the finalizer needs to release the data.)

I'll be fine with either solution, I just don't like the setup where the buffer is temporarily invalid after a setNewOwnedData call.

Comment 11

[Shian-Yow Wu [:swu]]

Attachment: Patch: Pass the flag in setDataPointer() to indicate whether data is memory-mapped.

The OwnsState decides whether to release the data, while the ArrayBufferFlags decides how to release the data.  Therefore we should keep the OwnsState.

To pass a flag to indidate the new data, not only setNewOwnedData(), but also other functions all the way like neuter(), changeContents(), and setDataPointer() needs such indication.  Doing such makes it clear but also makes code more complex.

Do you think we should add it to all these functions?  Or how about we just pass it in setDataPointer() as in this patch, and assume that no flag passed in other functions means the new data is not memory-mapped?

Comment 12

[Steve Fink [:sfink] [:s:]]

Attachment: Pass the flag in setDataPointer() to indicate whether data is memory-mapped

I wanted to check this out, so I tried patching it with a full BufferKind enum (a subset of ArrayBufferFlags) and writing a test. The test turned out to be kind of nasty to write, but it shows continuing leakage. I'm not sure if that means I messed up the patch, or if this is the remaining leak that you detected.

I'll try splitting up the patch and running the test without the fix to compare the leak rate. I don't have time to do more today. I'm posting the combined thing here first.

Comment 13

[Steve Fink [:sfink] [:s:]]

Er, wow. This is kind of weird.

If I change the test to map in a 10MB file instead, then the vmsize doesn't grow at all. (Well, it grows 68KB from the beginning to the end of the run.) If I map in a 20-byte file, which I would expect to get rounded up to one 4KB page, then the vmsize just grows and grows, and never goes back down.

I'm not sure if this means it's causing massive fragmentation somehow, or...?

Comment 14

[Steve Fink [:sfink] [:s:]]

If I scan a leaking log's strace output, just about all of the mmaps are paired up with munmaps. (The non-leaking log has lots of unpaired mmaps, but that's because of the tricks it does with alignment -- it allocates more than it needs, keeps the chunk in the middle that is aligned where it wants, and unmaps the extra before and after that chunk. I didn't teach my log parser how to deal with that.)

I guess I'll look at what's in the virtual memory.

Comment 15

[Emanuel Hoogeveen [:ehoogeveen]]

Might not be relevant here, but Jason Evans (a jemalloc dev) mentioned in bug 1005844 comment #61 that Linux' heuristics for finding unmapped virtual addresses can lead to fragmentation problems. jemalloc has --disable-munmap for that, but that just disables using munmap completely, which seems more than a little extreme to me.

Comment 16

[Steve Fink [:sfink] [:s:]]

Hrm. Very confusing. As the iteration count goes up, the memory is moving out of a couple of big anonymous mappings but into a collection of 2MB mappings (which end up larger overall.)

Also, if I force a gc every 1000 iterations, it stops the leaking. Which is weird, since I'm already forcing a gc after everything is done (before computing the final vmsize changes.)

Perhaps the problem is just a missing malloc accounting call.

Comment 17

[Shian-Yow Wu [:swu]]

I tried iteration based on js/src/jsapi-tests/testArrayBuffer.cpp and js/src/jsapi-tests/testMappedArrayBuffer.cpp, and found either mem-mapped or mem-allocated array buffer
can cause such VM leakage.

And changing GC interval between iterations results in different size of leakage.

Comment 18

[Shian-Yow Wu [:swu]]

(In reply to Steve Fink [:sfink] from comment #12)
> I wanted to check this out, so I tried patching it with a full BufferKind
> enum (a subset of ArrayBufferFlags) and writing a test. The test turned out
> to be kind of nasty to write, but it shows continuing leakage. I'm not sure
> if that means I messed up the patch, or if this is the remaining leak that
> you detected.
> 

The heap memory leakage was filed in bug 1039914.  We can focus on VM leakage for this bug.
I tried this patch and it can fix the VM leakage specific to mem-mapped array buffer, except one assertion failture that I need to comment out that line to test.  Can we try to land the patch first?

Assertion failure: kind & ~KIND_MASK == 0, at /home/sywu/work/mozilla-central/js/src/vm/ArrayBufferObject.cpp:597

Comment 19

[Steve Fink [:sfink] [:s:]]

Attachment: Pass the flag in setDataPointer() to indicate whether data is memory-mapped

Sorry, I thought I had already put this up for review. I think this version reads better, though the bit twiddling is a little weird.

Comment 20

[Steve Fink [:sfink] [:s:]]

Attachment: Pass the flag in setDataPointer() to indicate whether data is memory-mapped

Just for novelty, here's a version that compiles.

Comment 21

[Steve Fink [:sfink] [:s:]]

Attachment: Pass the flag in setDataPointer() to indicate whether data is memory-mapped

There was a precedence error in the previous patch that caused an assertion to fail.

Comment 22

[Steve Fink [:sfink] [:s:]]

Attachment: Increment the malloc counter for mapped array buffers

In my artificial test case, one problem is that we never trigger a GC based on mapped array buffer allocations, but each one of these is holding onto at least a full page of memory that can't be used for anything else.

With this patch, the vmem growth for the test case is limited to 2MB. This is still well above the 132KB growth I see if I force a GC every 1000 iterations, and it still makes no sense to me that the final GC doesn't clean all of it up. But it's enough that I'd like to know if this fixes the original problem.

Comment 23

[Steve Fink [:sfink] [:s:]]

Can you try your in-browser test case with that last patch? I have to run. Thanks!

Comment 24

[Shian-Yow Wu [:swu]]

Attachment: Test case: Iterations to transfer mapped array buffer by mochitest

Changed the iternation and timeout values from previous patch.

Comment 25

[Shian-Yow Wu [:swu]]

Here is the test result that applied patchs from comment 21 and comment 22.

  Initial:
    76.00 MB ── heap-mapped
    986.34 MB ── vsize
  After 10000 iterations:
    147.00 MB ── heap-mapped
    1,134.31 MB ── vsize
  After 20000 iterations:
    187.00 MB ── heap-mapped
    1,175.23 MB ── vsize

It fixed the original problem of unfreed safety buffer for mapped array buffer.
Though compared to previous result in comment 9, there is no obvious difference on the other vm size growth problem.

Comment 26

[Jeff Walden [:Waldo]]

Comment on attachment 8459888 [details] [diff] [review]
Pass the flag in setDataPointer() to indicate whether data is memory-mapped

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

Looks fair enough, generally.  But I'd like to see the data pointer/kind combined more closely, so we don't have to specify them separately everywhere, and so kind more closely adheres to creation, when possible.  That is, I'd like to see something like:

class BufferContents
{
    void* const data_;
    const BufferKind kind_;

  typedef void (BufferContents::* ConvertibleToBool)();
  void nonNull() {}

  public:
    template<BufferKind Kind>
    static create(void* data)
    {
      BufferContents contents;
      contents.data_ = data;
      contents.kind_ = Kind;
      return contents;
    }

    void* data() const { return data_; }
    BufferKind kind() const { return kind_; }

    operator ConvertibleToBool() const { return data_ ? &BufferContents::nonNull : nullptr; }
};

and then I'd like to see setNewOwnedData, changeContents, AllocateArrayBufferContents, initialize, neuter, and setDataPointer all take (by value) or return this.

Also, it'd be good to see ArrayBufferObject::releaseData refactored to clearly correspond to switching on BufferKind values, so that its correctness is clearer.

::: js/src/vm/ArrayBufferObject.cpp
@@ +498,1 @@
>      buffer->setIsAsmJSMappedArrayBuffer();

I'm not sure I believe this is needed if you pass in ASMJS_MAPPED_BUFFER above.  And why wouldn't you pass it in?  This code just seems wrong.  And with that changed, it seems like setIsAsmJSMappedArrayBuffer should die entirely.  In fact it looks like you should audit all of the setIs* methods here to see which ones have any relevance after this patch.

Comment 27

[Steve Fink [:sfink] [:s:]]

Attachment: Pass the flag in setDataPointer() to indicate whether data is memory-mapped

Something like this?

I had to add a two-arg constructor to BufferContents in addition to the templatized create() because stealContents needs to pass through an arbitrary kind. I think I ended up using it in one other place too.

I also removed the ASMJS_MAPPED_BUFFER bit, since using ASMJS_BUFFER|MAPPED_BUFFER works when that's what you mean, and there are locations that can use just one or the other.

I probably ought to make KIND_MASK = ASMJS_BUFFER | SHARED_BUFFER | MAPPED_BUFFER instead of giving it directly. What do you think?

Comment 28

[Steve Fink [:sfink] [:s:]]

Attachment: Pass the flag in setDataPointer() to indicate whether data is memory-mapped

And compiling with DEBUG found a few more spots.

Comment 29

[Jeff Walden [:Waldo]]

Comment on attachment 8460914 [details] [diff] [review]
Pass the flag in setDataPointer() to indicate whether data is memory-mapped

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

I think I'd like to see an interdiff of the changes, but I think the rest of this is probably short enough to not need a second pass (tho it's a close call).  Happy to re-review if you think a full pass is needed, tho.

::: js/src/vm/ArrayBufferObject.cpp
@@ +490,5 @@
>      memcpy(data, buffer->dataPointer(), buffer->byteLength());
>  
> +    // Swap the new elements into the ArrayBufferObject. Mark the
> +    // ArrayBufferObject so (1) we don't do this again, (2) we know not to
> +    // js_free the data in the normal way.

I think, with BufferContents intrinsically including the kind of data (and data-disposal steps implied by it), the second sentence here isn't quite necessary.

@@ +491,5 @@
>  
> +    // Swap the new elements into the ArrayBufferObject. Mark the
> +    // ArrayBufferObject so (1) we don't do this again, (2) we know not to
> +    // js_free the data in the normal way.
> +    buffer->changeContents(cx, BufferContents::create<BufferKind(ASMJS_BUFFER|MAPPED_BUFFER)>(data));

Mild preference for a contents local variable here, for readability.

@@ +674,2 @@
>          } else {
> +            void *data = AllocateArrayBufferContents(cx, nbytes);

AllocateArrayBufferContents should return a BufferContents, since it always creates MALLOCED_BUFFER data.

@@ +694,3 @@
>          void *data = obj->fixedData(reservedSlots);
>          memset(data, 0, nbytes);
> +        obj->initialize(nbytes, BufferContents::create<UNOWNED_DATA>(data), DoesntOwnData);

Maybe another BufferContents local here, tho this is an even weaker request.

@@ +704,5 @@
> +ArrayBufferObject *
> +ArrayBufferObject::create(JSContext *cx, uint32_t nbytes,
> +                          NewObjectKind newKind /* = GenericObject */)
> +{
> +    return create(cx, nbytes, BufferContents::create<UNOWNED_DATA>(nullptr));

Mildly wondering if BufferContents::nullContents() might be a worthwhile helper.  Or some better name, I gave this one about five seconds' thought.

@@ +782,5 @@
>          js_ReportOverRecursed(cx);
>          return nullptr;
>      }
>  
>      void *oldData = buffer->dataPointer();

ArrayBufferObject::contents() mentioned later on would be good here.  And I guess then rename to |oldContents|, and maybe |newContents| for the backstop.

@@ +798,4 @@
>          return newData;
>      }
>  
>      return oldData;

Uh...if (...) { ... return; } else { ... return; } return;.  Remove this last, preserve the can-be-stolen if/block, and add a comment before the couldn't-be-stolen case to explain why we're returning the new data.

@@ +1094,3 @@
>      } else {
>          newData = buffer->dataPointer();
> +        newKind = buffer->bufferKind();

ArrayBufferObject::contents() to wrap this up, I think.

@@ +1097,3 @@
>      }
>  
> +    ArrayBufferObject::neuter(cx, buffer, ArrayBufferObject::BufferContents(newData, newKind));

Hmm, I was sort of trying to avoid having a public two-arg constructor, seeing how the two halves can de-cohere.  But I guess it might be necessary for this one case of reusing the existing data.  :-\  Or, could we have BufferContents friend ABO::contents() to avoid that exposure?

@@ +1184,5 @@
>  JS_CreateMappedArrayBufferContents(int fd, size_t offset, size_t length)
>  {
> +    ArrayBufferObject::BufferContents contents =
> +        ArrayBufferObject::createMappedContents(fd, offset, length);
> +    MOZ_ASSERT_IF(contents, contents.kind() & ArrayBufferObject::MAPPED_BUFFER);

I wouldn't bother with the assert, seems like it should be very simply produced by createMappedContents.  But it doesn't hurt, at least.

::: js/src/vm/ArrayBufferObject.h
@@ +63,5 @@
>      static const size_t ARRAY_BUFFER_ALIGNMENT = 8;
>  
> +    enum BufferKind {
> +        UNOWNED_DATA        =    0,
> +        MALLOCED_BUFFER     =    0,

We distinguish between these using ownsData(), right?  Probably worth a comment to that effect here, since this looks wrong at a glance.

Should the remaining bits be compacted down to 0x1, and ArrayBufferFlags moved just below this?  Or maybe define all these bits in ArrayBufferFlags, then have BufferKind copy in values from there?  I kind of think we should do the latter.  At the very least, having these far separate isn't a good thing, and we should fix that.

@@ +72,5 @@
> +        KIND_MASK           = 0x1c
> +    };
> +
> +    class BufferContents {
> +        void *data_;

On second thought, can this be uint8_t*?  Buffer contents are always bytes, not like this is exposing typed data of unknown kind like a view's data pointer would be.

::: js/src/vm/SharedArrayObject.cpp
@@ +210,5 @@
>          return nullptr;
>  
>      JS_ASSERT(obj->getClass() == &class_);
>  
> +    obj->initialize(buffer->byteLength(), BufferContents::create<UNOWNED_DATA>(nullptr), DoesntOwnData);

This, uh, is rather screwy, seems to me.  But okay, I guess it works this way now.  :-\

Comment 30

[Steve Fink [:sfink] [:s:]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #29)
> Comment on attachment 8460914 [details] [diff] [review]
> Pass the flag in setDataPointer() to indicate whether data is memory-mapped
> 
> Review of attachment 8460914 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think I'd like to see an interdiff of the changes, but I think the rest of
> this is probably short enough to not need a second pass (tho it's a close
> call).  Happy to re-review if you think a full pass is needed, tho.

There was enough of a ripple effect that the interdiff ended up about the same size as the full patch, so I'll only generate it if you want me to.

> @@ +674,2 @@
> >          } else {
> > +            void *data = AllocateArrayBufferContents(cx, nbytes);
> 
> AllocateArrayBufferContents should return a BufferContents, since it always
> creates MALLOCED_BUFFER data.

True, though it wasn't immediately obvious that it should, because it also handles the realloc case. But yes, reallocing always produces a malloced buffer. I ended up splitting out the realloc case into a ReallocateArrayBufferContents, which ended up being definitely better. I'm conflicted about whether to keep the realloc stuff at all, now that we don't need the stupid ElementsHeader crap anymore. It's still meaningful because it gives a way to do proper malloc accounting for a potentially large consumer of memory -- except that its only caller passes in nullptr for cx, so we're not even getting that now.

Anyway, fodder for another bug.

> @@ +704,5 @@
> > +ArrayBufferObject *
> > +ArrayBufferObject::create(JSContext *cx, uint32_t nbytes,
> > +                          NewObjectKind newKind /* = GenericObject */)
> > +{
> > +    return create(cx, nbytes, BufferContents::create<UNOWNED_DATA>(nullptr));
> 
> Mildly wondering if BufferContents::nullContents() might be a worthwhile
> helper.  Or some better name, I gave this one about five seconds' thought.

I made BufferContents::createUnowned instead. Though the "create" there kind of makes it sound like you're creating something of substance (unlike ::create, which is obviously just a static constructor sort of thing.) Maybe it should be ::unowned() or something instead.

> @@ +798,4 @@
> >          return newData;
> >      }
> >  
> >      return oldData;
> 
> Uh...if (...) { ... return; } else { ... return; } return;.  Remove this
> last, preserve the can-be-stolen if/block, and add a comment before the
> couldn't-be-stolen case to explain why we're returning the new data.

It's obvious, isn't it? The code is:

        memcpy(newContents.data(), oldContents.data(), buffer->byteLength());
        ArrayBufferObject::neuter(cx, buffer, oldContents);
        return newContents.data();

"You can't steal, so make a copy and return it." Seems redundant.

> @@ +1094,3 @@
> >      } else {
> >          newData = buffer->dataPointer();
> > +        newKind = buffer->bufferKind();
> 
> ArrayBufferObject::contents() to wrap this up, I think.

Yes, better.

> @@ +1097,3 @@
> >      }
> >  
> > +    ArrayBufferObject::neuter(cx, buffer, ArrayBufferObject::BufferContents(newData, newKind));
> 
> Hmm, I was sort of trying to avoid having a public two-arg constructor,
> seeing how the two halves can de-cohere.  But I guess it might be necessary
> for this one case of reusing the existing data.  :-\  Or, could we have
> BufferContents friend ABO::contents() to avoid that exposure?

Ooh, yeah, that's better.

Except I don't know how to friend just ABO::contents from a nested class definition, since it isn't defined yet. I ended up friending all of ABO. Give me syntax and I will fix.

> ::: js/src/vm/ArrayBufferObject.h
> @@ +63,5 @@
> >      static const size_t ARRAY_BUFFER_ALIGNMENT = 8;
> >  
> > +    enum BufferKind {
> > +        UNOWNED_DATA        =    0,
> > +        MALLOCED_BUFFER     =    0,
> 
> We distinguish between these using ownsData(), right?  Probably worth a
> comment to that effect here, since this looks wrong at a glance.

I nuked UNOWNED_DATA since it would now only be used in BufferContents::createUnowned(). Think I need a comment for MALLOCED_BUFFER? (It's really just "no bits are set".)

> Should the remaining bits be compacted down to 0x1, and ArrayBufferFlags
> moved just below this?  Or maybe define all these bits in ArrayBufferFlags,
> then have BufferKind copy in values from there?  I kind of think we should
> do the latter.  At the very least, having these far separate isn't a good
> thing, and we should fix that.

The BufferKind is needed early since it's used in a bunch of return values. The ArrayBufferFlags could be right after, but it kind of wants to be protected. I could swap back and forth between public and protected.

I reordered the bits so that the ArrayBufferFlags begins with KIND_MASK and a comment saying that BufferKind goes in there. Take a look and tell me whether you think it's enough.

Comment 31

[Steve Fink [:sfink] [:s:]]

Attachment: Pass the flag in setDataPointer() to indicate whether data is memory-mapped

Ok, here's the latest patch.

Comment 32

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8459891 [details] [diff] [review]
Increment the malloc counter for mapped array buffers

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

Argh, forgot to mark this for review.

Comment 33

[Jeff Walden [:Waldo]]

Comment on attachment 8461222 [details] [diff] [review]
Pass the flag in setDataPointer() to indicate whether data is memory-mapped

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

(In reply to Steve Fink [:sfink], PTO Jul 28-Aug 6 from comment #30)

Blah, I neglected (despite intent to prioritize this while you were around) to get this done before you exited stage right.  Blah, I say!  Oh well, guess that let me punt til today.

> It's obvious, isn't it? The code is:
> 
>         memcpy(newContents.data(), oldContents.data(), buffer->byteLength());
>         ArrayBufferObject::neuter(cx, buffer, oldContents);
>         return newContents.data();
> 
> "You can't steal, so make a copy and return it." Seems redundant.

I've gone back and forth at times writing, then later reading, this code, and thinking it was/wasn't obvious.  I guess what makes it strange is the method by name says it's stealing contents, but then by variable name alone looks like it's not returning something that would be stolen.  "old" and "new" aren't good names here.

> Except I don't know how to friend just ABO::contents from a nested class
> definition, since it isn't defined yet.

Hum, right.  Probably no way around that.

> I nuked UNOWNED_DATA since it would now only be used in
> BufferContents::createUnowned(). Think I need a comment for MALLOCED_BUFFER?
> (It's really just "no bits are set".)

"malloced" isn't true in the inline case, so how about we move away from that?  I think having a name for no bits set is worthwhile for searchability, at the absolute minimum.  VANILLA_BUFFER, SIMPLE_BUFFER, PLAIN_BUFFER, INLINE_OR_MALLOCED_BUFFER, throwing out suggestions.  This would be a better thing to pass to create<> in createUnowned.

> > Should the remaining bits be compacted down to 0x1, and ArrayBufferFlags
> > moved just below this?  Or maybe define all these bits in ArrayBufferFlags,
> > then have BufferKind copy in values from there?

I'd have

  private:
    enum BufferKind {
      MALLOCED_BUFFER = 0x0,
      ASMJS_BUFFER    = 0x1,
      SHARED_BUFFER   = 0x2,
      MAPPED_BUFFER   = 0x4,
      KIND_MASK       = ASMJS_BUFFER | SHARED_BUFFER | MAPPED_BUFFER
    };

  protected:
    enum ArrayBufferFlags {
      KIND_FLAGS = BufferKind::KIND_MASK,

      NEUTERED     = 0x08,
      IN_LIVE_LIST = 0x10,
      OWNS_DATA    = 0x20,
    };

which is proximate enough for me.  Kind of too bad you can't inherit enums.  For this rare case, at least.

> The BufferKind is needed early since it's used in a bunch of return values.
> The ArrayBufferFlags could be right after, but it kind of wants to be
> protected. I could swap back and forth between public and protected.

Yes, swap back and forth.  Adjacent placement of the two enums is worth having to add extra access sections.

::: js/src/vm/ArrayBufferObject.cpp
@@ +685,5 @@
>      gc::AllocKind allocKind = GetGCObjectKind(nslots);
>  
>      Rooted<ArrayBufferObject*> obj(cx, NewBuiltinClassInstance<ArrayBufferObject>(cx, allocKind, newKind));
>      if (!obj)
>          return nullptr;

There's a bug on file for leaking contents here, right?  Should file and fix, separately, if not.

@@ +765,5 @@
>  
>  /* static */ bool
>  ArrayBufferObject::ensureNonInline(JSContext *cx, Handle<ArrayBufferObject*> buffer)
>  {
>      if (!buffer->ownsData()) {

So for everything but SharedArrayBuffer, !ownsData means the storage is in slots.  This code seems fine in that case.  But if it's a shared buffer, doesn't this method unexpectedly de-cohere the ArrayBuffer from its shared data?  Followup to investigate this, maybe, if there's indeed an issue here.

::: js/src/vm/ArrayBufferObject.h
@@ +85,5 @@
> +
> +        template<BufferKind Kind>
> +        static BufferContents create(void *data)
> +        {
> +            return BufferContents((uint8_t*)data, Kind);

static_cast<>

@@ +90,5 @@
> +        }
> +
> +        static BufferContents createUnowned(void *data)
> +        {
> +            return BufferContents((uint8_t*)data, BufferKind(0));

static_cast<>

@@ +97,5 @@
> +        uint8_t *data() const { return data_; }
> +        BufferKind kind() const { return kind_; }
> +
> +        typedef void (BufferContents::* ConvertibleToBool)();
> +        void nonNull() {}

Put both these lines in a private section?  The operator's the only thing that needs to be public.

Comment 34

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/02fd424797c9

Comment 35

[Shian-Yow Wu [:swu]]

There are two patches for this bug, did we miss to land the other one?

Comment 36

[Steve Fink [:sfink] [:s:]]

(In reply to Shian-Yow Wu [:swu] from comment #35)
> There are two patches for this bug, did we miss to land the other one?

Sorry, should have marked this [leave-open]. I'm still finishing up the other patch. (And it's not going to fix 100% of the problems.)

Comment 37

[Steve Fink [:sfink] [:s:]]

Attachment: Fix up ArrayBufferObject malloc accounting, assigning to correct zone for all types

nfroyd - changes to NativeOSFileInternals.cpp only
smaug - changes to nsXMLHttpRequest.cpp only

Comment 38

[Nathan Froyd [:froydnj]]

Comment on attachment 8472746 [details] [diff] [review]
Fix up ArrayBufferObject malloc accounting, assigning to correct zone for all types

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

I'll review a one-line patch anytime!

Comment 39

[Steve Fink [:sfink] [:s:]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c1ea647e6a70
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/495c012e6cf6

Hopefully it won't regress anything.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c1ea647e6a70
https://hg.mozilla.org/mozilla-central/rev/495c012e6cf6

Comment 42

[Steve Fink [:sfink] [:s:]]

Looks like everything here landed, and I just neglected to remove the leave-open.

Comment 43

[Shian-Yow Wu [:swu]]

Mark as RESOLVED/FIXED according to comment 42.