952612 - Preserve breakpoint when prettyprinting

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Codacoder]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0 (Beta/Release)
Build ID: 20131112160018

Steps to reproduce:

1 - Set a break point in loaded source
2 - Click Prettify

Affected: FF26, 27, 28, 29





Actual results:

In FF27, 28, 29, the breakpoint moved.

In FF26 it was deleted.


Expected results:

Breakpoints should be (re)assigned to new renumbered line or assigned a "logical" line that will survive Prettify/Unprettify.

Comment 1

[Girish Sharma [:Optimizer]]

Are there any plans to change the location of the breakpoint. We already have sourcemapped information, how about moving the breakpoint to the corresponding location ?

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Girish Sharma [:Optimizer] from comment #1)
> Are there any plans to change the location of the breakpoint. We already
> have sourcemapped information, how about moving the breakpoint to the
> corresponding location ?

We would need to iterate over all breakpoints in the source being prettified[0] and run them through the new source map[1] so that the pretty print RDP response[2] includes a list of new locations for every breakpoint. Then the frontend would have to move the existing breakpoint icons to the new locations when it gets the pretty print response[3].

I don't have any plans on working on this in the near future since 100% of my time is dedicated to memory tooling at the moment.

Happy to mentor anyone who wants to work on this, though.

General devtools hacking info: https://wiki.mozilla.org/DevTools/Hacking

[0] http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#178
[1] http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#4670
[2] http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#2494
[3] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-panes.js#433

Comment 3

[Deary Hudson jr.]

