1247701 - Differential Testing: Different output message involving .exec

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

/x/.exec(undefined);
x = /x/.exec('x');
Object.defineProperty(x, 5, {
    configurable: true
})
x.unshift(0);
for (var p in x) {
    x.shift();
}
print(p);


$ ./js-dbg-64-dm-clang-darwin-ac39fba33c6d --fuzzing-safe --no-threads --baseline-eager testcase.js
0

$ ./js-dbg-64-dm-clang-darwin-ac39fba33c6d --fuzzing-safe --no-threads --ion-eager testcase.js
input


Tested this on m-c rev ac39fba33c6d.

My configure flags are:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin14.5.0 --disable-jemalloc --enable-debug --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/funfuzz/js/compileShell.py -b "--enable-debug --enable-more-deterministic" -r ac39fba33c6d

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/28cc01acfd02
user:        Tooru Fujisawa
date:        Thu Sep 24 18:28:37 2015 +0900
summary:     Bug 1207922 - Part 1: Self-host RegExp.prototype.{exec,test}. r=till,h4writer

Arai-san, is bug 1207922 a likely regressor?

Comment 1

[Tooru Fujisawa [:arai]]

That changeset introduces self-hosted function for RegExp#exec, that can be a entry point for Ion execution.
Same issue happens with following code, before that changeset.  so the underlying issue is more old thing.

/x/.exec(undefined);
function f() {
    return /x/.exec('x');
}
x = f();
x = f();
Object.defineProperty(x, 5, {
    configurable: true
})
x.unshift(0);
for (var p in x) {
    x.shift();
}
print(p);

Comment 2

[Tooru Fujisawa [:arai]]

the for-in loop there is executed twice with --ion-eager, and 3 times with --baseline-eager.

[code]

/x/.exec(undefined);
function f() {
    return /x/.exec('x');
}
x = f();
x = f();
Object.defineProperty(x, 5, {
    configurable: true
})
x.unshift(0);
for (var p in x) {
    print(p, x[p]);
    var s = x.shift();
    print("shift:", s);
}
print(p);


[output with --baseline-eager]

index 0
shift: 0
input x
shift: x
0 undefined
shift: undefined
0


[output with --ion-eager]

index 0
shift: 0
input x
shift: x
input


I guess there are at least 2 factors:
  1. the difference between match result object created in JitCompartment::generateRegExpMatcherStub (generateRegExpExecStub) stub and js::CreateRegExpMatchResult
  2. the different for-in enumeration order caused by --enable-more-deterministic option (JS_MORE_DETERMINISTIC in jsiter.cpp)

Comment 3

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

Using the testcase in comment 1:

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/654dcfdaccca
user:        Steve Fink
date:        Fri Dec 06 17:00:49 2013 -0800
summary:     Bug 944164 - set the shell variable scriptPath to communicate the currently running script to the debugger, r=jorendorff

Steve, is bug 944164 a likely regressor? Also setting needinfo? from Jan in case there's something to do with JIT/regex as :arai analysed in comment 2.

(else, someone will have to dig into this for real.)

Comment 4

[Tooru Fujisawa [:arai]]

with --ion-eager, CallBoxedOrUnboxedSpecialization in array_slice always returns DenseElementResult::Incomplete, because ObjectMayHaveExtraIndexedProperties returns true in ArrayShiftDenseKernel.

with --baseline-eager, CallBoxedOrUnboxedSpecialization returns DenseElementResult::Incomplete on 1st time. and it returns DenseElementResult::Success on 2nd and 3rd time.


SuppressDeletedProperty is called only once for DenseElementResult::Success,
and may be called some times for DenseElementResult::Incomplete.

Comment 5

[Tooru Fujisawa [:arai]]

before for-in loop, x has 0, 1, and 6-th elements.

at 1st shift call, it moves elements from 1->0 and 6->5.
in SetArrayElement call for 6->5 move, ensureDenseElements is called with index=5, extra=1.

