1271507 - Assertion failure: chars_, at js/src/vm/SharedImmutableStringsCache.h:266

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

The following testcase crashes on mozilla-central revision 043082cb7bd8 (build with --enable-debug --enable-profiling, run with --fuzzing-safe --ion-offthread-compile=off --no-baseline --no-ion):

// Adapted from randomly chosen testcase: js/src/jit-test/tests/debug/bug-1238610.js
load("testcase.js");

and testcase.js is:

lfcode = new Array();
  oomAfterAllocations(100);
loadFile(file);
  lfGlobal = newGlobal()
  for (lfLocal in this)
      if (!(lfLocal in lfGlobal))
          lfGlobal[lfLocal] = this[lfLocal]
  offThreadCompileScript(lfVarx)
  lfGlobal.runOffThreadScript()

Backtrace:

0   js-dbg-64-prof-clang-darwin-043082cb7bd8	0x000000010f750136 js::SharedImmutableStringsCache::getOrCreate(mozilla::UniquePtr<char [], JS::FreePolicy>&&, unsigned long) + 582 (SharedImmutableStringsCache.h:266)
1   js-dbg-64-prof-clang-darwin-043082cb7bd8	0x000000010f539762 js::ScriptSource::setCompressedSource(js::ExclusiveContext*, mozilla::UniquePtr<char [], JS::FreePolicy>&&, unsigned long, unsigned long) + 66 (jsscript.cpp:2055)
2   js-dbg-64-prof-clang-darwin-043082cb7bd8	0x000000010f69ad8d js::SourceCompressionTask::complete() + 285 (HelperThreads.cpp:1628)
3   js-dbg-64-prof-clang-darwin-043082cb7bd8	0x000000010f41e948 Evaluate(JSContext*, JS::Handle<JSObject*>, JS::Handle<js::StaticScope*>, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::MutableHandle<JS::Value>) + 1256 (jscntxt.h:434)
4   js-dbg-64-prof-clang-darwin-043082cb7bd8	0x000000010f41e2e9 JS::Evaluate(JSContext*, JS::ReadOnlyCompileOptions const&, char const*, unsigned long, JS::MutableHandle<JS::Value>) + 409 (jsapi.cpp:4574)
/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)]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/98a28a1fce30
user:        Nick Fitzgerald
date:        Fri May 06 16:53:45 2016 -0700
summary:     Bug 1211723 and 1260570 - Share JS source text between JSRuntimes; r=jimb

This happens often with profiling builds, setting [fuzzblocker].

Nick, is bug 1211723 or bug 1260570 a likely regressor?

Comment 3

[Fuzzing Team]

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

Comment 4

[Jim Blandy :jimb]

god bless fuzzers

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I'm having a hard time reproducing :(

Gary, if you can still reproduce, can you attach the output of `bt full` from gdb? Thanks a ton!

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Don't insert nullptr into the cache

It looks like we are trying to insert a nullptr string into the cache, but based
on the stack, I can't figure out where it is coming from. Here are some more
asserts to hopefully help narrow it down.

Comment 7

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

Attachment: `bt all` output

Comment 8

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

I couldn't run `bt full` on lldb on Mac, perhaps `bt all` will do the trick?

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Thanks Gary. Unfortunately, the argument values I was hoping for appear to be been optimized away, or are otherwise unavailable :(

Here is a try push for the additional assertions patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf9e81438fab

Adding leave-open because this doesn't fix the underlying bug, just adds more asserts.

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push is good other than the missing guard for when `oomAfterAllocations` isn't defined.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/42ce52f86d7d

Comment 12

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

Attachment: `bt all` output take 2

Take 2, this time with --enable-debug --enable-more-deterministic --disable-optimize.

Comment 13

[Jim Blandy :jimb]

That is surprising. We trip the assertion creating the Lookup here:
https://hg.mozilla.org/mozilla-central/file/1579b9e2e50f/js/src/vm/SharedImmutableStringsCache-inl.h#l20

But the pointer was non-zero when we entered here:
https://hg.mozilla.org/mozilla-central/file/1579b9e2e50f/js/src/vm/SharedImmutableStringsCache.cpp#l111

So have we misunderstood what that code means???

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Only setCompressedSource() when we have the compressed source text

Finally reproduced locally, and here is a fix :)

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4821c959724

Comment 16

[Jim Blandy :jimb]

Comment on attachment 8751512 [details] [diff] [review]
Only setCompressedSource() when we have the compressed source text

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

::: js/src/vm/HelperThreads.cpp
@@ +1622,5 @@
>  
>      if (result == Success) {
> +        // ~SourceCompressionTask always calls `complete()`, but if someone
> +        // already explicitly called it, then we already gave ownership of the
> +        // compressed source text to the ScriptSource in that first call.

I don't believe this explanation. SourceCompressionTask::complete should only ever be called from the main thread, so we should be able to treat it as non-reentrant. Before we exit, we clear ss, which marks the SCT as !active(), so we should never run the body of the function more than once.

The only exception should be if the call to setCompressedSource fails: we've cleared compressed, but not ss. And tests/basic/bug-1271507.js *does* include an OOM. Isn't what's really going on here that this function is failing to roll back correctly after OOM?

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/42ce52f86d7d

Comment 18

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Ensure the SourceCompressionTask becomes inactive on OOM

You were right! Good eye ;)

Comment 19

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6afe6ac52fe5

Comment 20

[Jim Blandy :jimb]

Comment on attachment 8751926 [details] [diff] [review]
Ensure the SourceCompressionTask becomes inactive on OOM

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

Yeah, that's more like it.

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0671d2ac18f3

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0671d2ac18f3