(In reply to Nick Fitzgerald [:fitzgen] from comment #2)
> (In reply to Girish Sharma [:Optimizer] from comment #1)
> > Are there any plans to change the location of the breakpoint. We already
> > have sourcemapped information, how about moving the breakpoint to the
> > corresponding location ?
> 
> We would need to iterate over all breakpoints in the source being
> prettified[0] and run them through the new source map[1] so that the pretty
> print RDP response[2] includes a list of new locations for every breakpoint.
> Then the frontend would have to move the existing breakpoint icons to the
> new locations when it gets the pretty print response[3].
> 
> I don't have any plans on working on this in the near future since 100% of
> my time is dedicated to memory tooling at the moment.
> 
> Happy to mentor anyone who wants to work on this, though.
> 
> General devtools hacking info: https://wiki.mozilla.org/DevTools/Hacking
> 
> [0]
> http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> script.js#178
> [1]
> http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> script.js#4670
> [2]
> http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> script.js#2494
> [3]
> http://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/
> debugger-panes.js#433


@fitgen, I'd like to start working on this bug.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Thanks and good luck!

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Feel free to ask any questions of me or anyone else on IRC, the mailing list, or in this bug!

Comment 6

[Deary Hudson jr.]

fitzgen, so the goal here is to

1) Iterate through breakpoints being prettified 
( scripts line #178: findBreakPoints(...) )

2) Iterate the breakpoints through the sourcemap, which returns a pretty-print response with the new locations in the javascript source. 
( scripts.js line #4677 getOriginalLocation(...) )

3) Produce the correct line numbers from the pretty print response 
( scripts.js line#2501 onPrettyPrint(...) )

4) Then iterate through the pretty print response the new locations of the breakpoints, and render them on the front end.
(debugger-pane.js line #435)

Comment 7

[Codacoder]

And (if I may interject) if prettify is clicked *again*, revert to the original "ugly" line number(s).

Comment 8

[Deary Hudson jr.]

in doing that, all breakpoints (old & new) would have to be reiterated through the generated source map for it's correct line number in the minified file.

Comment 9

[Codacoder]

I'm not sure what you're saying...

There isn't necessarily a minified file. Prettify potentially changes the layout of any file in whatever state it was originally found, minified or not.  Therefore, unprettify (when prettify is clicked a second time) should put the file back to its original "ugly" state.

The bug here is that Prettify is not taking into account previously set breakpoints.  Likewise, unprettify. If Prettify cannot be used in conjunction with previously set breakpoints, offer a warning before blowing away my breakpoints.  Either that, or change the way the button behaves - atm it behaves like a toggle - which doesn't work (with breakpoints).

okay?

Comment 10

[Deary Hudson jr.]

Okay, I'm able to re-produce the bug. After I place breakpoints in the debugger, then press prettify, the breakpoints are ignored and not placed in their correct (or logical) place in the prettified source. Pressing prettify again (a second time), the breakpoints are ignored again and maintain their current positions both before/after prettify was pressed.

In the case that prettify is clicked a second time, you are correct that prettify is ignoring the breakpoints on that event as well. unless there is a way to logically find the correct breakpoint line numbers going back and forth from  "pretty" to "ugly" source, a warning message should be displayed that the breakpoints will not work in a prettified source, then blowing them away.

Comment 11

[Codacoder]

Right... now we're on the same page.  Ask Nick for his input - I don't know the source at all... he may have other ideas how it might best be handled.

Comment 12

[Deary Hudson jr.]

Right now the devtools team is out on a meeting in Portland. they'll be on irc tomorrow, so I'll shout and get in touch with one of the guys.

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

You're both right. My proposed solution in comment 2 was to move the existing breakpoints to the corresponding location in the prettified source rather than displaying a warning and removing all the breakpoints. I think this is a better user experience than removing the breakpoints a user meticulously set.

Deary, comment 6 is the right track. On top of what you mentioned, note that the moved breakpoints should be listed in the "prettyPrint" and "unPrettyPrint" RDP responses.

Comment 14

[Deary Hudson jr.]

fitz: I've set some logs on the prettified source after the source has been prettified, and when breakpoints are set (regardless of line number)the numbers do not reflect where they are being set on the prettified code in findBreakpoints(...)

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Deary Hudson jr. from comment #14)
> fitz: I've set some logs on the prettified source after the source has been
> prettified, and when breakpoints are set (regardless of line number)the
> numbers do not reflect where they are being set on the prettified code in
> findBreakpoints(...)

Hey Deary, sorry I missed this comment :(

In the future, setting "need more information from" to me should help make sure I reply. Sorry, again.

Are you still interested in working on this bug? I'm not sure I follow exactly what you mean in comment 14.

Comment 17

[Deary Hudson jr.]

@Fitz, sorry I missed your comment. At this point, I am unable to finish working on this bug due to work related projects that are sucking up most of my time.


(In reply to Nick Fitzgerald [:fitzgen] from comment #15)
> (In reply to Deary Hudson jr. from comment #14)
> > fitz: I've set some logs on the prettified source after the source has been
> > prettified, and when breakpoints are set (regardless of line number)the
> > numbers do not reflect where they are being set on the prettified code in
> > findBreakpoints(...)
> 
> Hey Deary, sorry I missed this comment :(
> 
> In the future, setting "need more information from" to me should help make
> sure I reply. Sorry, again.
> 
> Are you still interested in working on this bug? I'm not sure I follow
> exactly what you mean in comment 14.

Comment 18

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Deary would like to work on this again.

Comment 19

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Hi Deary, how can I help you out?

Comment 20

[Zimon Dai [:zimonD]]

Hey Nick, me again...

I did some quick glance on this bug, and found that it depends on bug1129784.

Prettifying source -> move breakpoints -> current BPs do not respect indent (having no generatedColumn value) -> error BP moving -> error prettified souce BP behaviours

Comment 21

[Zimon Dai [:zimonD]]

While bug1129784 is about column breakpoints, I filed a new bug for normal line breakpoints. bug1138471

Comment 22

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Hey Zimon! Glad to see you back around these parts.

I think :ejpbruel is a better person to talk about this bug with, since he is currently refactoring the way the debugger does breakpoints.

The fact that this is currently blocked, means it might not be a good bug to take right now, however. If that is the case, then perhaps you could pick up bug 1135435 in the meantime?

Comment 23

[Zimon Dai [:zimonD]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #22)
> Hey Zimon! Glad to see you back around these parts.
> 
> I think :ejpbruel is a better person to talk about this bug with, since he
> is currently refactoring the way the debugger does breakpoints.
> 
> The fact that this is currently blocked, means it might not be a good bug to
> take right now, however. If that is the case, then perhaps you could pick up
> bug 1135435 in the meantime?

Ah, I'm waiting for bug983469 patch being landed...

Comment 24

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Zimon Dai [:zimonD] from comment #23)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #22)
> > Hey Zimon! Glad to see you back around these parts.
> > 
> > I think :ejpbruel is a better person to talk about this bug with, since he
> > is currently refactoring the way the debugger does breakpoints.
> > 
> > The fact that this is currently blocked, means it might not be a good bug to
> > take right now, however. If that is the case, then perhaps you could pick up
> > bug 1135435 in the meantime?
> 
> Ah, I'm waiting for bug983469 patch being landed...

It should be soon, I wouldn't worry too much about it. You can work on top of that in your patch queue.

Comment 25

[Eddy Bruel [:ejpbruel]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #22)
> Hey Zimon! Glad to see you back around these parts.
> 
> I think :ejpbruel is a better person to talk about this bug with, since he
> is currently refactoring the way the debugger does breakpoints.
> 
> The fact that this is currently blocked, means it might not be a good bug to
> take right now, however. If that is the case, then perhaps you could pick up
> bug 1135435 in the meantime?

The way breakpoint sliding should work is this. When a breakpoint is set on a location that does not have any code, line breakpoints should slide to the next closest line, while column breakpoint should slide to the next closest column (possibly crossing lines).

Currently, both line and column breakpoints slide to the next closest line. In addition, line breakpoints in source mapped sources are treated as column breakpoints (because computing the generated location results in a generatedColumn), but still slide to the next closest line.

In other words, there are multiple things broken here, but I expect to fix most of them within the next few weeks. Fixing breakpoint sliding for non-source-mapped sources is in bug 1138975, and I have patches coming up for fixing breakpoint sliding for source-mapped sources. After that, I plan to spend a few weeks writing tests and cleaning things up. Hopefully I'll be able to get to 1129784 during that time as well.

Comment 26

[David Walsh :davidwalsh]

The new debugger shows breakpoints relative to their original position when pretty-printed.