894854 - Add env var that disables censoring of self-hosted script frames

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Felix S. Klock II [:pnkfelix, :fklock]]

We currently take pains to skip over the frames from self-hosted javascript code when building up stack traces.

That makes sense for inter-operation with tools that want to inspect the stack, where we want to hide our internal implementation details.

But when one is debugging the self-hosted code itself, skipping over those frames is throwing out useful information.

So we should provide some way for that information to be reported; not by default, but when used in tandem with some combination of:

* A debug-build of spidermonkey, and/or

* A shell build, and/or

* Some environment variable setting.

Comment 1

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch A v1: DEBUG guard alone

This is probably not what we actually want.  I know I'm inviting a bike shed discussion, but since I don't actually care what constraints we add to this "functionality", go ahead, pile on what shades of paint you want to see here.

Comment 2

[Till Schneidereit [:till]]

Sorry, should've filed this yesterday.

We can't make it hinge on debug mode, as that would make testing of at least some addons (such as GreaseMonkey) impossible. And in general, would be a pretty severe change of behavior between release and debug.

Enabling it only in the shell would work, but it would also be pretty unfortunate: there'd be no way to debug self-hosting issues that one can reproduce only in the browser. Plus, again, introduce a noticeable difference in behavior between two environments: the shell and the browser, in this case. We already have too many of those.

Thus, an env var is probably the way to go.

Comment 3

[Till Schneidereit [:till]]

Comment on attachment 777034 [details] [diff] [review]
patch A v1: DEBUG guard alone

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

Clearing feedback based on the previous comment.

Comment 4

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch A v2: env var guard alone

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch A v3: cached env var and debug guard both

Not asking for review yet because this is not tested beyond checking it compile; just checkpointing here; will sanity-check later today.

Comment 6

[Till Schneidereit [:till]]

Comment on attachment 777092 [details] [diff] [review]
patch A v3: cached env var and debug guard both

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

I know you didn't ask for a review, but if your testing shows this to work as expected, I'm fine with it.

My only quibble is the long name for the env var: I'm 100% certain I won't be able to remember that. How about MOZ_SHOW_SH_FRAMES, or even just SHOW_SH_FRAMES? (Little danger of that colliding with anything, I'd say.)

Comment 7

[Felix S. Klock II [:pnkfelix, :fklock]]

till notes that it would be good to update this page:

  https://developer.mozilla.org/en-US/docs/SpiderMonkey/Internals/self-hosting

in tandem with landing any patch for this bug.

Comment 8

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch A v4: cached env var and debug guard both

inherited r+ from till.

a bit of bikeshedding in pjs: http://logbot.glob.com.au/?c=mozilla%23pjs#c35581
yielded the environment variable name that we settled on: MOZ_SHOW_ALL_JS_FRAMES

Comment 9

[Felix S. Klock II [:pnkfelix, :fklock]]

try run:

try: -b do -p all -u all[x86] -t none

  https://tbpl.mozilla.org/?tree=Try&rev=5368efc761a4

(all green)

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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