1233175 - Assertion failure: !isSharedMemory, at /builds/slave/m-cen-m64-d-000000000000000000/build/src/js/src/vm/TypedArrayObject.cpp:373

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack without symbols

for (var i = 0; i < 999; i++) {
    eval("new Int8Array(new SharedArrayBuffer());");
}

asserts js debug shell on m-c rev 0babaa3edcf908c393b68a3dc2d1c2a2450c31ed with --fuzzing-safe --ion-offthread-compile=off --ion-eager at Assertion failure: !isSharedMemory, at /builds/slave/m-cen-m64-d-000000000000000000/build/src/js/src/vm/TypedArrayObject.cpp:373

I tested using:

https://queue.taskcluster.net/v1/task/w7SLIXgPQpCLbRCP9Cn_6Q/artifacts/public/build/jsshell-mac64.zip

from:

https://tools.taskcluster.net/index/artifacts/#gecko.v2.mozilla-central.pushdate.2015.12.16.20151216110158.firefox/gecko.v2.mozilla-central.pushdate.2015.12.16.20151216110158.firefox.macosx64-debug

on Mac OS X 10.7.2 on a build loaner slave. (Request one from https://wiki.mozilla.org/ReleaseEngineering/RequestingASlave)

No symbols are available because downloaded js shells don't have symbols.

The regression window seems to be:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=949161bdd28a3d8ad4b50fa18fa9af53a1267571&tochange=a1cd28ddbfa2a2afb8330ad43c7d5d0591f1e34b

which seems to point to bug 1230162 as a likely regressor. Thus, setting needinfo? from Lars. Also setting js-s-s because this involves SharedArrayBuffer.

This is a [fuzzblocker] which is happening very frequently while the build slaves are fuzzing in their spare time. This one took me a long time to track down.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This is not on FuzzManager because:

(1) it only happens on 10.7 (our build slaves run this), not on 10.11.
(2) our build slaves do not yet report to FuzzManager.
(3) we don't have symbols that would make bucketing this crash easier.

Comment 2

[Lars T Hansen [:lth]]

Will look at this in the morning.

Comment 3

[Lars T Hansen [:lth]]

I can repro with the provided shell on a local 10.7.5 machine (very old hardware), as well as with a fresh debug build on that machine.  No repro on 10.11.

There are a couple of failure modes here.

Looking at the code and the provided backtrace and the failing instruction, this is probably API abuse (of JS_GetArrayBufferViewType) resulting in a MOZ_CRASH, and not s-s.   Certainly there is a bug in CTypes.cpp near line 3109, it calls CanConvertTypedArrayObjectTo() on what is an ArrayBufferViewObject without knowing that the TA's buffer is an ArrayBuffer, yet CanConvertTypedArrayObjectTo() assumes just that by calling JS_GetArrayBufferViewType() on the buffer without an appropriate guard.  The comment at 3109, "Same as ArrayBuffer, above, ..." is misleading.  I guess the bug could technically be in CanConvertTypedArrayObjectTo(); potato-potahto.

The assertion that's reported in the bug summary is another matter, and this is where the debug build crashes too.  This is caused by some logic that uses IsInsideNursery() to draw some conclusions about whether the buffer is an inline TypedObject buffer; that logic is almost certainly stale and needs some work.

Why it fails on 10.7 and not 10.11 is unclear but could have to do with system effects, eg, SharedArrayBuffer uses mmap heavily and 10.7 may have strange restrictions; or the system is slower and GC may kick in or not.  Bug 1230162 changed the mmap patterns and probably did not cause this bug but may have brought it out into the open.

Comment 4

[Lars T Hansen [:lth]]

The assertion that fails is incorrect in the case of a length-zero SharedArrayRawBuffer that is allocated exactly adjoining the bottom of the nursery.  In this case, the "data" pointer will have the same address as the low boundary of the nursery and so the isInside() test will succeed, tripping the assertion.

This is a system effect of how mmap orders its pages and could easily vary among operating system versions, hence it's explainable why this happens on 10.7 only.

The best fix here is to adjust the assertion slightly.

Comment 5

[Lars T Hansen [:lth]]

BTW, the assertion is certainly benign, nothing bad will happen in release builds.  I'll look at the other problem with CTypes first, before unblocking, however.

Comment 6

[Lars T Hansen [:lth]]

Attachment: refine an assertion

Comment 7

[Lars T Hansen [:lth]]

I take back the comment about CTypes, that code is fine as it is I think.

Comment 8

[Terrence Cole [:terrence]]

Comment on attachment 8699430 [details] [diff] [review]
refine an assertion

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

Good find. This is not the first time a zero-size allocation optimization has bitten us in this way.

::: js/src/vm/TypedArrayObject.cpp
@@ +379,5 @@
> +                    MOZ_ASSERT(buffer->byteLength() == 0 &&
> +                               cx->runtime()->gc.nursery.start() ==
> +                                   buffer->dataPointerEither().unwrapValue());
> +                else
> +                    cx->runtime()->gc.storeBuffer.putWholeCell(obj);

Our style guide is clear about single statements not *needing* braces, but I still feel like this code could use them, given how far away the else is.

Comment 9

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/09fbc07f80c0226ef49e02e5e64a517420c8384f
Bug 1233175 - refine an assertion. r=terrence

Comment 10

[Lars T Hansen [:lth]]

Regarding whether an uplift is required: only if it affects fuzzing or other testing on the branches, LMK.

Comment 11

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

We should at least land it on 45, which is going to be the next ESR, and at present it is merely Aurora for now.

Comment 12

[Lars T Hansen [:lth]]

[Tracking Requested - why for this release]: See comment 11, it will be useful to clean up the incorrect assertion for the ESR release.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/09fbc07f80c0

Comment 14

[Jeff Walden [:Waldo]]

(In reply to Terrence Cole [:terrence] from comment #8)
> ::: js/src/vm/TypedArrayObject.cpp
> @@ +379,5 @@
> > +                    MOZ_ASSERT(buffer->byteLength() == 0 &&
> > +                               cx->runtime()->gc.nursery.start() ==
> > +                                   buffer->dataPointerEither().unwrapValue());
> > +                else
> > +                    cx->runtime()->gc.storeBuffer.putWholeCell(obj);
> 
> Our style guide is clear about single statements not *needing* braces, but I
> still feel like this code could use them, given how far away the else is.

Unreal.  That absolutely doesn't track any understanding I've ever had of SpiderMonkey style.  o_O  I fixed the style guide to reflect the long-standing rule that if any conditional, any consequent, or any alternative occupies multiples lines, regardless whether the occupying thing is or is not a single statement, everything gets braces.

Comment 15

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Opening up as per request by :lth over IRC.

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Lars, could you fill an uplift request to aurora? Thanks

Comment 17

[Lars T Hansen [:lth]]

Comment on attachment 8699430 [details] [diff] [review]
refine an assertion

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Fuzzing is tricky because of spurious false asserts
[Describe test coverage new/current, TreeHerder]: Fuzzing test case, not landed
[Risks and why]: Low, this is strictly more lenient
[String/UUID change made/needed]:

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cbdebcdd2a69