1421104 - De-COM nsIXPConnect::DebugPrintJSStack

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement, P3)

Description

[Andrew McCreight [:mccr8]]

This is only called from a single place, inside XPConnect, and doesn't directly access any fields of nsXPConnect, so I think it makes sense to inline this. My ulterior motive for this is that I want to add a slightly lower level public helper method to use in bug 1420975.

Comment 1

[Andrew McCreight [:mccr8]]

I don't actually need for this bug 1420975.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: Bug 1421104 - De-COM nsIXPConnect::DebugPrintJSStack.

This is only called in a single place, and does not use any fields of
nsXPConnect, so remove it from the XPIDL interface and inline the
method to the one caller. It also fixes an error message to no longer
refer to a non-existant interface. Otherwise, the behavior should be
unchanged.

Review commit: https://reviewboard.mozilla.org/r/204024/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/204024/

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8933061 [details]
Bug 1421104 - De-COM nsIXPConnect::DebugPrintJSStack.

https://reviewboard.mozilla.org/r/204024/#review209588

I'd like to understand more about this API before I r+ it.

::: js/xpconnect/src/nsXPConnect.cpp:1218
(Diff revision 1)
>  JS_EXPORT_API(void) DumpJSStack()
>  {
>      xpc_DumpJSStack(true, true, false);
>  }
>  
>  JS_EXPORT_API(char*) PrintJSStack()

Out of curiosity, who is supposed to be using this API? It's extremely odd to be returning a raw pointer in this day and age (any reason not to return a UniqueChars from the newly de-COM-taminated interface?).

Also, why does this do a raw printf if there's no JS context given that this function is returning a representation of the stack? Wouldn't it be better to return that in a string?

Comment 4

[Andrew McCreight [:mccr8]]

(In reply to Blake Kaplan (:mrbkap) from comment #3)
> Out of curiosity, who is supposed to be using this API? It's extremely odd
> to be returning a raw pointer in this day and age (any reason not to return
> a UniqueChars from the newly de-COM-taminated interface?).

It looks like it was added in bug 600304 to be used from gdb on an Android phone, because the debugger would print the return value but there was no ready access to stdout. In that context, we really want to just return char*. Note that nobody calls this method in our code base.

> Also, why does this do a raw printf if there's no JS context given that this
> function is returning a representation of the stack? Wouldn't it be better
> to return that in a string?

That's a good point. Thought I suppose you'd be able to tell given a null string, but it is more obscure.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8933061 [details]
Bug 1421104 - De-COM nsIXPConnect::DebugPrintJSStack.

https://reviewboard.mozilla.org/r/204024/#review209890

Comment 6

[Andrew McCreight [:mccr8]]

Comment on attachment 8933061 [details]
Bug 1421104 - De-COM nsIXPConnect::DebugPrintJSStack.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204024/diff/1-2/

Comment 7

[Andrew McCreight [:mccr8]]

I changed the return value for the failure case to return the string instead of printing it. I also had to change the return type to const char *.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dbc85c882639
De-COM nsIXPConnect::DebugPrintJSStack. r=mrbkap

Comment 9

[Raul Gurzau (:RaulG)]

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

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Andrew McCreight [:mccr8] from comment #7)
> I changed the return value for the failure case to return the string instead
> of printing it. I also had to change the return type to const char *.

At this point we should probably either add a comment that this function is intended to leak the return value or a comment to be clear that this function is only intended to be used from a debugger. Even in debug-only stuff, it makes me nervous to see a function returning both malloc'd memory and string literals.

Alternatively, maybe strdup the constant string?

Comment 11

[Andrew McCreight [:mccr8]]

I replied in bug 1422432.