Closed Bug 1105261 Opened 5 years ago Closed 5 years ago

mozilla::VectorBase with non-empty inlined storage allow infallibleAppend on non-reserved space.


(Core :: MFBT, defect)

Not set





(Reporter: nbp, Assigned: Waldo)



(1 file)

At the moment one can write the following code, and it will not assert on debug builds:

        JS::AutoValueVector vec(cx);

The issue here is that the implementation of the AutoValueVector has an inlined storage of 8 elements, which does not appear in the current snippet of code.  

Thus, if somebody were to change the code of AutoValueVector to reduce the amount of inlined storage, then infallible append will start to fail in debug build, and worse in optimized builds.

I suggest that we change the this behaviour such that the assertion are raised independently of the kInlineCapacity.
We *had* this behavior.  I'm certain of it.  When did this change?  :-(
Bah.  Odinmonkey landing.  I can't find any mention or discussion of this behavior change in that (tracking) bug, or in any of the dependencies.
Attached patch Patch with testSplinter Review
There were, fortunately, only a few places that depended on inline capacity being reserved, all easily fixed.  There were also two places that were reserving using the wrong quantity, in jsreflect.cpp.  I think those cases would actually have been visibly buggy, if inputs had exceeded the builtin capacity instead of usually/always being beneath it.
Attachment #8529162 - Flags: review?(nicolas.b.pierron)
Assignee: nobody → jwalden+bmo
Comment on attachment 8529162 [details] [diff] [review]
Patch with test

Review of attachment 8529162 [details] [diff] [review]:

::: mfbt/Vector.h
@@ +335,5 @@
>  #ifdef DEBUG
> +  /**
> +   * The amount of explicitly allocated space in this vector that is immediately
> +   * available to be filled by appending additional elements.  This value is
> +   * always greater than or equal to |length()| and |capacity()|.  It may be

|reserved()| is always lesser than or equal to |capacity()| ?

::: mfbt/tests/TestVector.cpp
@@ +60,5 @@
> +  MOZ_RELEASE_ASSERT(iv.reserve(55));
> +  MOZ_RELEASE_ASSERT(iv.reserved() == 55);
> +
> +  iv.clearAndFree();
> +  MOZ_RELEASE_ASSERT(iv.reserved() == 0);

nit: Can you also check the same is true after a move constructor?
Attachment #8529162 - Flags: review?(nicolas.b.pierron) → review+
Backed out this and the bug 052139 patch in because one of them was making b2g desktop crash like in linux64 mochitest-oop nearly all the time, and in both linux64 and mac gaia-ui-test-functional-1 and -3 quite often.

Pushed just this one to try for you in
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.