1562146 - Assertion failure: uncompressedStart < uncompressedLimit (subtraction below requires a non-empty range), at js/src/vm/Compression.h:79 with ES6 classes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

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

The following testcase crashes on mozilla-central revision 7ffabb358c42 (build with AR=ar 'CXX="clang++ -m32 -msse2 -mfpmath=sse"' 'CC="clang -m32 -msse2 -mfpmath=sse"' PKG_CONFIG_LIBDIR=/usr/lib/pkgconfig sh ./configure --target=i686-pc-linux --enable-debug --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests --disable-cranelift, run with --fuzzing-safe --ion-offthread-compile=off --ion-eager --dump-bytecode):

;function ion(    s) { }
mathy2 = (function(   t) {
    function f(      ) {
              (ff(     ff(      +Infinity),
(  0 + (d2)(1   / 0  ))) (1     ))    ;
    }
})(this, {
    ff:     class z { m          }
}, []);
gcslice(0);
gc(                  );

Backtrace:

#0  js::Compressor::rangeToChunkAndOffset (uncompressedStart=<optimized out>, uncompressedLimit=402, firstChunk=<optimized out>, firstChunkOffset=<optimized out>, firstChunkSize=<optimized out>, lastChunk=<optimized out>, lastChunkSize=<optimized out>) at js/src/vm/Compression.h:78
#1  js::ScriptSource::units<char16_t> (this=0xf6b5d8e0, cx=0xf6b20800, holder=..., begin=201, len=0) at js/src/vm/JSScript.cpp:2057
#2  0x5799bf03 in js::ScriptSource::PinnedUnits<char16_t>::PinnedUnits (cx=<optimized out>, source=<optimized out>, holder=..., begin=<optimized out>, len=<optimized out>, this=<optimized out>) at js/src/vm/JSScript.cpp:2138
#3  js::ScriptSource::substring (this=0xf6b5d8e0, cx=0xf6b20800, start=201, stop=201) at js/src/vm/JSScript.cpp:2168
#4  0x5793f323 in js::FunctionToString (cx=<optimized out>, fun=..., isToSource=<optimized out>) at js/src/vm/JSFunction.cpp:888
#5  0x57cc21ed in JS_DecompileFunction (cx=0xf6b20800, fun=...) at js/src/jsapi.cpp:3702
#6  0x577e886c in ToDisassemblySource (cx=<optimized out>, v=...) at js/src/vm/BytecodeUtil.cpp:1152
#7  0x577dbf42 in Disassemble1 (cx=0x0, script=..., pc=<optimized out>, loc=44, lines=<optimized out>, parser=0x0, sp=<optimized out>) at js/src/vm/BytecodeUtil.cpp:1508
/snip

For detailed crash information, see attachment.

Comment 1

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

Attachment: Detailed Crash Information

Comment 2

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

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset: https://hg.mozilla.org/mozilla-central/rev/27f0cd20a8b0
user: Jason Orendorff
date: Mon Apr 15 20:55:25 2019 +0000
summary: Bug 1529772 - Part 4: Implement ASI for fields that don't have initializers. r=jwalden

Jason, is bug 1529772 a likely regressor?

Comment 3

[Jason Orendorff [:jorendorff]]

Yes. Simpler:

/***********************************************************************
************************************************************************
*****************************************************/
class z { m }
gcslice(0);                       
gc();

This test case is 256 characters. Delete one and it won't trip the assertion.

I won't get to this until tomorrow.

Comment 4

[Fuzzing Team]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 5

[Liz Henry (:lizzard)]

Jason are you still planning to take this on?

Comment 6

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

Matthew mentioned over IRC that he might (starting fresh) look into this...

Comment 7

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

It looks to me like the root cause of this is PinnedUnits not being able to handle a zero range when the source is compressed.

Under rr this needs chaos mode to reproduce, a side effect of the off-thread compression needing to have finished before we ask for the string range of the function initalizer m, which is set to zero intentionally.

What's not clear to me is the correct design portion to put the fix. We could assert we have a non-zero range when constructing a PinnedUnits, and then fix the issue in ScriptSource::substring [1]. That feels easiest, but not sure if better to try and teach PinnedUnits not to freak out with a zero range?

[1]: Do we have a statically allocated JSString* that corresponds to the empty string somewhere?

Comment 8

[Jeff Walden [:Waldo]]

Short-length source isn't supposed to be compressed, ever.

  // There are several cases where source compression is not a good idea:
  //  - If the script is tiny, then compression will save little or no space.
  //  - If there is only one core, then compression will contend with JS
  //    execution (which hurts benchmarketing).
  //
  // Otherwise, enqueue a compression task to be processed when a major
  // GC is requested.

  bool canCompressOffThread = HelperThreadState().cpuCount > 1 &&
                              HelperThreadState().threadCount >= 2 &&
                              CanUseExtraThreads();
  if (length() < ScriptSource::MinimumCompressibleLength ||
      !canCompressOffThread) {
    return true;
  }

