Closed Bug 1271507 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: gkw, Assigned: fitzgen)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, testcase, Whiteboard: [fuzzblocker] [jsbugmon:])

Attachments

(4 files, 2 obsolete files)

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.
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?
Flags: needinfo?(nfitzgerald)
Whiteboard: [jsbugmon:update] → [fuzzblocker][jsbugmon:update]
Whiteboard: [fuzzblocker][jsbugmon:update] → [fuzzblocker] [jsbugmon:]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
god bless fuzzers
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
Flags: needinfo?(nfitzgerald)
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!
Flags: needinfo?(gary)
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.
Attachment #8750867 - Flags: review?(jimb)
Attachment #8750867 - Flags: review?(jimb) → review+
Attached file `bt all` output (obsolete) —
Flags: needinfo?(gary) → needinfo?(nfitzgerald)
I couldn't run `bt full` on lldb on Mac, perhaps `bt all` will do the trick?
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.
Flags: needinfo?(nfitzgerald)
Keywords: leave-open
Try push is good other than the missing guard for when `oomAfterAllocations` isn't defined.
Attached file `bt all` output take 2
Take 2, this time with --enable-debug --enable-more-deterministic --disable-optimize.
Attachment #8750923 - Attachment is obsolete: true
Flags: needinfo?(nfitzgerald)
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???
Finally reproduced locally, and here is a fix :)
Attachment #8751512 - Flags: review?(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?
Attachment #8751512 - Flags: review?(jimb) → review-
You were right! Good eye ;)
Attachment #8751926 - Flags: review?(jimb)
Attachment #8751512 - Attachment is obsolete: true
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.
Attachment #8751926 - Flags: review?(jimb) → review+
https://hg.mozilla.org/mozilla-central/rev/0671d2ac18f3
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.