js::DumpBacktrace doesn't need a Sprinter

RESOLVED FIXED in Firefox 57

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Yoric, Assigned: sourav.mukherjee619, Mentored)

Tracking

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [good first bug][js:tech-debt])

Attachments

(1 attachment)

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.
Whiteboard: [good first bug]
(Assignee)

Comment 1

2 years ago
I want to take this one.
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 :)
Ok, we can now proceed. Are you ready to go, Sourav, or do you have any question?
Flags: needinfo?(sourav.mukherjee619)
(Assignee)

Comment 4

2 years ago
Yoric, I am ready to go. :)
Flags: needinfo?(sourav.mukherjee619)
Comment hidden (mozreview-request)

Comment 6

2 years ago
mozreview-review
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 hidden (mozreview-request)
(Reporter)

Comment 8

2 years ago
mozreview-review
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 hidden (mozreview-request)
(Reporter)

Comment 10

2 years ago
mozreview-review
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.
Assignee: nobody → sourav.mukherjee619

Comment 11

2 years ago
mozreview-review
Comment on attachment 8907678 [details]
Bug 1397856 replacing Sprinter with js::GenericPrinter

https://reviewboard.mozilla.org/r/179348/#review184970
Attachment #8907678 - Flags: review?(nicolas.b.pierron) → review+
(Assignee)

Comment 12

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

checkin-needed
(Assignee)

Updated

2 years ago
Attachment #8907678 - Flags: checkin?(dteller)
(Reporter)

Comment 13

2 years ago
mozreview-review
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.
Attachment #8907678 - Flags: review?(dteller) → review+

Comment 14

2 years ago
Pushed by dteller@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e1cdf9566f46
replacing Sprinter with js::GenericPrinter r=nbp,Yoric
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.
Attachment #8907678 - Flags: checkin?(dteller)
Priority: -- → P3
Whiteboard: [good first bug] → [good first bug][js:tech-debt]
https://hg.mozilla.org/mozilla-central/rev/e1cdf9566f46
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.