If something is trying to compress zero-length source, it should be changed not to.

Comment 9

[Jason Orendorff [:jorendorff]]

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #7)

[1]: Do we have a statically allocated JSString* that corresponds to the empty string somewhere?

Yep, cx->emptyString().

Comment 10

[Jason Orendorff [:jorendorff]]

Oh, I bet you meant static as in StaticStrings. No, the empty string is created using Atomize.

Comment 11

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

Attachment: Bug 1562146 - Handle zero-length spans in ScriptSource::substring r?jorendorff

Comment 12

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

No, you totally got my meaning in the first one; static was the wrong word, i meant a shared empty string representation. :)

Comment 13

[Liz Henry (:lizzard)]

Too late to fix for 70 but we could still take a patch in 71/72.

Comment 14

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

Because this code is only accessible via a debugging interface in browser, I'm marking status-firefox71 as fix-optional, as I suspect i wouldn't request uplift.

(Sorry for the delay, was waiting for review before answering ni? but this can be done independently)

Comment 15

[Jason Orendorff [:jorendorff]]

For posterity: The reason we end up asking for a zero-length span here is that the function we're asking for is the implied field initializer for the field m in class z { m }.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cc3b04156828
Handle zero-length spans in ScriptSource::substring r=jorendorff

Comment 17

[Raul Gurzau (:RaulG)]

Backed out changeset cc3b04156828 (bug 1562146) for SM build bustage at bug1562146.js on a CLOSED TREE.

Backout link: https://hg.mozilla.org/integration/autoland/rev/8dfd8ae933128a7d9c8028c9294e8e0076c1d0a5

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedJob=270540837&resultStatus=testfailed%2Cbusted%2Cexception&revision=cc3b0415682819c351fc94d6142d3486004605fe

Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=270540837&repo=autoland&lineNumber=8719

Log snippet:
[task 2019-10-09T18:57:27.338Z] TEST-PASS | js/src/jit-test/tests/fields/bug1555979.js | Success (code 0, args "") [0.1 s]
[task 2019-10-09T18:57:27.358Z] TEST-PASS | js/src/jit-test/tests/fields/bug1571289.js | Success (code 0, args "") [0.1 s]
[task 2019-10-09T18:57:27.364Z] TEST-PASS | js/src/jit-test/tests/fields/bug1552875.js | Success (code 0, args "") [0.2 s]
[task 2019-10-09T18:57:27.365Z] /builds/worker/workspace/build/src/js/src/jit-test/tests/fields/bug1562146.js:9:1 ReferenceError: disassemble is not defined
[task 2019-10-09T18:57:27.365Z] Stack:
[task 2019-10-09T18:57:27.365Z] @/builds/worker/workspace/build/src/js/src/jit-test/tests/fields/bug1562146.js:9:1
[task 2019-10-09T18:57:27.365Z] Exit code: 3
[task 2019-10-09T18:57:27.365Z] FAIL - fields/bug1562146.js
[task 2019-10-09T18:57:27.365Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/fields/bug1562146.js | /builds/worker/workspace/build/src/js/src/jit-test/tests/fields/bug1562146.js:9:1 ReferenceError: disassemble is not defined (code 3, args "") [0.1 s]
[task 2019-10-09T18:57:27.365Z] INFO exit-status : 3
[task 2019-10-09T18:57:27.365Z] INFO timed-out : False
[task 2019-10-09T18:57:27.365Z] INFO stderr 2> /builds/worker/workspace/build/src/js/src/jit-test/tests/fields/bug1562146.js:9:1 ReferenceError: disassemble is not defined
[task 2019-10-09T18:57:27.365Z] INFO stderr 2> Stack:
[task 2019-10-09T18:57:27.365Z] INFO stderr 2> @/builds/worker/workspace/build/src/js/src/jit-test/tests/fields/bug1562146.js:9:1
[task 2019-10-09T18:57:27.392Z] TEST-PASS | js/src/jit-test/tests/fields/error.js | Success (code 0, args "") [0.1 s]
[task 2019-10-09T18:57:27.392Z] TEST-PASS | js/src/jit-test/tests/fields/literal.js | Success (code 0, args "") [0.1 s]
[task 2019-10-09T18:57:27.397Z] TEST-PASS | js/src/jit-test/tests/fields/initprop.js | Success (code 0, args "") [0.1 s]

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/15efc3d22653
Handle zero-length spans in ScriptSource::substring r=jorendorff

Comment 19

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

(Re-pushed after using pragmas to disable where disassemble isn't available)

Comment 20

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/15efc3d22653