1015339 - Dumping a JS stack crashes with ion code on the stack

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Boris Zbarsky [:bzbarsky]]

<jandem-away> jesup: yeah, the problem is that FormatFrame is bogus for Ion frames
<jandem-away> bz: well it's the iter.computeThis in FormatFrame; that doesn't work
for Ion frames

Comment 1

[Jan de Mooij [:jandem]]

FormatFrame calls FrameIter::computeThis() and since bug 716647 part 3 that function will now assert under FrameIter::abstractFramePtr() instead of returning early for Ion frames.

This should be reproducible in the browser by calling DumpJSStack() in gdb, but it'd be nice if we had a shell function for that too, so that the fuzzers would have caught this a long time ago.

Comment 2

[Nicolas B. Pierron [:nbp]]

FYI, I added js_DumpBacktrace(cx), and it can be used from gdb.

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Hacking_Tips#Printing_the_JS_stack_%28from_gdb%29

Comment 3

[Boris Zbarsky [:bzbarsky]]

The typical situation in Gecko code is that we don't have a cx in the frame where we want the stack.  DumpJSStack() has the benefit of locating the right cx (usually).

Should DumpJSStack() be calling js_DumpBacktrace instead of the APIs it calls now?

Comment 4

[Shu-yu Guo [:shu]]

Attachment: Spot fix DumpJSStack.

The easiest fix is just to rematerialize Ion frames when trying to dump them.

I don't know what people want to do about js_DumpBacktrace and adding a
DumpJSStack for the shell, so I didn't do anything about that.

js_DumpBacktrace seems like it should be preferred for simple backtraces.

DumpJSStack though, with its ability to print locals and args and this props
and all that jazz, is a different animal. FormatFrame should be rewritten to
operate on AbstractFramePtrs, but I don't have time to do that at the moment.

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8429757 [details] [diff] [review]
Spot fix DumpJSStack.

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

I think it's simpler to skip the computeThis call for Ion frames.

Rematerializing the frames feels too heavy-weight to get a stack trace in gdb.

Comment 6

[Shu-yu Guo [:shu]]

