1245860 - Hit MOZ_CRASH(Invalid PC offset for IC entry.) at js/src/jit/BaselineJIT.cpp:641 with crash [@ js::jit::BaselineScript::icEntryFromPCOffset]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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

g = newGlobal();
g.parent = this;
g.eval("(" + function() { 
  dbg = Debugger(parent);
  dbg.onExceptionUnwind = function(frame) frame.older.onStep  
} + ")()");
function test() { for (res = 0; !res;) {} }
function x() assertJSON;
g.eval("dbg.onIonCompilation = function () parent.x()");
test();



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x00000000005e218f in js::jit::BaselineScript::icEntryFromPCOffset (this=<optimized out>, pcOffset=<optimized out>) at js/src/jit/BaselineJIT.cpp:641
#0  0x00000000005e218f in js::jit::BaselineScript::icEntryFromPCOffset (this=<optimized out>, pcOffset=<optimized out>) at js/src/jit/BaselineJIT.cpp:641
#1  0x0000000000601204 in fallbackStub (this=0x7ffff6922d60) at js/src/jit/BaselineDebugModeOSR.cpp:142
#2  CloneOldBaselineStub (entryIndex=<optimized out>, entries=..., cx=0x7ffff6907800) at js/src/jit/BaselineDebugModeOSR.cpp:746
#3  js::jit::RecompileOnStackBaselineScriptsForDebugMode (cx=cx@entry=0x7ffff6907800, obs=..., observing=observing@entry=js::Debugger::Observing) at js/src/jit/BaselineDebugModeOSR.cpp:900
#4  0x00000000009bd4fd in js::Debugger::updateExecutionObservabilityOfFrames (cx=cx@entry=0x7ffff6907800, obs=..., observing=js::Debugger::Observing) at js/src/vm/Debugger.cpp:2042
#5  0x00000000009bd814 in js::Debugger::ensureExecutionObservabilityOfFrame (cx=cx@entry=0x7ffff6907800, frame=...) at js/src/vm/Debugger.cpp:2233
#6  0x00000000009c7d69 in js::Debugger::getScriptFrameWithIter (this=this@entry=0x7ffff694c800, cx=cx@entry=0x7ffff6907800, frame=..., maybeIter=maybeIter@entry=0x7fffffff9268, vp=..., vp@entry=...) at js/src/vm/Debugger.cpp:505
#7  0x00000000009c81a5 in getScriptFrame (vp=..., iter=..., cx=0x7ffff6907800, this=0x7ffff694c800) at js/src/vm/Debugger.h:888
#8  DebuggerFrame_getOlder (cx=0x7ffff6907800, argc=<optimized out>, vp=<optimized out>) at js/src/vm/Debugger.cpp:6420
#9  0x0000000000a9c7a2 in js::CallJSNative (cx=0x7ffff6907800, native=0x9c7ee0 <DebuggerFrame_getOlder(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:235
#10 0x0000000000a960a1 in js::Invoke (cx=cx@entry=0x7ffff6907800, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:475
#11 0x0000000000a96bcc in js::Invoke (cx=cx@entry=0x7ffff6907800, thisv=..., fval=..., argc=argc@entry=0, argv=argv@entry=0x0, rval=..., rval@entry=...) at js/src/vm/Interpreter.cpp:527
#12 0x0000000000a96df4 in js::InvokeGetter (cx=cx@entry=0x7ffff6907800, thisv=..., fval=..., rval=rval@entry=...) at js/src/vm/Interpreter.cpp:637
#13 0x0000000000a97130 in CallGetter (vp=..., shape=..., receiver=..., obj=..., cx=0x7ffff6907800) at js/src/vm/NativeObject.cpp:1672
#14 GetExistingProperty<(js::AllowGC)1> (cx=0x7ffff6907800, receiver=..., obj=..., shape=..., vp=...) at js/src/vm/NativeObject.cpp:1724
#15 0x0000000000a977e4 in NativeGetPropertyInline<(js::AllowGC)1> (cx=0x7ffff6907800, obj=..., receiver=..., id=..., nameLookup=nameLookup@entry=NotNameLookup, vp=...) at js/src/vm/NativeObject.cpp:1939
#16 0x0000000000a97d20 in js::NativeGetProperty (cx=<optimized out>, obj=..., receiver=..., id=..., vp=...) at js/src/vm/NativeObject.cpp:1973
#17 0x0000000000610cc0 in js::GetProperty (cx=<optimized out>, obj=..., receiver=..., id=..., vp=...) at js/src/vm/NativeObject.h:1471
#18 0x0000000000a98ab2 in GetProperty (vp=..., name=<optimized out>, receiver=..., obj=..., cx=0x7ffff6907800) at js/src/jsobj.h:822
#19 js::GetProperty (cx=cx@entry=0x7ffff6907800, v=..., v@entry=..., name=..., name@entry=..., vp=vp@entry=...) at js/src/vm/Interpreter.cpp:4068
#20 0x0000000000a8740f in GetPropertyOperation (vp=..., lval=..., pc=<optimized out>, script=..., fp=<optimized out>, cx=0x7ffff6907800) at js/src/vm/Interpreter.cpp:219
#21 Interpret (cx=cx@entry=0x7ffff6907800, state=...) at js/src/vm/Interpreter.cpp:2518
#22 0x0000000000a95dc8 in js::RunScript (cx=cx@entry=0x7ffff6907800, state=...) at js/src/vm/Interpreter.cpp:425
#23 0x0000000000a960fd in js::Invoke (cx=cx@entry=0x7ffff6907800, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:493
#24 0x0000000000a96bcc in js::Invoke (cx=cx@entry=0x7ffff6907800, thisv=..., fval=..., argc=argc@entry=2, argv=argv@entry=0x7fffffffa560, rval=..., rval@entry=...) at js/src/vm/Interpreter.cpp:527
#25 0x00000000009d5002 in js::Debugger::fireExceptionUnwind (this=this@entry=0x7ffff694c800, cx=cx@entry=0x7ffff6907800, vp=..., vp@entry=...) at js/src/vm/Debugger.cpp:1290
#26 0x00000000009d5331 in operator() (dbg=0x7ffff694c800, __closure=<synthetic pointer>) at js/src/vm/Debugger.cpp:762
#27 dispatchHook<js::Debugger::slowPathOnExceptionUnwind(JSContext*, js::AbstractFramePtr)::__lambda5, js::Debugger::slowPathOnExceptionUnwind(JSContext*, js::AbstractFramePtr)::__lambda6> (fireHook=..., cx=0x7ffff6907800, hookIsEnabled=...) at js/src/vm/Debugger.cpp:1465
#28 js::Debugger::slowPathOnExceptionUnwind (cx=cx@entry=0x7ffff6907800, frame=...) at js/src/vm/Debugger.cpp:763
#29 0x0000000000a86acb in onExceptionUnwind (frame=..., cx=0x7ffff6907800) at js/src/vm/Debugger-inl.h:58
#30 HandleError (regs=..., cx=0x7ffff6907800) at js/src/vm/Interpreter.cpp:1171
#31 Interpret (cx=cx@entry=0x7ffff6907800, state=...) at js/src/vm/Interpreter.cpp:3962
#32 0x0000000000a95dc8 in js::RunScript (cx=cx@entry=0x7ffff6907800, state=...) at js/src/vm/Interpreter.cpp:425
#33 0x0000000000a960fd in js::Invoke (cx=cx@entry=0x7ffff6907800, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:493
#34 0x0000000000a96bcc in js::Invoke (cx=cx@entry=0x7ffff6907800, thisv=..., fval=..., argc=<optimized out>, argv=0x7ffff455b1a0, rval=...) at js/src/vm/Interpreter.cpp:527
#35 0x0000000000984aa7 in js::DirectProxyHandler::call (this=this@entry=0x1bfb8d0 <js::CrossCompartmentWrapper::singleton>, cx=cx@entry=0x7ffff6907800, proxy=..., proxy@entry=..., args=...) at js/src/proxy/DirectProxyHandler.cpp:77
#36 0x0000000000989c13 in js::CrossCompartmentWrapper::call (this=0x1bfb8d0 <js::CrossCompartmentWrapper::singleton>, cx=0x7ffff6907800, wrapper=..., args=...) at js/src/proxy/CrossCompartmentWrapper.cpp:289
#37 0x0000000000988bda in js::Proxy::call (cx=0x7ffff6907800, proxy=proxy@entry=..., args=...) at js/src/proxy/Proxy.cpp:391
#38 0x0000000000988caa in js::proxy_Call (cx=<optimized out>, argc=<optimized out>, vp=<optimized out>) at js/src/proxy/Proxy.cpp:683
#39 0x0000000000a9c7a2 in js::CallJSNative (cx=0x7ffff6907800, native=0x988c00 <js::proxy_Call(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:235
#40 0x0000000000a9635a in js::Invoke (cx=cx@entry=0x7ffff6907800, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:463
#41 0x0000000000a86e12 in Interpret (cx=cx@entry=0x7ffff6907800, state=...) at js/src/vm/Interpreter.cpp:2799
#42 0x0000000000a95dc8 in js::RunScript (cx=cx@entry=0x7ffff6907800, state=...) at js/src/vm/Interpreter.cpp:425
#43 0x0000000000a960fd in js::Invoke (cx=cx@entry=0x7ffff6907800, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:493
#44 0x0000000000a96bcc in js::Invoke (cx=cx@entry=0x7ffff6907800, thisv=..., fval=..., argc=argc@entry=1, argv=argv@entry=0x7fffffffc020, rval=..., rval@entry=...) at js/src/vm/Interpreter.cpp:527
#45 0x00000000009cf09f in js::Debugger::fireOnIonCompilationHook (this=this@entry=0x7ffff694c800, cx=cx@entry=0x7ffff6907800, scripts=..., scripts@entry=..., graph=...) at js/src/vm/Debugger.cpp:1428
#46 0x00000000009cf47c in operator() (dbg=0x7ffff694c800, __closure=<synthetic pointer>) at js/src/vm/Debugger.cpp:1759
#47 dispatchHook<js::Debugger::slowPathOnIonCompilation(JSContext*, JS::Handle<js::GCVector<JSScript*> >, js::LSprinter&)::__lambda9, js::Debugger::slowPathOnIonCompilation(JSContext*, JS::Handle<js::GCVector<JSScript*> >, js::LSprinter&)::__lambda10> (fireHook=..., cx=0x7ffff6907800, cx@entry=0x7ffff6949850, hookIsEnabled=...) at js/src/vm/Debugger.cpp:1465
#48 js::Debugger::slowPathOnIonCompilation (cx=cx@entry=0x7ffff6907800, scripts=..., scripts@entry=..., graph=...) at js/src/vm/Debugger.cpp:1761
#49 0x000000000068027f in onIonCompilation (graph=..., scripts=..., cx=0x7ffff6907800) at js/src/vm/Debugger-inl.h:81
#50 js::jit::LazyLink (cx=0x7ffff6907800, calleeScript=..., calleeScript@entry=...) at js/src/jit/Ion.cpp:651
#51 0x000000000068278c in BaselineCanEnterAtBranch (pc=0x7ffff699e39f "ず", osrFrame=0x7fffffffc638, script=..., cx=0x7ffff6907800) at js/src/jit/Ion.cpp:2617
#52 js::jit::IonCompileScriptForBaseline (cx=cx@entry=0x7ffff6907800, frame=frame@entry=0x7fffffffc638, pc=pc@entry=0x7ffff699e39f "ず") at js/src/jit/Ion.cpp:2694
#53 0x00000000005f8157 in js::jit::DoWarmUpCounterFallbackOSR (cx=0x7ffff6907800, frame=0x7fffffffc638, stub=0x7ffff69bb0b8, infoPtr=0x7fffffffc610) at js/src/jit/BaselineIC.cpp:141
#54 0x00007ffff7ff2679 in ?? ()
[...]
#65 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7ffff6922d60	140737330163040
rcx	0x7ffff6ca53cd	140737333842893
rdx	0x0	0
rsi	0x7ffff6f7a9d0	140737336814032
rdi	0x7ffff6f791c0	140737336807872
rbp	0x7fffffff86d0	140737488324304
rsp	0x7fffffff86d0	140737488324304
r8	0x7ffff7fe07c0	140737354008512
r9	0x6372732f736a2f6c	7165916604736876396
r10	0x7fffffff8490	140737488323728
r11	0x7ffff6c27960	140737333328224
r12	0x7ffff4539f50	140737292509008
r13	0x7ffff6907800	140737330051072
r14	0x0	0
r15	0x1	1
rip	0x5e218f <js::jit::BaselineScript::icEntryFromPCOffset(unsigned int)+207>
=> 0x5e218f <js::jit::BaselineScript::icEntryFromPCOffset(unsigned int)+207>:	movl   $0x281,0x0
   0x5e219a <js::jit::BaselineScript::icEntryFromPCOffset(unsigned int)+218>:	callq  0x4a3e80 <abort()>

