If we reload the page when the debugger is paused, we get: ###!!! ASSERTION: Mismatched calls to ResumeTimeouts!: 'mTimeoutsSuspendDepth', file /Users/past/src/fx-team/dom/base/nsGlobalWindow.cpp, line 9779 We have to prevent the reload this somehow, or resume before reloading.

FWIW, the inspector has a bit of code that watches/prevents a reload temporarily.

q.v., http://mxr.mozilla.org/mozilla-central/source/browser/devtools/highlighter/inspector.jsm#1206

I did get the debugger to exit the nested event loop before navigation, but that doesn't seem to avoid the assertion. Still investigating...

I got it working, but there is still cleanup to do and I need to fix a few minor issues.

All use cases now work as expected. I just need to do some more cleanup and write a test.

Completed cleanups and fixed the leak. Now I only need to write tests.

Comment on attachment 638764 [details] [diff] [review] Working patch Apparently I can't write a test that fails without the patch. This happens because the triggered assertion checks an internal counter that the test has no access to. Also, the debugger behaves as expected even without this patch and I assume in optimized builds the assertion wouldn't even be there. What the patch does is temporarily suspend any page navigation, while the debugger detaches from the tab. This way content scripts resume and the assertion is not triggered when the new page loads. Because the debugger is detached while the new window is being created, it misses the DOMWindowCreated event, so the progress listener has to send the tabNavigated message when the progress event completes.

Luckily this doesn't appear to be an issue for Fennec. I pored through pages of logcat spew and I didn't see this assertion or anything remotely similar.

Comment on attachment 638764 [details] [diff] [review] Working patch - return !window.parent.content && !this._isRemoteDebugger; + // Directly accessing window.parent.content may throw in some cases. + return !("content" in window.parent) && !this._isRemoteDebugger; could use hasOwnProperty there or (shudder) window.parent && window.parent.content... That should be ok though. disconnect: function BTA_disconnect() { this._detach(); + + if (this._progressListener) { + this._progressListener.destroy(); + } could we move the _progressListener check and destroy calls into _detach()? looking at _detach a bit more, we're doing a bunch of if (attached) _detach(). Probably some cleanup we could do around the senders of _detach() to check for a return of false in the detach() method and then do something special if needed. Suitable for followup. no test yet, eh?

Comment on attachment 638764 [details] [diff] [review] Working patch +function DebuggerProgressListener(aBrowserTabActor) { + this._tabActor = aBrowserTabActor; + this._tabActor._tabbrowser.addProgressListener(this); +} + +DebuggerProgressListener.prototype = { + onStateChange: + function DPL_onStateChange(aProgress, aRequest, aFlag, aStatus) { if we're only interested in refreshes, you could implement an nsIWebProgressListener2 and implement onRefreshAttempted(); http://mxr.mozilla.org/mozilla-central/source/uriloader/base/nsIWebProgressListener2.idl

(In reply to Rob Campbell [:rc] (:robcee) from comment #9) > Comment on attachment 638764 [details] [diff] [review] > Working patch > > - return !window.parent.content && !this._isRemoteDebugger; > + // Directly accessing window.parent.content may throw in some cases. > + return !("content" in window.parent) && !this._isRemoteDebugger; > > could use hasOwnProperty there or (shudder) window.parent && > window.parent.content... That should be ok though. window.parent && window.parent.content wouldn't work, because it's accessing the property that throws. hasOwnProperty should be fine if |content| is not a property on |parent|'s prototype chain, but I don't remember what it is that triggered the exception unfortunately, so I can't test it :-( > disconnect: function BTA_disconnect() { > this._detach(); > + > + if (this._progressListener) { > + this._progressListener.destroy(); > + } > > could we move the _progressListener check and destroy calls into _detach()? Unfortunately we can't, because we call detach in DPL_onStateChange, while the progress listener is still doing it's job. > looking at _detach a bit more, we're doing a bunch of if (attached) > _detach(). Probably some cleanup we could do around the senders of _detach() > to check for a return of false in the detach() method and then do something > special if needed. Suitable for followup. Yes, I will file. > no test yet, eh? Like I said in comment 7, I didn't find any way to write a test for this. There is no external API to count the calls to ResumeTimeouts and this bug is virtually invisible in optimized builds. (In reply to Rob Campbell [:rc] (:robcee) from comment #10) > Comment on attachment 638764 [details] [diff] [review] > Working patch > > +function DebuggerProgressListener(aBrowserTabActor) { > + this._tabActor = aBrowserTabActor; > + this._tabActor._tabbrowser.addProgressListener(this); > +} > + > +DebuggerProgressListener.prototype = { > + onStateChange: > + function DPL_onStateChange(aProgress, aRequest, aFlag, aStatus) { > > if we're only interested in refreshes, you could implement an > nsIWebProgressListener2 and implement onRefreshAttempted(); > > http://mxr.mozilla.org/mozilla-central/source/uriloader/base/ > nsIWebProgressListener2.idl This assertion is actually triggered in both refreshes and navigation to other pages and I verified that this patch fixed both cases (previous iterations fixed one or the other).

Comment on attachment 638764 [details] [diff] [review] Working patch ok, I'll make an execution decision and allow this as is without the test.

(In reply to Rob Campbell [:rc] (:robcee) from comment #12) > Comment on attachment 638764 [details] [diff] [review] > Working patch > > ok, I'll make an execution decision and allow this as is without the test. Thank you! (In reply to Panos Astithas [:past] from comment #11) > (In reply to Rob Campbell [:rc] (:robcee) from comment #9) > > looking at _detach a bit more, we're doing a bunch of if (attached) > > _detach(). Probably some cleanup we could do around the senders of _detach() > > to check for a return of false in the detach() method and then do something > > special if needed. Suitable for followup. > > Yes, I will file. Filed bug 773563 for this.