Last Comment Bug 751949 - Reloading a page when the debuggee is paused results in: ASSERTION: Mismatched calls to ResumeTimeouts
: Reloading a page when the debuggee is paused results in: ASSERTION: Mismatche...
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: Firefox 16
Assigned To: Panos Astithas [:past]
:
: James Long (:jlongster)
Mentors:
Depends on:
Blocks: 773563 1299602
  Show dependency treegraph
 
Reported: 2012-05-04 10:46 PDT by Panos Astithas [:past]
Modified: 2016-09-01 02:40 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (6.42 KB, patch)
2012-06-21 10:53 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
WIP 2 (13.40 KB, patch)
2012-06-22 15:32 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
WIP 3 (10.71 KB, patch)
2012-07-02 10:41 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Working patch (9.85 KB, patch)
2012-07-03 09:22 PDT, Panos Astithas [:past]
rcampbell: review+
Details | Diff | Splinter Review

Description Panos Astithas [:past] 2012-05-04 10:46:46 PDT
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.
Comment 1 Dave Camp (:dcamp) 2012-05-04 16:12:22 PDT
FWIW, the inspector has a bit of code that watches/prevents a reload temporarily.
Comment 3 Panos Astithas [:past] 2012-06-21 10:53:15 PDT
Created attachment 635375 [details] [diff] [review]
WIP

I did get the debugger to exit the nested event loop before navigation, but that doesn't seem to avoid the assertion. Still investigating...
Comment 4 Panos Astithas [:past] 2012-06-22 15:32:09 PDT
Created attachment 635946 [details] [diff] [review]
WIP 2

I got it working, but there is still cleanup to do and I need to fix a few minor issues.
Comment 5 Panos Astithas [:past] 2012-07-02 10:41:27 PDT
Created attachment 638411 [details] [diff] [review]
WIP 3

All use cases now work as expected. I just need to do some more cleanup and write a test.
Comment 6 Panos Astithas [:past] 2012-07-03 09:22:27 PDT
Created attachment 638764 [details] [diff] [review]
Working patch

Completed cleanups and fixed the leak. Now I only need to write tests.
Comment 7 Panos Astithas [:past] 2012-07-04 03:02:12 PDT
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.
Comment 8 Panos Astithas [:past] 2012-07-04 08:40:36 PDT
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 9 Rob Campbell [:rc] (:robcee) 2012-07-10 10:11:57 PDT
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 10 Rob Campbell [:rc] (:robcee) 2012-07-10 10:14:33 PDT
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
Comment 11 Panos Astithas [:past] 2012-07-10 11:38:42 PDT
(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 12 Rob Campbell [:rc] (:robcee) 2012-07-12 12:51:03 PDT
Comment on attachment 638764 [details] [diff] [review]
Working patch

ok, I'll make an execution decision and allow this as is without the test.
Comment 13 Panos Astithas [:past] 2012-07-13 01:45:17 PDT
(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.
Comment 14 Panos Astithas [:past] 2012-07-13 05:20:53 PDT
https://hg.mozilla.org/integration/fx-team/rev/c9371c78dfb5
Comment 15 Ed Morley [:emorley] 2012-07-16 05:27:31 PDT
https://hg.mozilla.org/mozilla-central/rev/c9371c78dfb5

Note You need to log in before you can comment on or make changes to this bug.