https://dxr.mozilla.org/mozilla-central/rev/576a6dcde5b68c2ea45324ed5ce1dabb7d833d09/js/src/jsarray.cpp#2223
> bool
> js::array_shift(JSContext* cx, unsigned argc, Value* vp)
> ...
>             if (!SetArrayElement(cx, obj, i, value))
>                 return false;


with --ion-eager execution, currentCapacity is 14, and ensureDenseElements succeeds in following line:

https://dxr.mozilla.org/mozilla-central/rev/576a6dcde5b68c2ea45324ed5ce1dabb7d833d09/js/src/vm/NativeObject-inl.h#208
> inline DenseElementResult
> NativeObject::ensureDenseElements(ExclusiveContext* cx, uint32_t index, uint32_t extra)
> ...
>         if (index < currentCapacity) {
>             ensureDenseInitializedLengthNoPackedCheck(cx, index, 1);
>             return DenseElementResult::Success;
>         }


with --baseline-eager execution, currentCapacity is 2, and ensureDenseElements fails, and maybeDensifySparseElements is called on x and it's converted into dense array.  that's the reason of the different execution in 2nd loop.


so, 1st loop does different internal behavior depends on ion or not, but it shouldn't be the problem.
I guess, the issue is that inconsistent behavior about SuppressDeletedProperty in array_shift, between result==DenseElementResult::Success and result==DenseElementResult::Incomplete.

https://dxr.mozilla.org/mozilla-central/rev/576a6dcde5b68c2ea45324ed5ce1dabb7d833d09/js/src/jsarray.cpp#2194
>     DenseElementResult result = CallBoxedOrUnboxedSpecialization(functor, obj);
>     if (result != DenseElementResult::Incomplete) {
>...
>     }

Comment 6

[Tooru Fujisawa [:arai]]

when the array is dense and contains holes, CallBoxedOrUnboxedSpecialization succeeds, but we should update the enumeration by checking each element and calling SuppressDeletedProperty when the element is hole.

the content of array x changes following:
  before loop:     [0,    0,    hole, hole, hole, hole, undefined]
  after 1st shift: [0,    hole, hole, hole, hole, undefined]
  after 2nd shift: [hole, hole, hole, hole, undefined]


with --baseline-eager execution, SuppressDeletedProperty is not called for 0-th element, and "0" is still in enumeration list (ni->props_*).

Also, because of --enable-more-deterministic, "index" and "input" is enumerated before numeric properties, 3rd loop is executed with p = "0".

without --enable-more-deterministic, "0" is enumerated before the 1st shift, and the issue does not happen with this testcase.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Update for-in enumeration properties when shifting dense Array with holes.

Added SuppressDeletedProperty call to ArrayShiftDenseKernel, that corresponds to DeletePropertyOrThrow in array_shift with DenseElementResult::Incomplete case.

To call SuppressDeletedProperty, changed JSObject* parameter of ArrayShiftDenseKernel to HandleObject, and changed Value* to MutableHandleValue just for consistency.

Comment 8

[Tooru Fujisawa [:arai]]

sorry, the testcase in the patch was wrong.  it does not fail with unpatched tree.
will prepare fixed one.

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Update for-in enumeration properties when shifting dense Array with holes.

Replaced the testcase with simpler one.

It tests that enumerated property is owned by the object.

https://tc39.github.io/ecma262/#sec-enumerate-object-properties
> The mechanics and order of enumerating the properties is not specified
> but must conform to the rules specified below.
> ...
> Properties of the target object may be deleted during enumeration.
> A property that is deleted before it is processed by the iterator's next
> method is ignored.
> ...
> If new properties are added to the target object during enumeration, the
> newly added properties are not guaranteed to be processed in the active
> enumeration.

Currently we're not enumerating newly added properties, but it's not specced, so the testcase just asserts |p in x|, instead of testing against more reduced set.


Without this patch, it fails by enumerating "2" on 2rd iteration, that should be removed by SuppressDeletedProperty.  that's the same situation as "0" is enumerated in comment #0.

