Closed Bug 1247701 Opened 6 years ago Closed 6 years ago

Differential Testing: Different output message involving .exec

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
All
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed
firefox48 --- fixed
firefox-esr45 --- fixed

People

(Reporter: gkw, Assigned: arai)

Details

(Keywords: regression, testcase)

Attachments

(2 files, 2 obsolete files)

/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?
Flags: needinfo?(arai.unmht)
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);
Flags: needinfo?(arai.unmht)
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)
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.)
Flags: needinfo?(sphink)
Flags: needinfo?(jdemooij)
No longer blocks: 1207922
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.
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) {
>...
>     }
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.
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.
Assignee: nobody → arai.unmht
Attachment #8718815 - Flags: review?(jdemooij)
sorry, the testcase in the patch was wrong.  it does not fail with unpatched tree.
will prepare fixed one.
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.
Attachment #8718815 - Attachment is obsolete: true
Attachment #8718815 - Flags: review?(jdemooij)
Attachment #8718865 - Flags: review?(jdemooij)
(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.
Flags: needinfo?(sphink)
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 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
Attachment #8718865 - Flags: review?(jdemooij)
Flags: needinfo?(jdemooij)
Thank you for reviewing :)
Added bailing code to ArrayShiftDenseKernel, and removed SuppressDeletedProperty call from result == DenseElementResult::Success case.
Attachment #8718865 - Attachment is obsolete: true
Attachment #8719493 - Flags: review?(jdemooij)
maybe I should've added MOZ_UNLIKELY() to both conditions in ArrayShiftDenseKernel
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!
Attachment #8719493 - Flags: review?(jdemooij) → review+
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
https://hg.mozilla.org/mozilla-central/rev/c5631db6889e
https://hg.mozilla.org/mozilla-central/rev/98716dc7dbd9
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Arai-san, do you mind nominating this for approval‑mozilla‑esr45 as well? It would aid fuzzing on that branch.
Flags: needinfo?(arai.unmht)
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
Flags: needinfo?(arai.unmht)
Attachment #8738777 - Flags: review+
Attachment #8738777 - Flags: approval-mozilla-esr45?
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
Attachment #8738777 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
You need to log in before you can comment on or make changes to this bug.