1379936 - Differential Testing: Different output message involving RegExp

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

print("".replace(/x/));
(function () {
    for (var i = 0; i < 2; ++i) {
        x = print(/[^]/g.exec());
    }
})()


$ ./js-dbg-64-dm-linux-0e41d07a703f --fuzzing-safe --no-threads --baseline-eager --no-ion testcase.js

u
u

$ ./js-dbg-64-dm-linux-0e41d07a703f --fuzzing-safe --no-threads --ion-eager testcase.js

u
n

Tested this on m-c rev 0e41d07a703f.

My configure flags are:

AR=ar sh /home/ubuntu/trees/mozilla-central/js/src/configure --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 0e41d07a703f

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/76b0524a77e4
user:        Jan de Mooij
date:        Sun Jul 09 11:46:57 2017 +0200
summary:     Bug 1115355 - Optimize RegExpObject allocation in Ion. r=evilpie

Jan, is bug 1115355 a likely regressor?

Setting [fuzzblocker] because it is very difficult to ignore RegExp differential testing bugs in case of dupes.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

This is annoying. Usually template objects are immutable, but Ion has this no-clone optimization for regular expressions where we can use the template object directly if doing this is not observable (when it doesn't escape to arbitrary JS). However a side-effect is that this can change the lastIndex slot of the template RegExpObject.

This means that Ion background codegen can't use this slot because it can get a bogus value or race with the main thread updating it. Here's a patch to make sure we always initialize this slot to 0 instead of using the template object's value. The other slots are immutable and this matches what we do in CloneRegExpObject.

After all the regexp cloning optimizations, it might be worth removing the no-clone optimization for global/sticky regular expressions at some point...

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8885243 [details] [diff] [review]
Patch

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

::: js/src/jit/MacroAssembler.cpp
@@ +964,5 @@
> +        // Template objects are not exposed to script and therefore immutable.
> +        // However, regexp template objects are sometimes used directly (when
> +        // the cloning is not observable), and therefore we can end up with a
> +        // non-zero lastIndex. Detect this case here and just substitute 0, to
> +        // avoid racing with the main thread updating this slot.

Isn't there a case where the result of the match is attached as slots on the regexp object?

Comment 3

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

Jan, is this ready for landing?

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0ed1ef5a791
Make sure lastIndex slot is always initialized to 0 when cloning regexps in Ion. r=nbp

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> Isn't there a case where the result of the match is attached as slots on the
> regexp object?

There's the RegExpStatics thing but it's shared.

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #3)
> Jan, is this ready for landing?

Sorry for the delay!

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f0ed1ef5a791