Closed Bug 956434 Opened 7 years ago Closed 7 years ago

Intermittent TEST-UNEXPECTED-FAIL | testArrayBufferSlice.js | --no-baseline --no-ion | 8:12 Error: Assertion failed: got 16, expected 43

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox27 --- unaffected
firefox28 --- unaffected
firefox29 --- fixed
firefox-esr24 --- unaffected

People

(Reporter: KWierso, Assigned: sfink)

References

Details

(Keywords: intermittent-failure)

Attachments

(3 files, 1 obsolete file)

https://tbpl.mozilla.org/php/getParsedLog.php?id=32534074&tree=Mozilla-Inbound
slave: bld-linux64-ec2-123


TEST-PASS | js/src/jit-test/tests/basic/testArgumentsPropLookup.js | --no-baseline --no-ion --no-ti
TEST-PASS | js/src/jit-test/tests/basic/testArrayBufferSlice.js | 
TEST-PASS | js/src/jit-test/tests/basic/testArrayBufferSlice.js | --ion-eager --ion-parallel-compile=off
TEST-PASS | js/src/jit-test/tests/basic/testArrayBufferSlice.js | --ion-eager --ion-parallel-compile=off --ion-check-range-analysis --no-sse3
TEST-PASS | js/src/jit-test/tests/basic/testArrayBufferSlice.js | --baseline-eager
TEST-PASS | js/src/jit-test/tests/basic/testArrayBufferSlice.js | --baseline-eager --no-ti --no-fpu
FAIL - basic/testArrayBufferSlice.js
TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/basic/testArrayBufferSlice.js | --no-baseline --no-ion
INFO exit-status     : 3
INFO timed-out       : False
INFO stdout          > 
INFO stderr         2> /builds/slave/m-in_l64-d_sm-ra-0000000000000/src/js/src/jit-test/tests/basic/testArrayBufferSlice.js:8:12 Error: Assertion failed: got 16, expected 43
INFO stderr         2> 
TEST-PASS | js/src/jit-test/tests/basic/testArrayBufferSlice.js | --no-baseline --no-ion --no-ti
TEST-PASS | js/src/jit-test/tests/basic/testArrayComp1.js |
Disabled for now due to this test causing other unpredictable failures on rootanalysis builds from unrelated changes.

https://hg.mozilla.org/integration/mozilla-inbound/rev/658e342f400c
Whiteboard: [test disabled][leave open]
Making this block GGC so that we don't forget about this. Steve is investigating.
Assignee: nobody → sphink
This is a very real and major problem (when exact rooting is enabled.) I'm not really sure why this is so intermittent -- this ought to hit when you get a minor collection while creating any of the slices. I have a fix, but I'm going to try to figure out how to make it fail a little more reliably first.
The bug is that when slicing an array, a pointer to the origin buffer's contents is passed on the stack across a GC, so when that pointer is used to initialize the contents of the newly created buffer, it can be invalid.

But this only happens if the origin buffer is moved from the nursery to the tenured area when creating a slice. With JS_GC_ZEAL=7, there are lots of extra minor GCs, and if one of them hits for any other allocation, the origin buffer will get tenured and the problem won't be seen. (So sadly, forcing verification GCs too frequently  will make this sort of problem *less* likely to be found.)

I attempted to at least add some additional stuff to the test to make it fail reliably, but even a schedulegc(1) won't do it, because apparently the gczeal frequency doesn't apply to object cache allocations.

If I force it to apply to those too, I can force a failure.
Attachment #8356277 - Flags: review?(terrence)
This makes the test reliably reproduce. The part I tacked onto the end of the test isn't strictly necessary, since it'll hit earlier.
Attachment #8356288 - Flags: review?(terrence)
Comment on attachment 8356288 [details] [diff] [review]
Apply gczeal to object cache allocations

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

r=me

::: js/src/vm/Runtime-inl.h
@@ +55,5 @@
>      if (type->shouldPreTenure())
>          heap = gc::TenuredHeap;
>  
> +    if (cx->runtime()->upcomingZealousGC())
> +        return nullptr;

Nice! Might want to make sure the test suite actually runs with this change ;-).
Attachment #8356288 - Flags: review?(terrence) → review+
(In reply to Terrence Cole [:terrence] from comment #8)
> Nice! Might want to make sure the test suite actually runs with this change
> ;-).

Er, yeah. Good point. https://tbpl.mozilla.org/?tree=Try&rev=686a67373e4c
Comment on attachment 8356277 [details] [diff] [review]
Do not pass an internal pointer on the stack

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

I think this is clearer to boot. r=me

::: js/src/vm/TypedArrayObject.cpp
@@ -621,2 @@
>  {
> -    SkipRoot skip(cx, &contents);

e_e
Attachment #8356277 - Flags: review?(terrence) → review+
Please also remove the quit(0) from the test :)
Requesting re-review for the following changes:

1. I introduced a rooting hazard. Or rather, I previously hid a rooting hazard behind an internal pointer, and this exposed it. So I Handle-ified createSlice.

2. The memset(data, 0, len) got lost, so I wasn't clearing ArrayBuffer memory when allocating without initial data.

(I'll land a test for these problems too.)
Attachment #8356670 - Flags: review?(terrence)
Attachment #8356277 - Attachment is obsolete: true
Comment on attachment 8356670 [details] [diff] [review]
Do not pass an internal pointer on the stack

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

r=me

::: js/src/vm/TypedArrayObject.cpp
@@ +611,5 @@
>      SetViewList(this, view);
>  }
>  
>  JSObject *
> +ArrayBufferObject::create(JSContext *cx, uint32_t nbytes, bool clear)

I generally like to add a note that the parameter is defaulted and what the default is: e.g. (..., bool clear /* = true */). I've frequently found these annotations helpful when reading as it keeps you from having to flip to the header to get that info.

::: js/src/vm/TypedArrayObject.h
@@ -67,2 @@
>  
> -    static JSObject *createSlice(JSContext *cx, ArrayBufferObject &arrayBuffer,

I wonder if this was reported as an unsafe reference?
Attachment #8356670 - Flags: review?(terrence) → review+
But it improved octane-zlib on the 64-bit machine.
It also regressed octane-Gameboy (4% on 32-bit and 2,5% on 64-bit).
On the 32-bit it regressed asmjs-apps-zlib-throughput and asmjs-apps-zlib-loadtime.
On the 64-bit it improved asmjs-apps-zlib-throughput.
It regressed asmjs-apps-lua_binarytrees-loadtime on both machines.
It regressed asmjs-apps-box2d-loadtime on the 32-bit machine.
It regressed misc-bugs-636096-model2d on the 64-bit machine.
It regressed kraken-stringify-tinderbox on the 64-bit machine.
It turns out that the slowdown is due to a redundant zeroing of out-of-line memory (which is already allocated via calloc.)
Attachment #8357595 - Flags: review?(terrence)
Comment on attachment 8357595 [details] [diff] [review]
Avoid unnecessary zeroing of memory

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

Makes sense. r=me
Attachment #8357595 - Flags: review?(terrence) → review+
https://hg.mozilla.org/mozilla-central/rev/9ff6aa4c9bc9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.