1280257 - Breakpoints do not hit after page reload if "Show original sources" is disabled

Status: VERIFIED FIXED

(DevTools :: Debugger, defect, P1)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

STR:

1. Disable "Show original sources" in debugger options
2. Go to any page, for example http://todomvc.com/examples/react/
3. Set a breakpoint on some source, for example base.js line 8
4. Reload to hit the breakpoint

ER:

Breakpoint should hit.

AR:

Breakpoint only hits if you enable "Show original sources".  Source maps are not used on this page.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

47 is not affected, but 48 - 50 are.  Will bisect further in a moment...

[Tracking Requested - why for this release]: Debugger is effectively broken when a specific option is changed in the UI.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I'm not surprised here... bisecting points to bug 1269457:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=22047a4eea784c15026c77911c0bd6ea1b70fa68&tochange=7c0dce574b3f2d5e8f6e4e60caab5532f8e13fd6

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

The mystery at the moment is the Browser Toolbox correctly hits bps with "Show original sources" disabled, but the regular page toolbox fails to do so.

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

:ejpbruel, any ideas here?  Web content fails to hit bps after I added the `actor.isPending` check to the non-sourcemapped path in bug 1269457.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

It seems the issue is specific to what happens after reload.  So it makes sense that we don't see a problem in Browser Toolbox since you're not likely to be reloading chrome windows.

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Reset breakpoint pending state on reload

:ejpbruel, does this make sense in general?  It seems to fix the issue here, but I am not sure I really understand all the moving pieces.

If it looks good, I'll try to craft a test as well.

Comment 7

[:Gijs (he/him)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> It seems the issue is specific to what happens after reload.  So it makes
> sense that we don't see a problem in Browser Toolbox since you're not likely
> to be reloading chrome windows.

I vaguely recall seeing issues with breakpoints in secondary (browser/non-browser) windows when using the browser toolbox though, which might potentially be related to the same thing?

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to :Gijs Kruitbosch from comment #7)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> > It seems the issue is specific to what happens after reload.  So it makes
> > sense that we don't see a problem in Browser Toolbox since you're not likely
> > to be reloading chrome windows.
> 
> I vaguely recall seeing issues with breakpoints in secondary
> (browser/non-browser) windows when using the browser toolbox though, which
> might potentially be related to the same thing?

Hmm, possibly...?  I must confess I don't know the debugger internals very well.  If you have precise STR for this case, I can certainly check it out.

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8764708 [details] [diff] [review]
Reset breakpoint pending state on reload

I'll flag :jlong too and see who can get back faster...  Hopefully we can find the path forward soon.

Comment 10

[James Long (:jlongster)]

I don't think this is the right fix, I think it just happened to fix a bug that is still happening somewhere else. A breakpoint is only "pending" if we aren't sure if it needs to "slide" or not. If we have already resolved a breakpoint (meaning, we were able to find code to actually set it on), it will never be in a pending state. It will be pending if there is no code to set it on, and we can do sliding/resolving later.

Basically: once it isn't pending, it should never be pending again, which is why I think this patch isn't right.

I can look more into this later today. As far as I know, we might be able to remove the pending state completely because we never slide breakpoints anymore on reloads. I only see two places in the client and server where pending is used and it doesn't seem like it's needed anymore.

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to James Long (:jlongster) from comment #10)
> I don't think this is the right fix, I think it just happened to fix a bug
> that is still happening somewhere else. A breakpoint is only "pending" if we
> aren't sure if it needs to "slide" or not. If we have already resolved a
> breakpoint (meaning, we were able to find code to actually set it on), it
> will never be in a pending state. It will be pending if there is no code to
> set it on, and we can do sliding/resolving later.
> 
> Basically: once it isn't pending, it should never be pending again, which is
> why I think this patch isn't right.
> 
> I can look more into this later today. As far as I know, we might be able to
> remove the pending state completely because we never slide breakpoints
> anymore on reloads. I only see two places in the client and server where
> pending is used and it doesn't seem like it's needed anymore.

Okay, by all means take a look!  I am little worried when you say "remove the pending state", since bug 1269457 solved a large performance regression in the Browser Toolbox by testing the pending state.  But perhaps you have additional changes in mind to go along with removing the pending state that would avoid this problem in a different way.

Comment 12

[James Long (:jlongster)]

I thought that only depended on the sourcemap flags, but either way I'll make sure the the code changes so not change any behavior (and I'll keep that bug in mind). Will take a long once my build finished.

Comment 13

[James Long (:jlongster)]

Wow, talk about horrible typing in my last comment!

Comment 14

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8764708 [details] [diff] [review]
Reset breakpoint pending state on reload

Review of attachment 8764708 [details] [diff] [review]:
-----------------------------------------------------------------

I agree with James' assessment in comment 10. Once a breakpoint has been resolved, it should never become pending again. Apparently, the problem is that breakpoints aren't reset correctly after a reload. For some reason, making them breakpoints pending again will cause them to be set correctly.

Comment 15

[Eddy Bruel [:ejpbruel]]

I think I know what's going on here.

