1301377 - JitcodeGlobalEntry not found with profiler and compacting GC

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

The test case for bug 1297360 causes an assertion failure in compacting GC builds:

Assertion failure: entry, at /home/worker/workspace/build/src/js/src/jit/JitcodeMap.h:1038

This is in JitcodeGlobalTable::lookupInfallible when an entry is not found for the given pointer.

The test case is:

var lfLogBuffer = `
  gczeal(14);
  enableSPSProfiling();
  gczeal(15,3);
  var s = "";
  for (let i = 0; i != 30; i+=2) {}
  readSPSProfilingStack(s, "c0d1c0d1c0d1c0d1c0d1c0d1c0d1c0");
`;
loadFile(lfLogBuffer);
function loadFile(lfVarx) {
  evaluate(lfVarx);
}

Comment 1

[Jon Coppeard (:jonco)]

The testcase above requires the options "--ion-eager --ion-offthread-compile=off" and should be run with JS_GC_ZEAL=14.

The problem is that we can GC while using JS::ProfilingFrameIterator to iterate the stack.  GC can invalidate Ion code which means the iterator can ends up with stale pointers into invalidated code.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: bug1301377-profile-iterator

The fix is to disallow the possibility of GC while using ProfilingFrameIterator.  Most of the patch is to change the ReadSPSProfilingStack function to copy all the information it needs first before it creates JS objects from it.  We don't need to change the browser's use of this.

Comment 3

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

Comment on attachment 8800158 [details] [diff] [review]
bug1301377-profile-iterator

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

::: js/public/ProfilingFrameIterator.h
@@ +48,5 @@
>  {
>      JSRuntime* rt_;
>      uint32_t sampleBufferGen_;
>      js::Activation* activation_;
> +    JS::AutoAssertNoAlloc noAlloc;

Drive-by question: Why `AutoAssertNoAlloc` instead of `AutoCheckCannotGC`?

Comment 4

[Jon Coppeard (:jonco)]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #3)

> Drive-by question: Why `AutoAssertNoAlloc` instead of `AutoCheckCannotGC`?

Good point.  I generally prefer AutoAssertNoAlloc to AutoAssertOnGC but AutoCheckCannotGC has the benefit of working with the static analysis too.

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8800158 [details] [diff] [review]
bug1301377-profile-iterator

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

Sorry for the delay.

::: js/public/ProfilingFrameIterator.h
@@ +48,5 @@
>  {
>      JSRuntime* rt_;
>      uint32_t sampleBufferGen_;
>      js::Activation* activation_;
> +    JS::AutoAssertNoAlloc noAlloc;

What fitzgen said. The cases caught by AutoAssertNoAlloc are a strict subset of the ones caught by AutoCheckCannotGC?

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cdbe9d9ea62
Disallow GC while using ProfilingFrameIterator r=jandem

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5cdbe9d9ea62