1448136 - Assertion failure: offset < length(), at js/src/vm/JSScript.h:1234 with Debugger

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 7771df14ea18+ (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --cpu-count=2):

print = function(s) { return s.toString(); }
assertEq = function(a,b) {
  try { print(a); print(b); } catch(exc) {}
}
g = newGlobal();
g.parent = this;
g.eval("(" + function() {
  Debugger(parent).onExceptionUnwind = function(frame) {
    frame.older
  }
} + ")()")
function a() {};
function b() {};
for (let _ of Array(10000))
  assertEq(b instanceof a, true);
function c(){};
function d(){};
function e(){};
Object.defineProperty(a, Symbol.hasInstance, {value: assertEq });
let funcs = [a, b, c, d];
for (let f of funcs)
  assertEq(e instanceof f, true);


Backtrace:

received signal SIGSEGV, Segmentation fault.
0x0000000000643148 in JSScript::offsetToPC (this=<optimized out>, offset=<optimized out>) at js/src/vm/JSScript.h:1234
#0  0x0000000000643148 in JSScript::offsetToPC (this=<optimized out>, offset=<optimized out>) at js/src/vm/JSScript.h:1234
#1  0x000000000062332f in js::jit::ICEntry::pc (script=<optimized out>, this=<optimized out>) at js/src/jit/SharedIC.h:302
#2  js::jit::TryAttachInstanceOfStub (cx=0x7ffff5f16000, frame=frame@entry=0x7fffffffc588, stub=stub@entry=0x7ffff5fa2da8, lhs=..., lhs@entry=..., rhs=..., rhs@entry=..., attached=attached@entry=0x7fffffffc488) at js/src/jit/BaselineIC.cpp:4127
#3  0x00000000006235e4 in js::jit::DoInstanceOfFallback (cx=0x7ffff5f16000, frame=0x7fffffffc588, stub=0x7ffff5fa2da8, lhs=..., rhs=..., res=...) at js/src/jit/BaselineIC.cpp:4176
#4  0x00000797ccca0f8f in ?? ()
[...]
#24 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7ffff5fa2da8	140737320201640
rcx	0x7ffff6c282ad	140737333330605
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffc200	140737488339456
rsp	0x7fffffffc200	140737488339456
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4780	140737354024832
r10	0x58	88
r11	0x7ffff6b9e7a0	140737332766624
r12	0x7ffff5fa2dc8	140737320201672
r13	0x7fffffffc488	140737488340104
r14	0x7fffffffc4a0	140737488340128
r15	0x7fffffffc240	140737488339520
rip	0x643148 <JSScript::offsetToPC(unsigned long) const+104>
=> 0x643148 <JSScript::offsetToPC(unsigned long) const+104>:	movl   $0x0,0x0
   0x643153 <JSScript::offsetToPC(unsigned long) const+115>:	ud2


Likely debugger-only.

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/5d6cc408dfd9
user:        Matthew Gaudet
date:        Tue Dec 12 16:21:45 2017 -0600
summary:     Bug 1420910: Convert the Baseline InstanceOf IC to CacheIR r=jandem

This iteration took 274.364 seconds to run.

Comment 2

[Fuzzing Team]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 28d966d18818).

Comment 3

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

Spent some time looking at this today. I am able to reproduce as described. 

Looking at the stub, it appears that the IC entry has been destroyed by the time we hit the crash: the values are jemalloc'd free memory: 

(lldb) print *stub->icEntry()
(js::jit::ICEntry) $8 = {
  firstStub_ = 0xe5e5e5e5e5e5e5e5
  returnOffset_ = 3857049061
  pcOffset_ = 98952677
  kind_ = -2
}

Under rr, exploring, I am able to see that the icEntry is initialized here

#0  0x000055f64d340fe8 in js::jit::ICFallbackStub::fixupICEntry(js::jit::ICEntry*) (this=0x7fe52f6a4da8, icEntry=0x7fe52f6a2688) at /home/mgaudet/mozilla-unified/js/src/jit/SharedIC.h:791
#1  0x000055f64d328a59 in js::jit::BaselineScript::copyICEntries(JSScript*, js::jit::BaselineICEntry const*) (this=0x7fe52f6a2000, script=0x7fe52e9920d0, entries=0x7fe52f654000) at /home/mgaudet/mozilla-unified/js/src/jit/BaselineJIT.cpp:764
#2  0x000055f64e06eb6e in js::jit::BaselineCompiler::compile() (this=0x7ffd2c536950) at /home/mgaudet/mozilla-unified/js/src/jit/BaselineCompiler.cpp:237
#3  0x000055f64d326ecb in js::jit::BaselineCompile(JSContext*, JSScript*, bool) (cx=0x7fe52f616000, script=0x7fe52e9920d0, forceDebugInstrumentation=false) at /home/mgaudet/mozilla-unified/js/src/jit/BaselineJIT.cpp:256
#4  0x000055f64d3271ae in CanEnterBaselineJIT(JSContext*, JS::HandleScript, js::InterpreterFrame*) (cx=0x7fe52f616000, script=0x7fe52e9920d0, osrFrame=0x7fe52e563028) at /home/mgaudet/mozilla-unified/js/src/jit/BaselineJIT.cpp:300
#5  0x000055f64d327289 in js::jit::CanEnterBaselineAtBranch(JSContext*, js::InterpreterFrame*) (cx=0x7fe52f616000, fp=0x7fe52e563028) at /home/mgaudet/mozilla-unified/js/src/jit/BaselineJIT.cpp:333