In the source mapped case, we check whether the breakpoint is pending in script.js:1983. If the breakpoint is pending, we call _setBreakpoint. This method tries to resolve the breakpoint, performing breakpoint sliding if necessary (recall that we also used to do breakpoint sliding for source mapped sources).

If the breakpoint was resolved successfully, we now know the actual original location of the breakpoint. _setBreakpoint would then obtain the generated locations for that original location, and call _setBreakpointAtAllGeneratedLocations to set breakpoint handlers on the appropriate bytecode offsets using the debugger API.

On the other hand, if the breakpoint is not pending, it must already been resolved. In that case, we can skip the call to _setBreakpoint, because the actual original location of the breakpoint is already known. Instead, we can simply call getGeneratedLocations, and call _setBreakpointAtAllGeneratedLocations with the result.

Now for the non-source mapped case. Here, we also check whether the breakpoint is pending (script.js:2016). If the breakpoint is pending, we again try to resolve the breakpoint by calling _setBreakpoint. If the breakpoint is not pending (i.e. already resolved), we can skip the call to _setBreakpoint. Instead, we should call _setBreakpointAtGeneratedLocation, using the breakpoint's original location (since we're not using source maps in this case, original location == generated location).

The problem is that we forget that last step if the breakpoint is not pending. This explains why jryans' fix made things work.

Comment 16

[Eddy Bruel [:ejpbruel]]

Attachment: Reset non-pending breakpoints when tab is reloaded with source maps disabled.

Comment 17

[James Long (:jlongster)]

Comment on attachment 8765408 [details] [diff] [review]
Reset non-pending breakpoints when tab is reloaded with source maps disabled.

Review of attachment 8765408 [details] [diff] [review]:
-----------------------------------------------------------------

I can't wait to move sourcemaps to the frontend and simplify a lot of this code. I don't love that the config flag changes the code flow like this (we should be running all tests with the config flag on and off now). But this is a good quick fix.

Comment 18

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8765408 [details] [diff] [review]
Reset non-pending breakpoints when tab is reloaded with source maps disabled.

Review of attachment 8765408 [details] [diff] [review]:
-----------------------------------------------------------------

Great, this version seems to work on reload and also appears to avoid the performance problem in the Browser Toolbox from bug 1269457.

Comment 19

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8765408 [details] [diff] [review]
Reset non-pending breakpoints when tab is reloaded with source maps disabled.

Copying the r+ that I somehow cleared...

Comment 20

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Is this ready to land?

Comment 21

[Eddy Bruel [:ejpbruel]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #20)
> Is this ready to land?

I still need to get a try run for this. You're welcome to do that and land the patch yourself if you're in a rush ;-)

If not, I can put it on my todo list for tomorrow.

Comment 22

[Eddy Bruel [:ejpbruel]]

Try push for this patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=62a5cca58358

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/1aca6ed481ad
Reset non-pending breakpoints when tab is reloaded with source maps disabled;r=jryans

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1aca6ed481ad

Comment 25

[Gerry Chang [:gchang]]

Track this as this is a regression for 48~50.
Hi Eddy, do you want to uplift this for aurora & beta if this patch is not too risky.

Comment 26

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8765408 [details] [diff] [review]
Reset non-pending breakpoints when tab is reloaded with source maps disabled.

Approval Request Comment
[Feature/regressing bug #]: bug 1269457
[User impact if declined]: breakpoints will not work after refresh if source maps are disabled.
[Describe test coverage new/current, TreeHerder]: N/A
[Risks and why]: No significant risk; patch is small, and the added code is only executed if source maps are disabled.
[String/UUID change made/needed]: N/A

Comment 27

[Gerry Chang [:gchang]]

Comment on attachment 8765408 [details] [diff] [review]
Reset non-pending breakpoints when tab is reloaded with source maps disabled.

Review of attachment 8765408 [details] [diff] [review]:
-----------------------------------------------------------------

Fix a regression. Take it in 48 beta 6 and aurora.

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6c6b30a921d9

Comment 29

[Carsten Book [:Tomcat]]

has conflicts applying to beta:

grafting 352706:6c6b30a921d9 "Bug 1280257 - Reset non-pending breakpoints when tab is reloaded with source maps disabled;r=jryans, a=gchang"
merging devtools/server/actors/script.js
warning: conflicts while merging devtools/server/actors/script.js! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 30

[Eddy Bruel [:ejpbruel]]

Attachment: Rebased patch for mozilla-beta.

The reject hunk contained some code style fixes that weren't necessary on beta; this rebased patch should apply cleanly.

Comment 31

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

If we can land this by tonight then it can make it into beta 6.

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/315bb3aca912

Comment 33

[Tanvir Rahman]

I have reproduced this bug with nightly 50.0a1 (2016/06/15) on Windows 10, 64 bit!

The Bug's fix is now verified on Latest Nightly 50.0a1, Aurora 49.0a2 and Beta 48.0b9

Nightly 50.0a1
Build ID 	20160720030208
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0

Aurora 	49.0a2
Build ID 	20160720004018
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0

Beta 	48.0b9
Build ID 	20160718142219
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0


[bugday-20160720]