Closed
Bug 1379936
Opened 7 years ago
Closed 7 years ago
Differential Testing: Different output message involving RegExp
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: gkw, Assigned: jandem)
References
Details
(Keywords: testcase, Whiteboard: [fuzzblocker])
Attachments
(1 file)
(deleted),
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
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•7 years ago
|
||
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 2•7 years ago
|
||
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+
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•7 years 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)
Comment 6•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years 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.
Description
•