751949 - Reloading a page when the debuggee is paused results in: ASSERTION: Mismatched calls to ResumeTimeouts

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

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

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)]

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

Comment 2

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

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

Comment 3

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

Attachment: 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 (he/him) [:past] (please ni?)]

Attachment: 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 (he/him) [:past] (please ni?)]

Attachment: 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 (he/him) [:past] (please ni?)]

Attachment: Working patch

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

Comment 7

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

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 (he/him) [:past] (please ni?)]

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)]

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)]

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 (he/him) [:past] (please ni?)]

(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)]

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 (he/him) [:past] (please ni?)]

(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 (he/him) [:past] (please ni?)]

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

Comment 15

[Ed Morley [:emorley]]

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