783393 - Breakpoints not getting caught on reload

Status: VERIFIED FIXED

(DevTools :: Debugger, defect, P1)

Description

[Rob Campbell [:rc] (:robcee)]

STR:

1. Open http://htmlpad.org/debug-startup-test/
2. click through the alerts
3. Open the Debugger
4. Set a breakpoint on line 5.
5. Press Reload

Expected Results: Page should reload and the breakpoint should hit.

Actual: Breakpoint is not hit and the alerts are presented.

Affects Nightly, Aurora (16) and Beta (15).

Comment 1

[Dave Camp (:dcamp)]

During the navigation away, the server detaches the debugger.
In the DOMWindowCreated handler, we send a tabNavigated event.
But parsing continues, and the scripts are loaded.
The client gets the tabNavigated event, and reattaches
But by this point, the script is parsed and we're too late.

We probably need to rejigger the tab actor to remain attached across a reload: rather than a real detach() in the unload process, we can clear out the debugger object or whatever, but don't send a detach event.  In the DOMWindowCreated handler, we just add the new window as a debuggee (we still probably need to send a tabNavigated event so that the client knows to refresh...)

Comment 2

[Dave Camp (:dcamp)]

Attachment: proof of concept

This reattaches an attached debugger during reload.

A few things:
* The thread actor is now kept alive across reloads, as long as the debugger is attached.  Protocol-wise this was already the case, but it was broken and the UI didn't take advantage of it (it instantly detached/reattached).
* In order to keep the thread actor alive across reloads, I added a clearDebuggees method that should bring the actor back to a baseline state.
* I just put a return statement at the front of _onTabAttached: we don't need all that detach/re-attach stuff if the thread actor follows the reload.  But it should do some clearing of internal state anyway.
* I fixed up reloading-while-paused, but closing the window while paused seems to be broken at the minute.

Comment 3

[Victor Porof [:vporof][:vp]]

Attachment: patch

I made some changes that should have fixed everything.

This is fubar.
https://dl.dropbox.com/u/2388316/fubar.webm

Take a look at the video and the logging in the shell.
>>>>>>>>>>>>>>>>>>>>>>>>>>> SCRIPTS ADDED

>> url      :http://ajax.googleapis.com/ajax/libs/jquery/1.3.2/jquery.min.js
>> url      :http://htmlpad.org/debugger/
>> url      :http://htmlpad.org/debug-startup-test/

No. That should not happen.
Also, if you're persistent, you get to a point where after a tab navigation no more newscript or scriptsadded events are fired. Ever.

Something has wildly changed, and it's not in the debugger frontend.

Comment 4

[Victor Porof [:vporof][:vp]]

Tests to add:
https://etherpad.mozilla.org/debugger-reload

Comment 5

[Victor Porof [:vporof][:vp]]