Comment 10

[Steve Fink [:sfink] [:s:]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #3)
> Using the testcase in comment 1:

I think arai updated the test case.

> Steve, is bug 944164 a likely regressor?

No.

> (else, someone will have to dig into this for real.)

Looks like arai is on top of it.

Comment 11

[Tooru Fujisawa [:arai]]

looks like, this was partially fixed in bug 624041.
and maybe related to bug 486326.
not sure which bug was the actual regressor tho.

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8718865 [details] [diff] [review]
Update for-in enumeration properties when shifting dense Array with holes.

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

Thanks for fixing this!

In CanOptimizeForDenseStorage, we simply bail if the object has the OBJECT_FLAG_ITERATED flag. Can we do the same at the start of ArrayShiftDenseKernel? I think that's fine and it seems a bit simpler than trying to handle this edge case. Furthermore, after that we can get rid of the SuppressDeletedProperty call in js::array_shift, and that might be a (very small) perf win :)

(We should really replace SuppressDeletedProperty with something better, but that's not for this bug.)

::: js/src/jsarray.cpp
@@ +2150,5 @@
>      MOZ_ASSERT(result != DenseElementResult::Incomplete);
>      if (result == DenseElementResult::Failure)
>          return DenseElementResult::Failure;
>  
> +    if (obj->getGroup(cx)->hasAllFlags(OBJECT_FLAG_ITERATED)) {

Nit: getGroup is fallible

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Bail from ArrayShiftDenseKernel if the array is used by for-in iteration.

Thank you for reviewing :)
Added bailing code to ArrayShiftDenseKernel, and removed SuppressDeletedProperty call from result == DenseElementResult::Success case.

Comment 14

[Tooru Fujisawa [:arai]]

maybe I should've added MOZ_UNLIKELY() to both conditions in ArrayShiftDenseKernel

Comment 15

[Jan de Mooij [:jandem]]

Comment on attachment 8719493 [details] [diff] [review]
Bail from ArrayShiftDenseKernel if the array is used by for-in iteration.

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

Thanks!

Comment 16

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5631db6889e4a4c1afe3004aac3f22e5736b4a7
Bug 1247701 - Bail from ArrayShiftDenseKernel if the array is used by for-in iteration. r=jandem

Comment 17

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/98716dc7dbd981d3c7da9f951a5c76e99b724e91
Bug 1247701 followup - Change ArrayShiftDenseKernel to receive handle. r=bz

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c5631db6889e
https://hg.mozilla.org/mozilla-central/rev/98716dc7dbd9

Comment 19

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

Arai-san, do you mind nominating this for approval‑mozilla‑esr45 as well? It would aid fuzzing on that branch.

Comment 20

[Tooru Fujisawa [:arai]]

Attachment: (mozilla-esr45) Bail from ArrayShiftDenseKernel if the array is used by for-in iteration. r=jandem,bz

Folded 2 patched landed to m-c into 1, backported to mozilla-esr45.

[Approval Request Comment]
> If this is not a sec:{high,crit} bug, please state case for ESR consideration
This bug can be easily hit with --enable-more-deterministic configuration, that is used in fuzzing.  Now fuzzing team is preparing to run fuzzer against ESR branch, and this bug will be hit there too.  It would be nice to backport this, to focus on other more important bug on ESR branch.

> User impact if declined:
It's hard to hit this bug on release configuration (no --enable-more-deterministic), so not so much impact for browser users.

> Fix Landed on Version
47

> Risk to taking this patch (and alternatives if risky)
Almost no, it's tested for nearly 2 cycles on nightly and aurora.

> String or UUID changes made by this patch
None

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8738777 [details] [diff] [review]
(mozilla-esr45) Bail from ArrayShiftDenseKernel if the array is used by for-in iteration. r=jandem,bz

Simplify the life of the fuzzer, taking it.
Should be in 45.1.0

Comment 22

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

https://hg.mozilla.org/releases/mozilla-esr45/rev/cadc27270fb0