Closed Bug 1301377 Opened 5 years ago Closed 5 years ago

JitcodeGlobalEntry not found with profiler and compacting GC

Categories

(Core :: JavaScript: GC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(1 file)

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);
}
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.
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.
Attachment #8800158 - Flags: review?(jdemooij)
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`?
(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 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?
Attachment #8800158 - Flags: review?(jdemooij) → review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cdbe9d9ea62
Disallow GC while using ProfilingFrameIterator r=jandem
https://hg.mozilla.org/mozilla-central/rev/5cdbe9d9ea62
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.