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

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Nicolas B. Pierron [:nbp]]

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

        JS::AutoValueVector vec(cx);
        vec.infallibleAppend(...);

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.

Comment 1

[Jeff Walden [:Waldo]]

We *had* this behavior.  I'm certain of it.  When did this change?  :-(

Comment 2

[Jeff Walden [:Waldo]]

Bah.  http://hg.mozilla.org/mozilla-central/rev/b3d85b68449d  Odinmonkey landing.  I can't find any mention or discussion of this behavior change in that (tracking) bug, or in any of the dependencies.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Patch with test

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.

Comment 4

[Nicolas B. Pierron [:nbp]]

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?

Comment 5

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3ce0e3a147c

Comment 6

[Phil Ringnalda (:philor)]

Backed out this and the bug 052139 patch in https://hg.mozilla.org/integration/mozilla-inbound/rev/7b8eb088c380 because one of them was making b2g desktop crash like https://treeherder.mozilla.org/logviewer.html#?job_id=4324215&repo=mozilla-inbound 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 https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=53b2d2d55f03

Comment 7

[Phil Ringnalda (:philor)]

Relanded in https://hg.mozilla.org/integration/mozilla-inbound/rev/68388b632918 as apparently mostly-harmless.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/68388b632918