Differential Testing: Different output message involving RegExp

RESOLVED FIXED in Firefox 56

Status

()

Core
JavaScript Engine: JIT
--
major
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: gkw, Assigned: jandem)

Tracking

(Blocks: 2 bugs, {testcase})

Trunk
mozilla56
x86_64
All
testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [fuzzblocker])

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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.
Flags: needinfo?(jdemooij)
(Assignee)

Comment 1

a year ago
Created attachment 8885243 [details] [diff] [review]
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...
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8885243 - Flags: review?(nicolas.b.pierron)
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?
Attachment #8885243 - Flags: review?(nicolas.b.pierron) → review+
(Reporter)

Comment 3

a year ago
Jan, is this ready for landing?
Flags: needinfo?(jdemooij)

Comment 4

a year ago
Pushed by jandemooij@gmail.com:
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
(Assignee)

Comment 5

a year ago
(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!
Flags: needinfo?(jdemooij)
https://hg.mozilla.org/mozilla-central/rev/f0ed1ef5a791
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.