703519 - Components.stack is providing wrong stack trace

Status: RESOLVED WORKSFORME

(Core :: XPConnect, defect)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Firebug Console panel is using JSD to get proper stack frames, which is in some cases more precise than just climbing Components.stacks.

Follow this test case:
http://getfirebug.com/tests/content/branches/1.9/console/4658/issue4658.html

... and compare Firebug Console panel result with Web Console (built in) result.

- Firebug is correctly showing: FirebugBug.js (line 6) - based on JSD 
- Web Console is showing: dojo.js.uncompressed.js:510 - based on Components.stack

Could we fix Components.stack so, Web Console works and Firebug Console panel doesn't need to activate JSD (performance penalty) to get correct stack frames?

Honza

Comment 1

[Luke Wagner [:luke]]

Out of curiosity, is the code in question consuming all the stack frames or just the top frame?  I ask because we recently started capping the max depth of the stack traversed by Console.stack (since it was hanging browsers with deep stacks).  In general, Console.stack is really expensive, especially considering that most sites just want Console.stack[0] or [1].

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Luke Wagner [:luke] from comment #1)
> Out of curiosity, is the code in question consuming all the stack frames or
> just the top frame?
The top frame is only important in this particular case (console.log),
but in case of e.g. errors (console.error) - Firebug is displaying full stack trace.

> I ask because we recently started capping the max depth
> of the stack traversed by Console.stack (since it was hanging browsers with
> deep stacks).  In general, Console.stack is really expensive, especially
> considering that most sites just want Console.stack[0] or [1].
Console.stack? Do you mean Components.stack?

Honza

Comment 3

[Luke Wagner [:luke]]

(In reply to Jan Honza Odvarko from comment #2)
> Console.stack? Do you mean Components.stack?

Oops, yes.

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Any change we could get this one fixed?
Honza

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Could we set target milestone for this problem? Fx 15:
Honza

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Any chance we could get this bug higher priority?
Could we fix it as part of the "remove JSD1" effort? 

Honza

Comment 7

[Luke Wagner [:luke]]

Based on a quick look, Components.stack calls nsXPCComponents::GetStack which uses JS::DescribeStack which does proper stack iteration.  I didn't step through the STR, but based on the results in comment 0, I think we are just iterating the stack of the wrong JSContext.  The JSContext is extracted by GetCurrentJSContext() whereas, iirc, jsd is a lot more explicit about JSContext (which I never actually understood).

Perhaps jimb or bholley could look at this?

Comment 8

[Bobby Holley (:bholley)]

Yeah, what luke says sounds right.

Honza, can you clarify what you're trying to do here and why this is a blocker for removing JSD1? JSD1 does all kinds of crazy stuff with the JSContext (see bug 871306 for example), but I was hoping we could just let it die rather than messing with it too much. ;-)

If we wanted to fix this, we probably need to push a cx somewhere in JSD. I don't know the code well enough to have much of an idea of where to do that, though.

Does the same problem happen with JSD2?

Comment 9

[Luke Wagner [:luke]]

Bobby: we have the ability to enumerate *everything* live on the stack (regardless of JSContext), which we could provide (perhaps by a bool allTheThings param to JS::DescribeStack).  Do you think that would fix the problem and be generally suitable for Components.stack?

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Luke Wagner [:luke] from comment #9)
> Bobby: we have the ability to enumerate *everything* live on the stack
> (regardless of JSContext), which we could provide (perhaps by a bool
> allTheThings param to JS::DescribeStack).

(Note that AllFramesIter currently does not support inlined (Ion-)frames. However, as part of the stack overhaul my plan is to merge it with ScriptFrameIter since the difference between the two will be very small: skip activations from other contexts or don't.)

Comment 11

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] from comment #9)
> Bobby: we have the ability to enumerate *everything* live on the stack
> (regardless of JSContext), which we could provide (perhaps by a bool
> allTheThings param to JS::DescribeStack).  Do you think that would fix the
> problem and be generally suitable for Components.stack?

Whoa, what? How do we do that? I think stacks were per-JSContext.

That sounds like it maps reasonably well to the cx-less world, so if it deals with interleaving and all that, maybe we should give that a try.

Comment 12

[Luke Wagner [:luke]]

There is only one actual stack per JSRuntime (rt->stackSpace); all JSContext execution interleaves in a LIFO manner and threads through the same rt->stackSpace.  The normal ScriptFrameIter iterates within a context, but the AllFramesIter walks over the raw rt->stackSpace.  Jan was pointing out some of the current limitations of AllFramesIter, though.

> That sounds like it maps reasonably well to the cx-less world

Definitely; the multi-cx world is just a complication layered on top.

Comment 13

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Bobby Holley (:bholley) (PTO thurs/fri) from comment #8)
> Honza, can you clarify what you're trying to do here and why this is a
> blocker for removing JSD1? JSD1 does all kinds of crazy stuff with the
> JSContext (see bug 871306 for example), but I was hoping we could just let
> it die rather than messing with it too much. ;-)

I have been looking at the provided test case again (comment #0) and noticed that it involves @sourceURL directive, which is additionally processed by Firebug. It explains the difference between Firebug Console panel and Web Console.

I have been looking for a better test case and haven't found one so far. Ao, I agree that it shouldn't be a blocker for JSD1 removal (at least till there is a proper test case).

Honza

Comment 14

[Sebastian Zartner [:sebo]]

This looks fixed to me in Firefox 41.0. The console displays 'FirebugBug.js:6:9'.

According to mozregression it was fixed somewhere in this push log (couldn't find the related bug, though):
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a280a03c9f3c&tochange=ae1dfa192faf

Sebastian