Closed Bug 783393 Opened 12 years ago Closed 12 years ago

Breakpoints not getting caught on reload

Categories

(DevTools :: Debugger, defect, P1)

x86
macOS
defect

Tracking

(firefox15+ disabled, firefox16+ fixed)

VERIFIED FIXED
Firefox 17
Tracking Status
firefox15 + disabled
firefox16 + fixed

People

(Reporter: rcampbell, Assigned: past)

References

Details

(Keywords: relnote, Whiteboard: [fixed-in-fx-team])

Attachments

(4 files, 15 obsolete files)

2.46 KB, patch
dcamp
: review+
Details | Diff | Splinter Review
29.54 KB, patch
past
: review+
Details | Diff | Splinter Review
1.17 KB, patch
past
: review+
Details | Diff | Splinter Review
24.74 KB, patch
Details | Diff | Splinter Review
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).
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...)
Attached patch proof of concept (obsolete) — Splinter Review
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.
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #652645 - Flags: feedback?(dcamp)
Priority: -- → P1
(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.
Attached patch another rev (obsolete) — Splinter Review
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?
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.
(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.
cache is hard.
Attached patch again (obsolete) — Splinter Review
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.
Attachment #652624 - Attachment is obsolete: true
Attachment #652645 - Attachment is obsolete: true
Attachment #652910 - Attachment is obsolete: true
Attachment #652645 - Flags: feedback?(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 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?
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 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?
(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.
(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.
adding tracking flags for aurora 16 and beta 15. This is affected on both.
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
(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?
(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.
Attached patch disable-nav-on-beta (obsolete) — Splinter Review
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.
Attachment #653533 - Flags: review?(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?
(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!
(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.
BUGZILLAAAAAAAA!!!
Sorry Lukas.
Attached patch disable-nav-on-beta-2 (obsolete) — Splinter Review
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.
Attachment #653533 - Attachment is obsolete: true
Attachment #653533 - Flags: review?(dcamp)
Attachment #653753 - Flags: review?
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.
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?
(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).
Attached patch Patch v5 (obsolete) — Splinter Review
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.
Assignee: nobody → past
Attachment #652951 - Attachment is obsolete: true
Status: NEW → ASSIGNED
3rd crack, based on feedback from panos.
Attachment #653753 - Attachment is obsolete: true
Attachment #653753 - Flags: review?
Attachment #653881 - Flags: review?(dcamp)
Comment on attachment 653881 [details] [diff] [review]
disable-nav-on-beta-3

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

Clever.  Thanks Rob.
Attachment #653881 - Flags: review?(dcamp) → review+
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.
Attachment #653881 - Flags: approval-mozilla-beta?
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).
Attachment #653881 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Sooner than 9/2!  8/31, even.
Attached patch Patch v6: "Always bet on dcamp" (obsolete) — Splinter Review
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.
Attachment #653821 - Attachment is obsolete: true
This should also fix bug 773732.
Attachment #653976 - Attachment is obsolete: true
Attached patch Patch v8 (obsolete) — Splinter Review
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.
Attachment #654003 - Attachment is obsolete: true
Attached patch Patch v9 (obsolete) — Splinter Review
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
Attachment #654192 - Attachment is obsolete: true
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.
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.
(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.
Attached patch Patch v10 (obsolete) — Splinter Review
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.
Attachment #654203 - Attachment is obsolete: true
Attachment #654235 - Flags: review?(rcampbell)
Attachment #654235 - Flags: review?(dcamp)
Keywords: relnote
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);
> +    }

:)
Attachment #654235 - Flags: review?(rcampbell) → review+
Blocks: 774788
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.
Attachment #654235 - Flags: review?(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.
(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.
(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.
Desktop B2G and Fennec are unaffected by this patch.
Attached patch Patch v11Splinter Review
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.
Attachment #654235 - Attachment is obsolete: true
Attachment #654585 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/b2900e347f5c
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 17
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 17 → ---
Attached patch fix test_listscripts-01.js hang (obsolete) — Splinter Review
Not sure if this invalidates the *point* of the test or not, but it makes it pass.
Attachment #654815 - Flags: review?
Removed unrelated change from elsewhere in my patch queue.
Attachment #654815 - Attachment is obsolete: true
Attachment #654815 - Flags: review?
Attachment #654821 - Flags: review?
Attachment #654821 - Attachment is patch: true
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.
Attachment #654821 - Flags: review? → review+
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.
Attachment #654585 - Flags: approval-mozilla-aurora?
Attachment #654821 - Flags: approval-mozilla-aurora?
Attachment #654585 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #654821 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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
https://hg.mozilla.org/mozilla-central/rev/d6b2e1f60d02
https://hg.mozilla.org/mozilla-central/rev/73622d728d4c
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
Attached patch patch v11 Aurora (obsolete) — Splinter Review
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"
Status: RESOLVED → VERIFIED
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.
Attachment #655312 - Flags: feedback+
Attached patch patch v11 Aurora fix (obsolete) — Splinter Review
This should fix the "TypeError: DebuggerView.Scripts.selectIndex is not a function" thing.
Attachment #655312 - Attachment is obsolete: true
Attachment #655320 - Attachment is obsolete: true
I am seeing this behavior again on Firefox 30.
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.
I can reproduce this too. Firefox 30 Debian Linux x64.
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.
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.
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.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.