1604952 - Differential Testing: Different output message involving deferred parser allocation

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

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

/x/;

$ ./js-64-asan-linux-x86_64-f870bccd07ee --fuzzing-safe --no-threads --no-baseline --no-ion --parser-deferred-alloc testcase.js
-----------------------------------------------------
Suppressions used:
  count      bytes template
      1          4 js::frontend::Parse
-----------------------------------------------------

$

$ ./js-64-asan-linux-x86_64-f870bccd07ee --fuzzing-safe --no-threads --no-baseline --no-ion testcase.js
$

Tested this on m-c rev f870bccd07ee, run with ASAN_OPTIONS=detect_leaks=1

My configure flags are:

AR=ar sh ./configure --enable-address-sanitizer --disable-jemalloc --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset: https://hg.mozilla.org/mozilla-central/rev/886319fa10ea
user: Matthew Gaudet
date: Wed Nov 06 18:45:28 2019 +0000
summary: Bug 1580338 - Defer RegExp allocation until after parse r=tcampbell

Matt, is bug 1580338 a likely regressor?

Comment 1

[Matthew Gaudet (he/him) [:mgaudet]]

100% a likely regressor.

I've been looking at this for a little bit since Gary pinged me offline.

The leaked allocation is here, where we duplicate the RegExp string in RegExpCreationData::init.

It's worth noting that the leak doesn't occur if the regexp is used (so /x/.exec("") does not leak); this makes me suspect the cleanup story is getting corrupted by something like a dead-code elimination pass.

It's seems worth noting that I seem to have lost some of the thread around this exact case when attempting to land this code. It landed with an out-of-date comment that suggested that this would be registered in the ParseInfo, and cleaned up that way; but there's nothing in the ParseInfo of that revision that suggests it was the case.

Working on reverse-engineering how this cleanup does happen right now, and how we could accidentally be losing it.

Comment 2

[Matthew Gaudet (he/him) [:mgaudet]]

Ah, OK. I see the bug:

So the normal case is that during bytecode emission, when we emit a RegExpLiteral, we need to get the GCThingList index, and so we call this append method, which moves the RegExpCreationData into the GCThingList. At this point, it is the GCThingList being destroyed that cleans up and frees the pointer that's being lost.

In the simple /x/ test case, what's happening is that something (FoldConstants is my guess) deletes the reference to the RegExpLiteralNode, but this means it's no longer emitted, and so the memory is leaked when the LifoAlloc tears down all the parse nodes without running destructors.

As to how to fix this: My feeling is that ParseInfo ought to have a Vector<RegExpCreationData>, and RegExpLiteral ought to refer to the creation data via an index.

Comment 3

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1604952 - Rename FunctionCreationData.h to Stencil.h r?tcampbell

Because I'm planning on moving non-function data into this, I think the time is
potentially right to start calling this stencil.h

Comment 4

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1604952 - Move RegExpCreationData to Stencil.h r?tcampbell

As a side effect of removing a now-cylic-dependency, this rehomes
FunctionSytaxKind.

Depends on D57902

Comment 5

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1604952 - Change ownership of RegExpCreationData by holding them on frontend::ParseInfo r?tcampbell

By doing this we guarantee RegExpCreationData are cleaned up, avoiding a possible leak where
dead code elimination removes a ParseNode without cleaning up the RegExpCreationData.

Depends on D57903

Comment 6

[Ryan VanderMeulen [:RyanVM]]

I assume we're going to leave Fx72 as-is here. Also, can we land a test for this?

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/54a84425dcf0
Rename FunctionCreationData.h to Stencil.h r=tcampbell

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf14c11659ad
Move RegExpCreationData to Stencil.h r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/19f1d3da6e34
Change ownership of RegExpCreationData by holding them on frontend::ParseInfo r=tcampbell

Comment 9

[Matthew Gaudet (he/him) [:mgaudet]]

Good reminder; I did add a test to the landed patches.

Definitely wontfix for previous releases as this code path is dead without explicit opt in.

Comment 10

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/54a84425dcf0
https://hg.mozilla.org/mozilla-central/rev/bf14c11659ad
https://hg.mozilla.org/mozilla-central/rev/19f1d3da6e34