(In reply to Victor Porof [:vp] from comment #3)
We're chasing our own tails.
The scripts-from-other-windows thing happens with this patch applied only.

Dave, I think
+    for (let s of this.dbg.findScripts()) {
+      this._addScript(s);
+    }
is misbehaving. Or something.

Comment 6

[Dave Camp (:dcamp)]

Attachment: another rev

From victor's patch:

* Moved a few things out of handleTabNavigation: the idea is that the thread actor will retain its state across navigations, so we don't need to:
** Refresh the stack (we know the application is running, because we resume on navigate.
** Reset pauseOnExceptions (that should have been retained in the state of the actor - although I'll admit I haven't tested it).

* Made sure a potential double-tabNavigated doesn't happen.

* Stopped using the scriptCache in the dbg-client.  It's broken, and should be removed (either here or as a followup).

I haven't patched the fennec or b2g actors, maybe Panos or Victor could port my changes to it?

Comment 7

[Dave Camp (:dcamp)]

I don't like that we use the tabNavigated notification from the tab actor to infer information about the state of the thread actor (that the scripts need to be cleared, for example).  I'd suggest we clear that up in a followup.

Comment 8

[Dave Camp (:dcamp)]

(In reply to Dave Camp (:dcamp) from comment #6)
> * Moved a few things out of handleTabNavigation: the idea is that the thread
> actor will retain its state across navigations, so we don't need to:
** Don't refetch the list of scripts from the thread actor - because the actor is already attached, we'll get a newScript for each new script as part of the navigation.

Comment 9

[Victor Porof [:vporof][:vp]]

cache is hard.

Comment 10

[Dave Camp (:dcamp)]

Attachment: again

Try this one:

* Moved getScripts back into tab navigation, because when bfcached we might not get newScript notifications.
* Add the frame tree in onWindowCreated, again for bfcache
* Wrap getScripts in an interrupt.

Comment 11

[Dave Camp (:dcamp)]

(In reply to Rob Campbell [:rc] (:robcee) from comment #0)
> STR:
> 
> 1. Open http://htmlpad.org/debug-startup-test/
> 2. click through the alerts
> 3. Open the Debugger
> 4. Set a breakpoint on line 5.
> 5. Press Reload
> 
> Expected Results: Page should reload and the breakpoint should hit.

btw, I changed this to add a HELLO to the page rather than run alerts.
> Actual: Breakpoint is not hit and the alerts are presented.
> 
> Affects Nightly, Aurora (16) and Beta (15).

Comment 12

[Victor Porof [:vporof][:vp]]

Comment on attachment 652951 [details] [diff] [review]
again

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

::: browser/devtools/debugger/debugger-controller.js
@@ +873,5 @@
>     *        The next function in the initialization sequence.
>     */
>    connect: function SS_connect(aCallback) {
>      window.addEventListener("Debugger:LoadSource", this._onLoadSource, false);
> +    this.debuggerClient.addListener("newScript", this._onNewScript);

I understand why we removed scriptscleared.
But no more scriptsadded?

Comment 13

[Dave Camp (:dcamp)]

Old code used fillScripts, which would getScripts, put the response in a script cache, and fire 'scriptsadded' to let the user know that the cache had changed.

New code avoids fillScripts, calls getScripts itself and uses the response directly.  No need for the scriptsadded notification.

Comment 14

[Victor Porof [:vporof][:vp]]

(In reply to Dave Camp (:dcamp) from comment #13)
> Old code used fillScripts, which would getScripts, put the response in a
> script cache, and fire 'scriptsadded' to let the user know that the cache
> had changed.
> 
> New code avoids fillScripts, calls getScripts itself and uses the response
> directly.  No need for the scriptsadded notification.

In this case there's *a lot* of cleaning up to do in the frontend. Followup for this?

Comment 15

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

(In reply to Dave Camp (:dcamp) from comment #6)
> I haven't patched the fennec or b2g actors, maybe Panos or Victor could port
> my changes to it?

From reading the patch I think it should work as is, but I'll test to be sure.

Comment 16

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

(In reply to Dave Camp (:dcamp) from comment #13)
> Old code used fillScripts, which would getScripts, put the response in a
> script cache, and fire 'scriptsadded' to let the user know that the cache
> had changed.
> 
> New code avoids fillScripts, calls getScripts itself and uses the response
> directly.  No need for the scriptsadded notification.

The idea with reusing the stack frames approach was that we wouldn't want the debugger frontend stall when loading pages with a huge number of scripts. It always bothered me though that I didn't have a real world example to measure against.

Comment 17

[Rob Campbell [:rc] (:robcee)]

adding tracking flags for aurora 16 and beta 15. This is affected on both.

Comment 18

[Victor Porof [:vporof][:vp]]

This doesn't look right. The debugger appears to not be resuming anymore in some cases (not bfcache related afaict).

https://dl.dropbox.com/u/2388316/fubar-2.webm

Comment 19

[Rob Campbell [:rc] (:robcee)]

(In reply to Victor Porof [:vp] from comment #18)
> This doesn't look right. The debugger appears to not be resuming anymore in
> some cases (not bfcache related afaict).
> 
> https://dl.dropbox.com/u/2388316/fubar-2.webm

any idea how you got into that state? Or what caused it?

Comment 20

[Victor Porof [:vporof][:vp]]

(In reply to Rob Campbell [:rc] (:robcee) from comment #19)
> (In reply to Victor Porof [:vp] from comment #18)
> > This doesn't look right. The debugger appears to not be resuming anymore in
> > some cases (not bfcache related afaict).
> > 
> > https://dl.dropbox.com/u/2388316/fubar-2.webm
> 
> any idea how you got into that state? Or what caused it?

I opened Nightly with that patch applied and did exactly what's shown in the video.

Comment 21

[Rob Campbell [:rc] (:robcee)]

Attachment: disable-nav-on-beta

This is about the dirtiest implementation of this I can imagine. It's also the simplest. If anybody has any improvements on this, would love to hear them. Also disabled the failing tests.

To be clear, this is for Beta. still hoping to get a cleaner fix for Aurora and Nightly.

Comment 22

[Dave Camp (:dcamp)]

Comment on attachment 653533 [details] [diff] [review]
disable-nav-on-beta

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

All the tests but the disabled one pass with this change?

::: browser/devtools/debugger/debugger-controller.js
@@ +191,5 @@
>      client.activeThread.detach(function() {
>        client.activeTab.detach(function() {
>          client.listTabs(function(aResponse) {
> +          // XXX See bug 783393 - we need to kill the debugger on navigation
> +          // for this release.

Could you do that outside the listTabs, right after detaching everything?

Comment 23

[Rob Campbell [:rc] (:robcee)]

(In reply to Dave Camp (:dcamp) from comment #22)
> Comment on attachment 653533 [details] [diff] [review]
> disable-nav-on-beta
> 
> Review of attachment 653533 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> All the tests but the disabled one pass with this change?

two disabled. Yes.

> 
> ::: browser/devtools/debugger/debugger-controller.js
> @@ +191,5 @@
> >      client.activeThread.detach(function() {
> >        client.activeTab.detach(function() {
> >          client.listTabs(function(aResponse) {
> > +          // XXX See bug 783393 - we need to kill the debugger on navigation
> > +          // for this release.
> 
> Could you do that outside the listTabs, right after detaching everything?

Ok, sure. Will update tomorrow morning. Thanks!

Comment 24

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

(In reply to Rob Campbell [:rc] (:robcee) from comment #21)
> This is about the dirtiest implementation of this I can imagine. It's also
> the simplest. If anybody has any improvements on this, would love to hear
> them. Also disabled the failing tests.

I'm not particularly concerned with elegance in urgent, temporary, last-minute fixes, but you could probably also use the tabDetached event handler for the tabNavigated event as well. Haven't tested it though.

Comment 25

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

BUGZILLAAAAAAAA!!!
Sorry Lukas.

Comment 26

[Rob Campbell [:rc] (:robcee)]

Attachment: disable-nav-on-beta-2

update. Just took out the listTabs section because I don't think it's needed for this.

Was there some extra detachery this needed as well? toggleDebugger calls all of the various detach and disconnects via the UI so I'm not sure what else is required.

Also, not sure where Panos was pointing me. Will ask in the meeting.

Comment 27

[Lukas Blakk [:lsblakk] use ?needinfo]

Will track for 15/16. If this fix only affects the debugger we can take the uplift as long as there's no risk to desktop users who aren't using the debugger.

Comment 28

[Lukas Blakk [:lsblakk] use ?needinfo]

In your risk assessment for approval please let us know what the worst case scenario is here - would we have to chemspill if this broke things worse than they already are?

Comment 29

[Rob Campbell [:rc] (:robcee)]

(In reply to Lukas Blakk [:lsblakk] from comment #28)
> In your risk assessment for approval please let us know what the worst case
> scenario is here - would we have to chemspill if this broke things worse
> than they already are?

Hey Lukas,

So, this is only a risk to users of the Debugger. Regular desktop users are completely unaffected.

At worst, shipping a fix for this will affect an already broken feature and only in the scenario that we're trying to fix. We should still have a killswitch or backout option if we absolutely need to turn it off (though we have shipped a broken feature once in the past, and would rather not do it again if at all possible).

Comment 30

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

Attachment: Patch v5

Fixed the crux of the problem in comment 18, but now I'm getting context and object actors to disappear after reloads. Rob's STR result in a missing object actor, whereas if you don't add a breakpoint, but just reload twice, you get a missing context actor.

Main changes in this version:
* made the client packet dispatcher not treat unsolicited pause packets as responses to in-flight requests
* made getScripts not interrupt first (this was to fix Victor's issue, that turned out to be unrelated, but I left it in since interrupting is redundant anyway)

I think that not detaching from the tab on navigation will break the fix for bug 751949, but reverting that patch seemed to generate other issues, so I shelved it for now.

Comment 31

[Rob Campbell [:rc] (:robcee)]

Attachment: disable-nav-on-beta-3

3rd crack, based on feedback from panos.

Comment 32

[Dave Camp (:dcamp)]

Comment on attachment 653881 [details] [diff] [review]
disable-nav-on-beta-3

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

Clever.  Thanks Rob.

Comment 33

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 653881 [details] [diff] [review]
disable-nav-on-beta-3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new feature. 
User impact if declined: users will be confused by breakpoints that don't work when navigating to new pages or refreshing.
Testing completed (on m-c, etc.): local.
Risk to taking this patch (and alternatives if risky): negligible. This disables the debugger on navigation/reload. Alternatives are worse as it causes user confusion and mistrust.
String or UUID changes made by this patch: none.

Comment 34

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 653881 [details] [diff] [review]
disable-nav-on-beta-3

As discussed in a meeting with rob and dcamp - we're taking this for FF15 as a temporary measure to ensure users of the new-to-15 debugger get a known (unfortunately annoying) experience instead of a silent, not working right experience with the breakpoints.  We'll land this to beta and then do a lot of messaging to devs that the proper fix will be in 16, and that will be on the Beta channel next week (9/2).

Comment 35

[Lukas Blakk [:lsblakk] use ?needinfo]

Sooner than 9/2!  8/31, even.

Comment 36

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 653881 [details] [diff] [review]
disable-nav-on-beta-3

landed in beta:

https://hg.mozilla.org/releases/mozilla-beta/rev/205115c395d4

Comment 37

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

Attachment: Patch v6: "Always bet on dcamp"

So this version fixes the problem in comment 30, by not randomly removing the last pool when no argument is supplied in DSC_removeActorPool. As Dave said, "it's a nasty one". 

Still need to see if any other bugs are there, like in bfcache interaction, before fixing the tests.

Comment 38

[Victor Porof [:vporof][:vp]]

Attachment: Patch v7: all tests now work

Comment 39

[Victor Porof [:vporof][:vp]]

This should also fix bug 773732.

Comment 40

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

Attachment: Patch v8

Mostly cleanups and a fix for Fennec. All tests pass locally. I was about to consider this done, when I stumbled across another problematic case:

1) Visit http://htmlpad.org/debugger/
2) Hit 'Click me'
3) Resume once
4) After the debuggee is paused again, reload the page.

The issue here is that the client hasn't been informed that the server has silently resumed in the process of navigation, and is still displaying stack frames, etc.

It gets even better though. Same STR as above, but this time omit step 3. Now the server resumes, but before navigation finishes, the second debugger statement is hit. Eventually the navigation completes, but the client has once again lost track of the fact that the server has resumed.

Comment 41

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

Attachment: Patch v9

This version fixes the issues in comment 40. Still trying to find cases that may be mishandled.

Changes:
* make the client dispatcher generate a fake resumption packet when a tabNavigated packet arrives, in order to trigger the client state change listeners
* disable debugging after resuming the thread on navigation and enable it again right after the old debuggees are removed

Comment 42

[Dave Camp (:dcamp)]

Comment on attachment 654203 [details] [diff] [review]
Patch v9

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

::: toolkit/devtools/debugger/server/dbg-server.js
@@ +367,5 @@
>     */
>    removeActorPool: function DSC_removeActorPool(aActorPool) {
> +    if (aActorPool) {
> +      let index = this._extraPools.splice(this._extraPools.lastIndexOf(aActorPool), 1);
> +    }

You might want to also protect/warn against actor pools that don't exist in extraPools.

Comment 43

[Victor Porof [:vporof][:vp]]

Did a try run last night with version 7 of the patch.
https://tbpl.mozilla.org/?tree=Try&rev=8f29ae7a46b6

Looks good except bfcache and location-changes-blank fails on windows.
Rob said he's going to crank up a win build.

Comment 44

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

(In reply to Victor Porof [:vp] from comment #43)
> Did a try run last night with version 7 of the patch.
> https://tbpl.mozilla.org/?tree=Try&rev=8f29ae7a46b6
> 
> Looks good except bfcache and location-changes-blank fails on windows.
> Rob said he's going to crank up a win build.

The location-changes-blank failures are unrelated and should be fixed by the patch from bug 770882 that I pushed a few hours ago.

Comment 45

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

Attachment: Patch v10

Took Dave's advice from comment 42. I couldn't find any other failing cases, including interactions with bfcache and breakpoints in inline scripts.

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=0355a75dea6f

I'll try building fennec and b2g next, to make sure the changes do not break them.

Comment 46

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 654235 [details] [diff] [review]
Patch v10

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

I don't have much to add to this. The changes are relatively straightforward. I've tested this out and it seems to work well for the cases I've tried. Even creating iframes in pages and doing back-forward navigation seems to work well.

::: browser/devtools/debugger/debugger-controller.js
@@ +319,5 @@
>      this.activeThread.addListener("paused", this._update);
>      this.activeThread.addListener("resumed", this._update);
>      this.activeThread.addListener("detached", this._update);
>  
> +    this.handleTabNavigation();

I'm half-inclined to leave _update() and call that here instead of handleTabNavigation. Though I also worry about update-spaghetti which is a terrible, usually fatal condition.

handleTabNavigation doesn't appear to be called outside any of these objects. Should probably be _handleTabNavigation if it's essentially private.

@@ +403,5 @@
>      this.activeThread.addListener("resumed", this._onResume);
>      this.activeThread.addListener("framesadded", this._onFrames);
>      this.activeThread.addListener("framescleared", this._onFramesCleared);
>  
> +    this.handleTabNavigation();

this change seems anticipatory. Are you just doing this for consistency with ThreadState?

@@ +921,5 @@
>      }
>  
>      this._addScript({ url: aPacket.url, startLine: aPacket.startLine }, true);
>  
> +    let preferredScriptUrl = DebuggerView.Scripts.preferredScriptUrl;

is an unsolicited newScript ever going to be the preferredScript? I guess it could be if you have a navigation occur and try to return to your previous state.

Just puzzling this out.

@@ +965,3 @@
>        DebuggerView.Scripts.selectIndex(0);
>      }
> +

some shared code between these two methods, but not sure it's worth grouping them into one method or not. I'm ok with not.

::: toolkit/devtools/debugger/dbg-client.jsm
@@ +431,5 @@
> +      if (aPacket.type == UnsolicitedNotifications.tabNavigated &&
> +          aPacket.from in this._tabClients) {
> +        let resumption = { from: this.activeThread._actor, type: "resumed" };
> +        this.activeThread._onThreadState(resumption);
> +      }

ok!

::: toolkit/devtools/debugger/server/dbg-script-actors.js
@@ +61,5 @@
> +    this.conn.removeActorPool(this._threadLifetimePool || undefined);
> +    this._threadLifetimePool = null;
> +    // Unless we carefully take apart the scripts table this way, we end up
> +    // leaking documents. It would be nice to track this down carefully, once
> +    // we have the appropriate tools.

that sounds scary. Do we need a bug filed for this?

@@ +83,5 @@
> +      this._dbg.uncaughtExceptionHook = this.uncaughtExceptionHook.bind(this);
> +      this._dbg.onDebuggerStatement = this.onDebuggerStatement.bind(this);
> +      this._dbg.onNewScript = this.onNewScript.bind(this);
> +      // Keep the debugger disabled until a client attaches.
> +      this.dbg.enabled = this._state != "detached";

a little funny moving from _dbg to dbg here. Might prefer to continue referencing the object directly inside the if block.

@@ +119,3 @@
>      }
> +    this._dbg.enabled = false;
> +    this._dbg = null;

do we need to remove the event handlers from dbg created in addDebuggee or is nulling the debugger instance sufficient?

::: toolkit/devtools/debugger/server/dbg-server.js
@@ +368,5 @@
>    removeActorPool: function DSC_removeActorPool(aActorPool) {
> +    let index = this._extraPools.lastIndexOf(aActorPool);
> +    if (index > -1) {
> +      this._extraPools.splice(index, 1);
> +    }

:)

Comment 47

[Dave Camp (:dcamp)]

Comment on attachment 654235 [details] [diff] [review]
Patch v10

I don't think I need to review this.  At a first glance, the bits since my last patch make sense to me.

Comment 48

[Dave Camp (:dcamp)]

(In reply to Rob Campbell [:rc] (:robcee) from comment #46)
> @@ +83,5 @@
> > +      this._dbg.uncaughtExceptionHook = this.uncaughtExceptionHook.bind(this);
> > +      this._dbg.onDebuggerStatement = this.onDebuggerStatement.bind(this);
> > +      this._dbg.onNewScript = this.onNewScript.bind(this);
> > +      // Keep the debugger disabled until a client attaches.
> > +      this.dbg.enabled = this._state != "detached";
> 
> a little funny moving from _dbg to dbg here. Might prefer to continue
> referencing the object directly inside the if block.

Agreed, and in a followup I think we should just ditch the _dbg/dbg thing and use dbg directly as an instance variable.  

> @@ +119,3 @@
> >      }
> > +    this._dbg.enabled = false;
> > +    this._dbg = null;
> 
> do we need to remove the event handlers from dbg created in addDebuggee or
> is nulling the debugger instance sufficient?

The enabled=false will prevent hooks from being called.

Comment 49

[Victor Porof [:vporof][:vp]]

(In reply to Panos Astithas [:past] from comment #44)
> (In reply to Victor Porof [:vp] from comment #43)
> > Did a try run last night with version 7 of the patch.
> > https://tbpl.mozilla.org/?tree=Try&rev=8f29ae7a46b6
> > 
> > Looks good except bfcache and location-changes-blank fails on windows.
> > Rob said he's going to crank up a win build.
> 
> The location-changes-blank failures are unrelated and should be fixed by the
> patch from bug 770882 that I pushed a few hours ago.

That's awesome. This means that test-wise this is good to go. I'll push to try again and spam those oths to be sure.

(In reply to Rob Campbell [:rc] (:robcee) from comment #46)
> Comment on attachment 654235 [details] [diff] [review]
> Patch v10
> 
> Review of attachment 654235 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't have much to add to this. The changes are relatively
> straightforward. I've tested this out and it seems to work well for the
> cases I've tried. Even creating iframes in pages and doing back-forward
> navigation seems to work well.
> 
> ::: browser/devtools/debugger/debugger-controller.js
> @@ +319,5 @@
> >      this.activeThread.addListener("paused", this._update);
> >      this.activeThread.addListener("resumed", this._update);
> >      this.activeThread.addListener("detached", this._update);
> >  
> > +    this.handleTabNavigation();
> 
> I'm half-inclined to leave _update() and call that here instead of
> handleTabNavigation. Though I also worry about update-spaghetti which is a
> terrible, usually fatal condition.
> 
> handleTabNavigation doesn't appear to be called outside any of these
> objects. Should probably be _handleTabNavigation if it's essentially private.
> 

I agree that we should underscore those functions. However, let's start preventing those spaghetti fatal conditions.

> @@ +403,5 @@
> >      this.activeThread.addListener("resumed", this._onResume);
> >      this.activeThread.addListener("framesadded", this._onFrames);
> >      this.activeThread.addListener("framescleared", this._onFramesCleared);
> >  
> > +    this.handleTabNavigation();
> 
> this change seems anticipatory. Are you just doing this for consistency with
> ThreadState?

Almost. It's part of the initialization procedure, which happens the first time the debugger is started but also needs to be called on tab navigation. Maybe the wording isn't right, but _handleInitializationAndTabNavigaion is a mouthful.

> 
> @@ +921,5 @@
> >      }
> >  
> >      this._addScript({ url: aPacket.url, startLine: aPacket.startLine }, true);
> >  
> > +    let preferredScriptUrl = DebuggerView.Scripts.preferredScriptUrl;
> 
> is an unsolicited newScript ever going to be the preferredScript? I guess it
> could be if you have a navigation occur and try to return to your previous
> state.
> 
> Just puzzling this out.

Yes. Besides bf navigation, after a page refresh or navigation you'll get the scripts in the _onNewScript handler, and not after calling getScripts for _onScriptsAdded.

> 
> @@ +965,3 @@
> >        DebuggerView.Scripts.selectIndex(0);
> >      }
> > +
> 
> some shared code between these two methods, but not sure it's worth grouping
> them into one method or not. I'm ok with not.

The semantics of the two are slightly different (look at the first branch of the conditional). I think grouping them is not necessary at this point.

> 
> @@ +119,3 @@
> >      }
> > +    this._dbg.enabled = false;
> > +    this._dbg = null;
> 
> do we need to remove the event handlers from dbg created in addDebuggee or
> is nulling the debugger instance sufficient?
> 

I've also seen a lot of "JavaScript Error: this._dbg is null", hence I wrapped those in a conditional. I'm not sure if that's related to event handlers not clearing when they should, but sounds like a thing we should investigate a bit, unless Panos knows better.

Comment 50

[Victor Porof [:vporof][:vp]]

(In reply to Dave Camp (:dcamp) from comment #48)
> 
> > @@ +119,3 @@
> > >      }
> > > +    this._dbg.enabled = false;
> > > +    this._dbg = null;
> > 
> > do we need to remove the event handlers from dbg created in addDebuggee or
> > is nulling the debugger instance sufficient?
> 
> The enabled=false will prevent hooks from being called.

Ok.

Comment 51

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

Desktop B2G and Fennec are unaffected by this patch.

Comment 52

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

Attachment: Patch v11

Besides the points made by Dave and Victor I only want to add a couple of notes:

(In reply to Rob Campbell [:rc] (:robcee) from comment #46)
> ::: browser/devtools/debugger/debugger-controller.js
> @@ +319,5 @@
> >      this.activeThread.addListener("paused", this._update);
> >      this.activeThread.addListener("resumed", this._update);
> >      this.activeThread.addListener("detached", this._update);
> >  
> > +    this.handleTabNavigation();
> 
> I'm half-inclined to leave _update() and call that here instead of
> handleTabNavigation. Though I also worry about update-spaghetti which is a
> terrible, usually fatal condition.
> 
> handleTabNavigation doesn't appear to be called outside any of these
> objects. Should probably be _handleTabNavigation if it's essentially private.

Made these private, and I plan to simplify this part in a followup.

> @@ +403,5 @@
> >      this.activeThread.addListener("resumed", this._onResume);
> >      this.activeThread.addListener("framesadded", this._onFrames);
> >      this.activeThread.addListener("framescleared", this._onFramesCleared);
> >  
> > +    this.handleTabNavigation();
> 
> this change seems anticipatory. Are you just doing this for consistency with
> ThreadState?

Yeah, but I plan on removing this in the aforementioned followup.

> ::: toolkit/devtools/debugger/server/dbg-script-actors.js
> @@ +61,5 @@
> > +    this.conn.removeActorPool(this._threadLifetimePool || undefined);
> > +    this._threadLifetimePool = null;
> > +    // Unless we carefully take apart the scripts table this way, we end up
> > +    // leaking documents. It would be nice to track this down carefully, once
> > +    // we have the appropriate tools.
> 
> that sounds scary. Do we need a bug filed for this?

This is actually not new code, but exists since the "scripts" packet was implemented. With new tools like about:ccdump we may be able to figure this one out. I'll file a bug.

> @@ +83,5 @@
> > +      this._dbg.uncaughtExceptionHook = this.uncaughtExceptionHook.bind(this);
> > +      this._dbg.onDebuggerStatement = this.onDebuggerStatement.bind(this);
> > +      this._dbg.onNewScript = this.onNewScript.bind(this);
> > +      // Keep the debugger disabled until a client attaches.
> > +      this.dbg.enabled = this._state != "detached";
> 
> a little funny moving from _dbg to dbg here. Might prefer to continue
> referencing the object directly inside the if block.

Yeah, I'll convert all of these in a followup.

Comment 53

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

https://hg.mozilla.org/integration/fx-team/rev/b2900e347f5c

Comment 54

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 55

[Ryan VanderMeulen [:RyanVM]]

And backed out for Windows xpcshell tests hangs in test_listscripts-01.js. I will point out that these hangs were also occurring on fx-team...
https://hg.mozilla.org/integration/mozilla-inbound/rev/f49728317fad

Comment 56

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

Attachment: fix test_listscripts-01.js hang

Not sure if this invalidates the *point* of the test or not, but it makes it pass.

Comment 57

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

Attachment: fix test_listscripts-01.js hang

Removed unrelated change from elsewhere in my patch queue.

Comment 58

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

Comment on attachment 654821 [details] [diff] [review]
fix test_listscripts-01.js hang

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

On the contrary, I'd say this change is actually more in line with the point of the test.

Comment 59

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

Relanded with the test fix:
https://hg.mozilla.org/integration/fx-team/rev/d6b2e1f60d02
https://hg.mozilla.org/integration/fx-team/rev/73622d728d4c

Comment 60

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 654585 [details] [diff] [review]
Patch v11

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New feature.
User impact if declined: Users won't be able to hit breakpoints after a reload or navigation. Worse, they will not know why.
Testing completed (on m-c, etc.): On m-c, locally.
Risk to taking this patch (and alternatives if risky): Minimal impact to regular users. Improvement to debugger is worth the risk of additional code, imo.
String or UUID changes made by this patch: none

Our plan/hope for aurora is to have this merged into central in the next couple of hours. In time for the nightly builders to produce a build for us tomorrow. We'll verify the fixes on that nightly and then push to aurora after we've made sure things are good.

Comment 61

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 654821 [details] [diff] [review]
fix test_listscripts-01.js hang

Agreed with comment 61 - once there's an m-c nightly verified this can land on aurora any time later today or over the weekend.

Comment 62

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Lukas Blakk [:lsblakk] from comment #61)
> Comment on attachment 654821 [details] [diff] [review]
> fix test_listscripts-01.js hang
> 
> Agreed with comment 61 - once there's an m-c nightly verified this can land
> on aurora any time later today or over the weekend.

Sigh. comment 60

Comment 63

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/d6b2e1f60d02
https://hg.mozilla.org/mozilla-central/rev/73622d728d4c

Comment 64

[Rob Campbell [:rc] (:robcee)]

Attachment: patch v11 Aurora

Comment 65

[Rob Campbell [:rc] (:robcee)]

verified fix is in nightly, 

[10:42:21.931] "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:17.0) Gecko/17.0 Firefox/17.0"

Comment 66

[Victor Porof [:vporof][:vp]]

Comment on attachment 655312 [details] [diff] [review]
patch v11 Aurora

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

Looks good.

::: browser/devtools/debugger/debugger-controller.js
@@ +426,5 @@
>    /**
> +   * Handles any initialization on a tab navigation event issued by the client.
> +   */
> +  _handleTabNavigation: function SF__handleTabNavigation(aCallback) {
> +    // Nothing to do here yet.

http://tinyurl.com/7jcf32a
Sorry I couldn't resist.

@@ +878,4 @@
>    connect: function SS_connect(aCallback) {
>      window.addEventListener("Debugger:LoadSource", this._onLoadSource, false);
>  
> +    this._handleTabNavigation();

You forgot a
    this.debuggerClient.addListener("newScript", this._onNewScript);
just before this call.

@@ +948,5 @@
> +    // XXX bug 783393, this differs from final implemenation. preferedScriptUrl.
> +    // the first entry if there's not one selected yet.
> +    if (!DebuggerView.Scripts.selected) {
> +      DebuggerView.Scripts.selectIndex(0);
> +    }

Yup, this should do it, as long as the test stays disabled.

In principle, you could remove this altogether if also backing out 783393. The original implementation in debugger-view made sure that the first script added is always selected.
But let's not overcomplicate our lives.

::: browser/devtools/debugger/test/browser_dbg_location-changes-blank.js
@@ +64,5 @@
> +
> +        is(gDebugger.DebuggerView.Scripts.selected, null,
> +          "There should be no selected script.");
> +        is(gDebugger.editor.getText().length, 0,
> +          "The source editor not have any text displayed.");

I think you may have missed some stuff here, checking the empty menulist state:

+        is(gDebugger.DebuggerView.Scripts.selected, null,
+          "There should be no selected script.");
+        is(gDebugger.editor.getText().length, 0,
+          "The source editor not have any text displayed.");

and so on (4 checks).

::: browser/devtools/debugger/test/browser_dbg_location-changes.js
@@ +56,2 @@
>  
> +      closeDebuggerAndFinish();

Hmm, are those menulist tests not in Aurora?
If not, ignore what I'm saying.

Comment 67

[Victor Porof [:vporof][:vp]]

Attachment: patch v11 Aurora fix

This should fix the "TypeError: DebuggerView.Scripts.selectIndex is not a function" thing.

Comment 68

[Victor Porof [:vporof][:vp]]

Attachment: patch v11 Aurora fix fix

Comment 69

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ea826c966c2f

Comment 70

[George Mauer]

I am seeing this behavior again on Firefox 30.

Comment 71

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

It works for me with the steps to reproduce from comment 0. Please file a new bug with the exact steps you took to observe the problem.

Comment 72

[Günter Zöchbauer]

I can reproduce this too. Firefox 30 Debian Linux x64.

Comment 73

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

See comment 71.

Comment 74

[George Mauer]

I would love to submit steps but not sure how to do that without starting to pull out things bit by bit from my large application and checking over and over again. In fact, if the problem went away before I removed absolutely everything client specific I wouldn't be able to submit it at all!

I of course appreciate how difficult it is to fix things without a reproducible case but telling people to submit steps is completely impractical for things like this. Is there any sort of diagnosis report or debugging mode that I can trigger to provide a log of whats going on? Otherwise I'm afraid you're limiting bug reporting on issues like this solely to people who understand firefox internals AND can replicate it AND care to dig in.

(In reply to Panos Astithas [:past] from comment #71)
> It works for me with the steps to reproduce from comment 0. Please file a
> new bug with the exact steps you took to observe the problem.

Comment 75

[George Mauer]

I would love to submit steps but not sure how to do that without starting to pull out things bit by bit from my large application and checking over and over again. In fact, if the problem went away before I removed absolutely everything client specific I wouldn't be able to submit it at all!

I of course appreciate how difficult it is to fix things without a reproducible case but telling people to submit steps is completely impractical for things like this. Is there any sort of diagnosis report or debugging mode that I can trigger to provide a log of whats going on? Otherwise I'm afraid you're limiting bug reporting on issues like this solely to people who understand firefox internals AND can replicate it AND care to dig in.

(In reply to Panos Astithas [:past] from comment #71)
> It works for me with the steps to reproduce from comment 0. Please file a
> new bug with the exact steps you took to observe the problem.

Comment 76

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

Rest assured that I appreciate you taking the time to report the problem here. However, as a matter of managing our workload, we prefer to file new bugs for similar issues, unless the committed fix no longer works for the exact same STR. We've had quite a few "breakpoints not working after a reload" bugs in the past, stemming from various issues that were not always related.

I cannot guarantee that your bug will be actionable without some more details, but I'm willing to look at it in any case. Let's followup there.