1203189 - Breakpoint is skipped after setting a breakpoint, navigating to a different page and then going back

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Brian Grinstead [:bgrins]]

Attachment: nav1.html

STR:

Open the attached html file
Set a breakpoint on line 7
Notice that clicking on the "Click me" text hits the bp (continue through that)
Click the link
Go back

Expected
Clicking on "Click me" pauses at the bp

Actual
Doesn't pause

Comment 1

[Brian Grinstead [:bgrins]]

Is this a known bug?  Feel free to dupe if so.

Comment 2

[James Long (:jlongster)]

I can reproduce. If I refresh the page it works again. Is this something with bfcache? Might be a platform bug, but I'm not sure. It definitely seem like it only happens when loaded from bfcache.

This is a unique bug as far as I know, thanks!

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to James Long (:jlongster) from comment #2)
> I can reproduce. If I refresh the page it works again. Is this something
> with bfcache? Might be a platform bug, but I'm not sure. It definitely seem
> like it only happens when loaded from bfcache.

Definitely seems somehow related to bfcache.  Same issue happens when I navigate forward to the attached page.  No idea if it's a platform issue or something in the debugger server.  The breakpoint is visible in the UI even after navigating back/forwards.

Comment 4

[James Long (:jlongster)]

I'll look into it more today or tomorrow and see the the debugger server looks like in that state.

Comment 5

[James Long (:jlongster)]

As I expected, we are not getting onNewScript notifications from the debugger when the page is loaded from bfcache. Because we aren't notified of the script, we don't set breakpoints on it. Should bfcache just be disabled when devtools is open?

Nick probably knows about the platform side also, so I ni? him in case he knows anything. I'll ping Eddy tomorrow too.

Comment 6

[James Long (:jlongster)]

This also doesn't seem to be a recent regression, I was able to reproduce in Firefox 40.

Comment 7

[James Long (:jlongster)]

I think the issue is _discoverSources: https://github.com/mozilla/gecko-dev/blob/master/toolkit/devtools/server/actors/script.js#L1319-L1333

Basically, when a tab navigates, we clear the debuggees: https://github.com/mozilla/gecko-dev/blob/master/toolkit/devtools/server/actors/webbrowser.js#L1643-L1653 This is going to forget about all breakpoints for those JSScript instances.

This is fine because when we navigate *to* a page, we get `onNewScript` notifications, and in those handler we re-set breakpoints that have been set in previous sessions. We need to re-set the breakpoints because they are new JSScript instances.

But in the case of a page coming from bfcache, the debuggee has been cleared (so JSScript breakpoints are gone) but we *don't* get onNewScript notifications because they are the same JSScript instances.

`_discoverSources` may just need to call `_addSource` or `_restoreBreakpoints` to make sure everything is in order. This flow is complicated though and we'll need to make this change carefully. I'd like to refactor all these functions in a way that makes all this more consistent... (if possible).

Comment 8

[James Long (:jlongster)]

Clearing needinfo because I think our thread actor is just buggy with bfcache.

Comment 9

[Eddy Bruel [:ejpbruel]]

The bfcache is problematic for a number of reasons. This is one of them. Another one is that we currently cannot detect the difference between workers that are frozen because they moved into the bfcache and workers that are frozen because the main thread paused (see bug 1178721).

Since the bfcache is essentially a performance optimization, I'd be in favor of disabling it when the devtools are open (assuming this does not adversely affect user experience). It would make our life significantly easier.

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

I'm pretty sure that breakpoints used to work correctly with the bfcache in the past. The pageshow/pagehide handlers in webbrowser.js were added for this purpose. I remember testing breakpoints in inline scripts that would trigger when going back/forward even before the page was loaded. I thought we had a test for this.

I don't know when this bug crept in, but it can't have been there forever.

Comment 11

[James Long (:jlongster)]

I opened a bug about disabling bfcache: bug 1204999

I don't see anywhere that the debugger server handles pagehide/pageshow. The tab actor handles it to emit certain events, but that's all.

Comment 12

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

My understanding of how things were supposed to work:

When we navigate to a page in the bfcache, the ScriptStore was destroyed on navigation when we removed debuggees so the first time we access the `scripts` getter, we find all D.Scripts on the page[0]. Then I thought we called `_restoreBreakpoints` on each navigation (which would call the getter and trigger finding all those scripts), but it seems that we do not.

I think this may be the bug right here.

[0] https://dxr.mozilla.org/mozilla-central/rev/9ed17db42e3e46f1c712e4dffd62d54e915e0fac/toolkit/devtools/server/actors/script.js#503

Comment 13

[James Long (:jlongster)]

@fitzgen: yeah, _addSource is the low-level function that re-sets the breakpoints and I don't see where that is called. it's only called from `_restoreBreakpoints` (which I still don't know when that's called, supposed to be when we attach be I don't know when we attach and also have stuff in the breakpoint store) and the `onNewScript` handler.

We should probably just fix this bug, and let Eddy decide about the fate of bfcache. Not sure how hard the problem about workers he mentioned is.

Comment 14

[Jason Laster [:jlast]]

this now works