(In reply to Jan de Mooij [:jandem] from comment #5)
> Comment on attachment 8429757 [details] [diff] [review]
> Spot fix DumpJSStack.
> 
> Review of attachment 8429757 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think it's simpler to skip the computeThis call for Ion frames.
> 
> Rematerializing the frames feels too heavy-weight to get a stack trace in
> gdb.

That's the dichotomy between js_DumpBacktrace and DumpJSStack I was talking about. js_DumpBacktrace is nice and simple for just a stack trace. DumpJSStack on the other hand prints all this detailed info. If speed is a concern, I say call js_DumpBacktrace.

Comment 7

[Jan de Mooij [:jandem]]

OK, but then we definitely need a shell function for this.

That may be a good idea anyway, so that the fuzzers can hammer the materialize-Ion-frames code without entering debug mode.

Comment 8

[Shu-yu Guo [:shu]]

Boris, how used to the "DumpJSStack" name are the Gecko devs? What do you think about the following rename?

js_DumpBacktrace -> js_DumpSimpleBacktrace
DumpJSStack -> js_DumpDetailedBacktrace

Comment 9

[Boris Zbarsky [:bzbarsky]]

> how used to the "DumpJSStack" name are the Gecko devs?

Good question.  I expect most of them use it via something like https://wiki.mozilla.org/Gecko:Debugging_Tools#Dumping_the_JS_Stack or the equivalent bit in the in-tree .gdbinit

So renaming would probably work if you just updated the .gdbinit.

But why bother?  We can introduce a js_DumpDetailedBacktrace and just have the existing DumpJSStack() call it, right?

Comment 10

[Shu-yu Guo [:shu]]

(In reply to Boris Zbarsky [:bz] from comment #9)
> > how used to the "DumpJSStack" name are the Gecko devs?
> 
> Good question.  I expect most of them use it via something like
> https://wiki.mozilla.org/Gecko:Debugging_Tools#Dumping_the_JS_Stack or the
> equivalent bit in the in-tree .gdbinit
> 
> So renaming would probably work if you just updated the .gdbinit.
> 
> But why bother?  We can introduce a js_DumpDetailedBacktrace and just have
> the existing DumpJSStack() call it, right?

Yeah, certainly can.

Comment 11

[Shu-yu Guo [:shu]]

Attachment: Spot fix FormatStackDump.

A simpler lighterweight spotfix.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8447351 [details] [diff] [review]
Spot fix FormatStackDump.

r=me

Comment 13

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/23b4075fe10a

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/23b4075fe10a

Comment 15

[Boris Zbarsky [:bzbarsky]]

This is still crashing in exactly the same function and in the same exact way, but with a different operation on the frame iterator.  Now it's:

  arg = iter.unaliasedActual(i, DONT_CHECK_ALIASING);

leading to:

  MOZ_ASSERT(hasUsableAbstractFramePtr());

Comment 16

[Boris Zbarsky [:bzbarsky]]

Naveed, can we find someone to fix this, please?

Comment 17

[Jan de Mooij [:jandem]]

Patch today or tomorrow. This is exactly why I repeatedly asked for a shell function :(

Comment 18

[Steve Fink [:sfink] [:s:]]

Attachment: imported patch shell-backtrace2

This is not that shell function, but it's what I have lying around in my queue. It calls js_DumpBacktrace. For this bug, I think you want FormatStackDump.

Comment 19

[Steve Fink [:sfink] [:s:]]

Attachment: Add functions for getting and dumping the current backtrace

I suppose while I'm here, I ought to implement something. r? Waldo because I figure he's likely to object to my hacky interface.

I don't know how to test this properly. I'll attach a very basic test file that doesn't crash.

I will probably not be around to finish this up, so someone else should take this if it's useful.

(And I need to unassign myself from this bug.)

Comment 20

[Steve Fink [:sfink] [:s:]]

Attachment: Useless test case

Comment 21

[Daniel Trebbien]

Could this change be backported to Firefox 31 ESR when fixed?

Yesterday I was trying to troubleshoot an issue where a website that I am working on crashes Firefox 31.0. (It runs fine in Firefox 30.0.) Due to this issue, I cannot print the JS stack trace in gdb to know what is causing the crash.

I have tested Firefox 32 builds and the issue is fixed in that branch. Normally I would just wait for FF32 to be released (currently scheduled for September 2), but much to my dismay, I discovered that Firefox 31.0 was the branch point for a new ESR line, and I am experiencing the same crash in Firefox 31.0 ESR.

Comment 22

[Nicolas B. Pierron [:nbp]]

(In reply to Daniel Trebbien from comment #21)
> Yesterday I was trying to troubleshoot an issue where a website that I am
> working on crashes Firefox 31.0. (It runs fine in Firefox 30.0.) Due to this
> issue, I cannot print the JS stack trace in gdb to know what is causing the
> crash.

Have you tried js_DumpBacktrace(cx) (comment 2)?

Comment 23

[Jan de Mooij [:jandem]]

Steve, thanks for the patch to add the testing functions! Will see if I can repro with that.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> Have you tried js_DumpBacktrace(cx)

Typically this is needed in Gecko code which may not have a cx.  DumpJSStack finds the right one for you as needed.

Comment 25

[Jan de Mooij [:jandem]]

Attachment: Fix + test

Requesting review from Waldo as he's also reviewing the other patch (on which this patch depends).

Patch uses MagicValue(JS_OPTIMIZED_OUT) if we can't get the argument (when the frame is an Ion frame), and then displays it as "[unavailable]". As this is just a debugging function, I don't want to spend too much time on it.

I verified the jit-test asserted with --ion-eager --ion-offthread-compile=off and the fuzzers can use this test to know about these new functions.

Comment 26

[Daniel Trebbien]

(In reply to Nicolas B. Pierron [:nbp] from comment #22)
> (In reply to Daniel Trebbien from comment #21)
> > Yesterday I was trying to troubleshoot an issue where a website that I am
> > working on crashes Firefox 31.0. (It runs fine in Firefox 30.0.) Due to this
> > issue, I cannot print the JS stack trace in gdb to know what is causing the
> > crash.
> 
> Have you tried js_DumpBacktrace(cx) (comment 2)?

I did. In lldb, I get:

error: warning: Stopped in a C++ method, but 'this' isn't available; pretending we are in a generic context
error: use of undeclared identifier 'js_DumpBacktrace'
error: 1 errors parsing expression

When I use gdb compiled from source (http://stackoverflow.com/questions/20553784/gdb-on-mac-10-9-fails-with-not-in-executable-format-file-format-not-recognized/24918436#24918436)
the `cx' variable is not available:

(gdb) bt
#0  js::ObjectImpl::numFixedSlots () at jsfriendapi.h:563
#1  js::shadow::Object::numFixedSlots () at :668
#2  js::CallObject::aliasedVarFromArguments () at :572
#3  0x00000001030db59a in ArgGetter (cx=<value temporarily unavailable, due to optimizations>) at /builds/slave/rel-m-rel-osx64_bld-0000000000/build/js/src/vm/ArgumentsObject.cpp:308
#4  0x000000005fbe43f0 in ?? ()
Previous frame inner to this frame (gdb could not unwind past this frame)
Current language:  auto; currently c++
(gdb) print cx
$1 = <value temporarily unavailable, due to optimizations>

Comment 27

[Jan de Mooij [:jandem]]

Waldo do you think you can review these patches this week or should I ask somebody else?

Comment 28

[Jan de Mooij [:jandem]]

Comment on attachment 8461516 [details] [diff] [review]
Fix + test

More than two weeks => switching review to shu.

Comment 29

[Steve Fink [:sfink] [:s:]]

Pushed with API change suggested by Waldo. You now have to do getBacktrace({ thisprops: true }) instead of getBacktrace("thisprops").

https://hg.mozilla.org/integration/mozilla-inbound/rev/549b4ef82544

Comment 30

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/549b4ef82544

Comment 31

[Daniel Trebbien]

Which versions of Firefox will include this change?  Is it going to be included in the next Firefox 31.0 ESR release?

Comment 32

[Daniel Trebbien]

This morning I tried out the web site where I was seeing the crash in Firefox 31.0 and 31.0esr, now using Firefox 31.1.0esr, and I still see the crash.  Unfortunately, I haven't been able to prepare a minimal test case.  I have isolated a particular line of JavaScript that "fixes" the crash when commented out, but this does not indicate where the crash might be occurring because the line enables a lot of other code, and attaching the JS debugger fixes the issue.

Attaching to the Firefox process in lldb, I see:

> (lldb) p (void)DumpJSStack()
> error: Error resuming inferior the 2 time: "Resume timed out.".
> Couldn't execute function; result was eExecutionSetupError

.. at the EXC_BAD_ACCESS.

Using gdb I see:

> (gdb) print (void)DumpJSStack()
> 
> Program received signal EXC_BAD_ACCESS, Could not access memory.
> Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000020
> 0x00000001031247d3 in js_CallContextDebugHandler ()
> The program being debugged was signaled while in a function called from GDB.

Comment 33

[Jan de Mooij [:jandem]]

Part 2:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4307416e17ed

Sorry for the delay, forgot to push this sooner.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4307416e17ed