and the values are clobbered here: 

#9  0x000055f64d3278c1 in js::jit::BaselineScript::Destroy(js::FreeOp*, js::jit::BaselineScript*) (fop=0x7fe52f62a040, script=0x7fe52f6a2000) at /home/mgaudet/mozilla-unified/js/src/jit/BaselineJIT.cpp:471
#10 0x000055f64e084da8 in js::jit::RecompileOnStackBaselineScriptsForDebugMode(JSContext*, js::Debugger::ExecutionObservableSet const&, js::Debugger::IsObserving) (cx=0x7fe52f616000, obs=..., observing=js::Debugger::Observing) at /home/mgaudet/mozilla-unified/js/src/jit/BaselineDebugModeOSR.cpp:906
#11 0x000055f64d900bc8 in js::Debugger::updateExecutionObservabilityOfFrames(JSContext*, js::Debugger::ExecutionObservableSet const&, js::Debugger::IsObserving) (cx=0x7fe52f616000, obs=..., observing=js::Debugger::Observing) at /home/mgaudet/mozilla-unified/js/src/vm/Debugger.cpp:2515

My totally uninformed hypothesis is that there's a missing patch somewhere, which is allowing us to return into invalid state.

Comment 4

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

I think my next step is going to see if there's a good reason this stopped reproducing on central; will bisect for when the test case stopped failing.

Comment 5

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

Attachment: Ensure Debug OSR transition is respected in InstanceOf Fallback stub

Comment 6

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

Attachment: Ensure Debug OSR transition is respected in InstanceOf Fallback stub

Comment 7

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

try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2982b42d4b30dde7d6f3a0f7f4367dabceb2b13&selectedJob=169985288

(rust is red, but I am suspicious that this is simply because the base revision for the try build is comparatively old)

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8961935 [details] [diff] [review]
Ensure Debug OSR transition is respected in InstanceOf Fallback stub

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

Good catch! r=me with nits below addressed.

::: js/src/jit-test/tests/cacheir/bug1448136.js
@@ +10,5 @@
> +  }
> +} + ")()")
> +function a() {};
> +function b() {};
> +for (let _ of Array(10000))

Nit: does it still fail without the patch if we change this to 100 or so?

::: js/src/jit/BaselineIC.cpp
@@ +4156,5 @@
>          return false;
>      }
>  
> +    // This fallback stub may trigger debug mode toggling.
> +    DebugModeOSRVolatileStub<ICInstanceOf_Fallback*> debug_stub(ICStubEngine::Baseline, frame, stub);

Nit: to match what we do elsewhere, I'd move this to the top of the function, rename the function's |stub| argument to |stub_| and here do:

DebugModeOSRVolatileStub<ICInstanceOf_Fallback*> stub(frame, stub_);

Comment 9

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

Attachment: Ensure Debug OSR transition is respected in InstanceOf Fallback stub

Comment 10

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

Attachment: Ensure Debug OSR transition is respected in InstanceOf Fallback stub

Comment 11

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

Comment on attachment 8962872 [details] [diff] [review]
Ensure Debug OSR transition is respected in InstanceOf Fallback stub

Carrying Jan's earlier r+, with nits addressed. 

The test case did fail with a smaller array size, so I dropped it to 100.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/af66807a5c4b
Ensure Debug OSR transition is respected in InstanceOf Fallback stub. r=jandem

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/af66807a5c4b

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Is there a user impact which warrants uplift consideration here, or can this ride the trains?

Comment 15

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

Comment on attachment 8962872 [details] [diff] [review]
Ensure Debug OSR transition is respected in InstanceOf Fallback stub

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1420910
[User impact if declined]: There's a chance of crashes when debugging
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No 
[Why is the change risky/not risky?]: Low risk as the code only has impact in a small number of circumstances when running in the debugger. 
[String changes made/needed]: None

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8962872 [details] [diff] [review]
Ensure Debug OSR transition is respected in InstanceOf Fallback stub

Low-risk crash fix with automated test coverage. Approved for 60.0b8.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bfe7012b7d58