Closed Bug 1105261 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: nbp, Assigned: Waldo)

Details

Attachments

(1 file)

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.
We *had* this behavior.  I'm certain of it.  When did this change?  :-(
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.
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
Status: NEW → ASSIGNED
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 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
https://hg.mozilla.org/mozilla-central/rev/68388b632918
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.