771655 - Debugger does not show up if any progress listener (e.g. NoScript) reads the WebProgress argument's DOMWindow property in onStateChange()

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Gabriela [:gaby2300]]

Steps to reproduce:

1. Open Aurora in a profile with NoScript installed.
2. Go to Aurora>Web Developer>Debugger or press Ctrl+Shift+S.
3. Notice a white flash in the lower part of the window but no debugger showing up.
4. Open the Addons Manager.
5. Disable or remove NoScript.
6. Go to Aurora>Web Developer>Debugger or press Ctrl+Shift+S.
7. The debugger shows up.

The expected would be the debugger showing up no matter what addon is installed.

Windows 7 and Mozilla/5.0 (Windows NT 6.1; rv:15.0) Gecko/20120706 Firefox/15.0a2

Comment 1

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

I don't know why NoScript does this, but it's apparently blocking the debugger's fetching of the scripts in the page, and forcibly closes the UI. CC'ing the author.

Comment 2

[Giorgio Maone [:ma1]]

Contrived test case to be executed in the browser chrome context, e.g. via Scratchpad (shift+F4) with "Browser" as the Environment:

<SNIPPET>

if ("_testPL" in window)
  Cc['@mozilla.org/docloaderservice;1'].getService(Ci.nsIWebProgress)
    .removeProgressListener(_testPL);

window._testPL = {
  START_DOC: Ci.nsIWebProgressListener.STATE_START | Ci.nsIWebProgressListener.STATE_IS_DOCUMENT,
  onStateChange: function(wp, req, stateFlags, status) {
    // if (/^data:/.test(req.name)) return;
    if ((stateFlags & this.START_DOC) === this.START_DOC) {
      wp.DOMWindow;
    }
  },
  QueryInterface: function(iid) {
    if (iid.equals(Ci.nsISupportsWeakReference) || 
        iid.equals(Ci.nsIWebProgressListener))
      return this;
    throw Cr.NS_ERROR_NO_INTERFACE;
  }
}

Cc['@mozilla.org/docloaderservice;1'].getService(Ci.nsIWebProgress)
  .addProgressListener(_testPL, Ci.nsIWebProgress.NOTIFY_STATE_REQUEST);

</SNIPPET>

After installing this progress listener, which just attempts to read the DOMWindow property from the first argument of onStateChange() when a document loads, the debugger doesn't show up. Notice that no exception is thrown when the property is read (it correctly evaluates to a ChromeWindow).

NoScript 2.4.7 and below happened to have a progress listener which triggers this bug (worked around in 2.4.8), and we can't exclude other extensions do as well.

Comment 3

[Thierry Régagnon]

I can't say if it's because of a progress listener, but I am on Firefox 15 and when I try to open the debugger using the Tools menu (or keyboard shortcut), it shows up blank at the bottom of the screen for less than a second.

Comment 4

[Michael Kraft [:morac]]

The debugger doesn't show up (well it opens and immediately closes) if Roboform is installed.  I don't know if that's caused by this issue or not though since Roboform obfuscates its code.

Comment 5

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

Roboform!

Comment 6

[Michael Kraft [:morac]]

(In reply to Rob Campbell [:rc] (:robcee) from comment #5)
> Roboform!

I've contacted Roboform support and pointed them towards this bug and they can confirm they are triggering this bug by reading DOMWindow property on a onStateChange to catch Basic Auth requests.  They don't believe there's any other way to do so so they can't work around the bug.

Comment 7

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

Attachment: Patch v1

So the problem is that something fires an unload event on the debugger iframe and that causes the frontend to shut down. I am avoiding this in this patch by returning early from the unload handler, but I haven't really figured out what triggers the unload event in the first place.

Comment 8

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

Attachment: Patch v2

This is a better fix for ignoring spurious unload events before the debugger has finished initializing. I still don't have a theory to explain why touching the DOMWindow from an nsIWebProgressListener triggers an unload event in the debugger iframe.

Comment 9

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

Comment on attachment 669908 [details] [diff] [review]
Patch v2

   _startupDebugger: function DC__startupDebugger() {
     if (this._isInitialized) {
       return;
     }
     this._isInitialized = true;
     window.removeEventListener("DOMContentLoaded", this._startupDebugger, true);
+    this._isLoaded = false;
+    window.addEventListener("Debugger:Loaded", this._onInitFinished, false);

I would love to see a unittest that can duplicate this condition. I'm a little worried about additional raciness adding two stages of load listeners. It's conceivable that these could fire pretty close together. Might be better putting the Debugger:Loaded listener before the DOMContentLoaded listener to ensure it doesn't get missed.

+   * initializing, so that _shutdowDebugger can ignore spurious unload events.

_shutdownDebugger - typo.

Comment 10

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

Attachment: Patch v3

Added test and made all suggested changes.

(In reply to Rob Campbell [:rc] (:robcee) from comment #9)
> I would love to see a unittest that can duplicate this condition. I'm a
> little worried about additional raciness adding two stages of load
> listeners. It's conceivable that these could fire pretty close together.
> Might be better putting the Debugger:Loaded listener before the
> DOMContentLoaded listener to ensure it doesn't get missed.

I did that even though I don't see how this could possibly happen, given that the second event is fired by the handler of the first one, effectively serializing the sequence. Nevertheless, better safe than sorry.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=3d06e53d68f1

Comment 11

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

Comment on attachment 672748 [details] [diff] [review]
Patch v3

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

this likely needs to be rebased. Canceling review.

Comment 12

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

Attachment: Patch v4

Rebased.

Comment 13

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

Comment on attachment 677000 [details] [diff] [review]
Patch v4

asking victor to take a pass through this.

Comment 14

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

Attachment: v5

As suspected, the extra listeners weren't necessary anymore. I stripped out them and kept the test to be safe.

Comment 15

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

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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