Comment 1

[Jan de Mooij [:jandem]]

NI shu as this involves debug mode OSR.

Comment 2

[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/3bbd0d929128
user:        Hannes Verschore
date:        Fri Aug 14 17:57:57 2015 +0200
summary:     Bug 1178834: IonMonkey - Always lazy link code, r=jandem

This iteration took 250.965 seconds to run.

Comment 3

[Shu-yu Guo [:shu]]

What's going on here is that we are attempting to enable debug mode for on-stack frames in the middle of a WarmUpCounter_Fallback, due to the new onIonCompilation hook. Normally, WarmUpCounter_Fallback cannot trigger debug mode toggling. In this fuzz test, the onIonCompilation hook enters the debuggee compartment via |parent.x()| and throws an error, which in turn triggers onExceptionUnwind and attempts to do a debug mode OSR on top of normal OSR. This is complexity that I'd rather not wrap my head around.

After talking with Jim, I don't think it's worth the implementation complexity to support calling this hook *at the exact point of Ion compilation*. Instead, this interface should be moved to a log-like interface where Ion compile events enqueue to a log and the debugger user drains the log. I forget if I argued for or against a log-like structure in the beginning, but I think this case is sufficient argument to do away with the current hook API.

nbp, would you be willing to change the API?

Comment 4

[Jan de Mooij [:jandem]]

That seems reasonable to me; onIonCompilation keeps showing up in fuzz tests.

Comment 5

[Hannes Verschore [:h4writer]]

If we do we can revert the code/work around in bug 1214059

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Shu-yu Guo [:shu] from comment #3)
> nbp, would you be willing to change the API?

Moving the onIonCompilation hook, to be logging on the Debugger compartment and not calling any functions sounds good to me.

I think we can keep the reporting mechanism which is currently in place and just replace the call to the JS function by a storage on the debugger object.

Unfortunately, I intent to avoid getting additional work such that I can commit to the ThreeHeadedMonkey soon.

Comment 7

[Shu-yu Guo [:shu]]

Okay, I don't have time right now either. Who would have time to do this work?

Comment 8

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

(In reply to Shu-yu Guo [:shu] from comment #7)
> Okay, I don't have time right now either. Who would have time to do this
> work?

Setting needinfo? for Jan to find someone who can do this.

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] from comment #6)
> Unfortunately, I intent to avoid getting additional work such that I can
> commit to the ThreeHeadedMonkey soon.

That's not how it works. Dealing with regressions, including fuzz bugs, is simply part of the job. All of us have bugs and projects we *want* to work on, but that doesn't mean we get to neglect our other responsibilities.

Since you seem unwilling to maintain the onIonCompilation hook, and it's not used in m-c, the other option is to remove it from SpiderMonkey.

Comment 10

[Nicolas B. Pierron [:nbp]]

Attachment: Remove Debugger.onIonCompilation Hook.

As the onIonCompilation hook seems to be unused, and there is no plan to
make use of it for the JitCoach, or similar devtools projects ...

I guess we should remove this code, even if this is the only way we had to
make Jit developer lifes better in the browser.

Comment 11

[Shu-yu Guo [:shu]]

(In reply to Nicolas B. Pierron [:nbp] from comment #10)
> Created attachment 8717417 [details] [diff] [review]
> Remove Debugger.onIonCompilation Hook.
> 
> As the onIonCompilation hook seems to be unused, and there is no plan to
> make use of it for the JitCoach, or similar devtools projects ...
> 
> I guess we should remove this code, even if this is the only way we had to
> make Jit developer lifes better in the browser.

Removal was your choice over changing the API and maintenance. Don't be passive aggressive about it.

Comment 12

[Nicolas B. Pierron [:nbp]]

(In reply to Shu-yu Guo [:shu] from comment #11)
> Removal was your choice over changing the API and maintenance. Don't be
> passive aggressive about it.

No, removal is the only choice left if nobody wants to take over the maintenance of this code, and if I do not want to be distracted by this code anymore.

I would be happy to not land this patch if there is enough interest to use this feature.  So far, I have not heard of anything which crystallized as a goal.

Personally, I think I have higher priorities than spending time at arguing over the interface of the Debugger and its documentation.  Thus, removing the code is the easiest way forward for me, but backward for the devtools.

Comment 13

[Fuzzing Team]

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

Comment 14

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

=== Treeherder Build Bisection Results by autoBisect ===

The "bad" changeset has the timestamp "20160219133522" and the hash "1205efecce10f87c04a9bf2bfb91c6b5cf5f2239".
The "good" changeset has the timestamp "20160219134321" and the hash "2feba844e67bbf6dddec9578a171b95ee896dfea".

Likely fix window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1205efecce10f87c04a9bf2bfb91c6b5cf5f2239&tochange=2feba844e67bbf6dddec9578a171b95ee896dfea

Shu-yu, is bug 912337 a likely fix?

Comment 15

[Shu-yu Guo [:shu]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #14)
> === Treeherder Build Bisection Results by autoBisect ===
> 
> The "bad" changeset has the timestamp "20160219133522" and the hash
> "1205efecce10f87c04a9bf2bfb91c6b5cf5f2239".
> The "good" changeset has the timestamp "20160219134321" and the hash
> "2feba844e67bbf6dddec9578a171b95ee896dfea".
> 
> Likely fix window:
> https://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=1205efecce10f87c04a9bf2bfb91c6b5cf5f2239&tochange=2feb
> a844e67bbf6dddec9578a171b95ee896dfea
> 
> Shu-yu, is bug 912337 a likely fix?

Not a real fix, but hid the crash. The fuzzers have a habit of generating Debugger tests like

var dbg = newGlobal().Debugger(this);
dbg.onSomething = () => { ... };

Note that the Debugger instance is from a newGlobal() and its hook handler is from the current global. With bug 912337, triggering 'onSomething' will always throw since we no longer allow re-entering debuggee code (the current global) from debugger code (the newGlobal()).

Could the fuzzers instead generate code more like the following?

var g = newGlobal();
var dbg = new Debugger(g);
dbg.onSomething = () => { ... };
g.eval(`foo`);

Comment 16

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

Debugger code isn't something that jsfunfuzz currently generates, while the other fuzzers make use of existing tests and move things around, so it might have been that this was adapted from existing tests.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/193a242cef54
https://hg.mozilla.org/integration/mozilla-inbound/rev/f788142ec96f

Comment 18

[Mike Hommey [:glandium]]

(In reply to Pulsebot from comment #17)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f788142ec96f

Why is this change attached to this bug? Moreover, that's not enough. --with-nspr-exec-prefix, --with-nss-prefix and --with-nss-exec-prefix need to be added too.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/193a242cef54
https://hg.mozilla.org/mozilla-central/rev/f788142ec96f

Comment 20

[Nicolas B. Pierron [:nbp]]

[Tracking Requested - why for this release]:
Note, this feature was an unused feature of the Debugger, I suggest we change the status of 47 to wontfix.

PS: This do not seems like a good idea for me to set the status to wontfix without knowledge from the RelMngt team, but it seems like a good idea to do it such that we do not panic later in the cycle because the number of remaining issues is too high.

Comment 21

[Jim Blandy :jimb]

Nicolas, I think you accidentally cleared the needinfo without answering glandium's question in comment 18.

Comment 22

[Nicolas B. Pierron [:nbp]]

(In reply to Mike Hommey [:glandium] from comment #18)
> (In reply to Pulsebot from comment #17)
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/f788142ec96f
> 
> Why is this change attached to this bug? Moreover, that's not enough.
> --with-nspr-exec-prefix, --with-nss-prefix and --with-nss-exec-prefix need
> to be added too.

Because otherwise I would have committed it with "no bug", and will would not have room for discussion about it.

And I committed it as part of this bug, because this was blocking me from testing this patch after a rebase.

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Nicolas, are you saying that this crash will not be hit on Fx47? Or is the likelihood of that very low and the risk associated with the fix not worth uplifting? Just want to make sure I understand it right before wontfix'g.

Comment 24

[Nicolas B. Pierron [:nbp]]

(In reply to Ritu Kothari (:ritu) from comment #23)
> Hi Nicolas, are you saying that this crash will not be hit on Fx47? Or is
> the likelihood of that very low and the risk associated with the fix not
> worth uplifting? Just want to make sure I understand it right before
> wontfix'g.

This crash will not hit 47, nor any of the earlier version.

This feature is only exposed to the Debugger API, and was not used by anybody.

Comment 25

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Nicolas B. Pierron [:nbp] from comment #24)
> (In reply to Ritu Kothari (:ritu) from comment #23)
> > Hi Nicolas, are you saying that this crash will not be hit on Fx47? Or is
> > the likelihood of that very low and the risk associated with the fix not
> > worth uplifting? Just want to make sure I understand it right before
> > wontfix'g.
> 
> This crash will not hit 47, nor any of the earlier version.
> 
> This feature is only exposed to the Debugger API, and was not used by
> anybody.

Ok. Thanks! Unaffected for Fx47 then.