1397856 - js::DumpBacktrace doesn't need a Sprinter

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[David Teller [:Yoric] - still alive but not very active]

In jsobj.cpp, method `js::DumpBacktrace` uses a `Sprinter` to generate a string, then passes this string to a `js::GenericPrinter`. This used to be useful when this method used a `FILE*`, but now that bug 1397717 has introduced the `js::GenericPrinter`, we could simply reuse the generic printer.

The objective of this bug is to cleanup method `js::DumpBacktrace` to get rid of the `Sprinter` and use the `GenericPrinter` everywhere instead.

Comment 1

[sourav.mukherjee619]

I want to take this one.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Sounds good. I just forgot to land the previous patch, so we'll have to wait until tomorrow (normally) until you can actually get to work on it. Sorry about that, I put "[good first bug]" a bit too early :)

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Ok, we can now proceed. Are you ready to go, Sourav, or do you have any question?

Comment 4

[sourav.mukherjee619]

Yoric, I am ready to go. :)

Comment 5

[sourav.mukherjee619]

Attachment: Bug 1397856 replacing Sprinter with js::GenericPrinter

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

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8907678 [details]
Bug 1397856 replacing Sprinter with js::GenericPrinter

https://reviewboard.mozilla.org/r/179348/#review184498

::: js/src/jsobj.cpp:3822
(Diff revision 1)
>          }
>      }
> -    out.printf("%s", sprinter.string());
>  #ifdef XP_WIN32
>      if (IsDebuggerPresent())
> -        OutputDebugStringA(sprinter.string());
> +        OutputDebugStringA(out.string());

This is not implemented for all GenericPrinter-s, I think we can just remove this line, or move it to another variant of DumpBacktrace function whch call this function with an Sprinter as argument.

Comment 7

[sourav.mukherjee619]

Comment on attachment 8907678 [details]
Bug 1397856 replacing Sprinter with js::GenericPrinter

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

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8907678 [details]
Bug 1397856 replacing Sprinter with js::GenericPrinter

https://reviewboard.mozilla.org/r/179348/#review184546

::: js/src/jsobj.cpp:3791
(Diff revision 2)
>  }
>  
>  JS_FRIEND_API(void)
>  js::DumpBacktrace(JSContext* cx, js::GenericPrinter& out)
>  {
> -    Sprinter sprinter(cx, false);
> +    

Nit: Could you avoid these lines with empty spaces?

::: js/src/jsobj.cpp
(Diff revision 2)
>          } else {
> -            sprinter.printf(" (%p)\n", i.pc());
> +            out.printf(" (%p)\n", i.pc());
>          }
>      }
> -    out.printf("%s", sprinter.string());
> +
> -#ifdef XP_WIN32

Oh, I hadn't noticed that `OutputDebugStringA`. Let me think a second.

Comment 9

[sourav.mukherjee619]

Comment on attachment 8907678 [details]
Bug 1397856 replacing Sprinter with js::GenericPrinter

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

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8907678 [details]
Bug 1397856 replacing Sprinter with js::GenericPrinter

https://reviewboard.mozilla.org/r/179348/#review184906

Looks good to me. Let's run this through our test suite.

::: js/src/jsobj.cpp
(Diff revision 3)
>      }
> -    out.printf("%s", sprinter.string());
> +
> -#ifdef XP_WIN32
> -    if (IsDebuggerPresent())
> -        OutputDebugStringA(sprinter.string());
> -#endif

Ok, after checking what this line was introduced, I confirm that we can just get rid of it. I'll open a followup bug to reintroduce the feature in a more robust way.

Comment 11

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8907678 [details]
Bug 1397856 replacing Sprinter with js::GenericPrinter

https://reviewboard.mozilla.org/r/179348/#review184970

Comment 12

[sourav.mukherjee619]

Comment on attachment 8907678 [details]
Bug 1397856 replacing Sprinter with js::GenericPrinter

checkin-needed

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8907678 [details]
Bug 1397856 replacing Sprinter with js::GenericPrinter

https://reviewboard.mozilla.org/r/179348/#review185038

Tests are good, let's land this.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e1cdf9566f46
replacing Sprinter with js::GenericPrinter r=nbp,Yoric

Comment 15

[David Teller [:Yoric] - still alive but not very active]

Thank you very much for the patch.

If you're interested, I have opened followup bug 1399777, which is about restoring the debugging feature we have just removed, just in more situations.

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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