Closed Bug 787181 Opened 9 years ago Closed 9 years ago

DebuggerController._isChromeDebugger is broken


(DevTools :: Debugger, defect, P2)



(Not tracked)

Firefox 18


(Reporter: vporof, Assigned: vporof)



(1 file, 1 obsolete file)

Although it's disabled by default, it's something we should fix.
Afaict, the remoteIncomingPromptMessage dialog never shows up, resulting in the DebuggerServer never initializing, resulting in the second process throwing up some errors and never being able to close.

PS, after making it work again, we also need to change
Services.prefs.setBoolPref("devtools.debugger.remote-enabled", false);
to also be accompanied by a 
Services.prefs.setBoolPref("", false);
Actually a regression from bug 751949.
Summary: ChromeDebuggerProcess._allowConnection is broken → DebuggerController._isChromeDebugger is broken
Assignee: nobody → vporof
Attached patch v1 (obsolete) — Splinter Review
Attachment #660459 - Flags: review?(past)
Interestingly enough, the simple change
-    return !("content" in window.parent) && !this._isRemoteDebugger;
+    let hasContent;
+    try {
+      hasContent = !!window.parent.content;
+    } catch (e) {
+      // If an exception was thrown, we can't rely on the result to be accurate.
+      return false;
+    }
+    return !hasContent && !this._isRemoteDebugger;

causes leaks...
This is pretty obvious now, since before this patch !("content" in window.parent) would always be false, hence the function would always return false. Things are different now and the function returns The Right Thing in some cases, instead of The Wrong Thing in all cases.


TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_menustatus.js | leaked until shutdown [nsGlobalWindow #242 about:blank]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_panesize-inner.js | leaked until shutdown [nsGlobalWindow #280 about:blank]

Both tests share the same pattern of listening for Debugger:Loaded, doing some stuff, and then handling Debugger.Unloaded for cleanup. We're in callback hell again.
Even more, these leaks didn't appear before because of TypeErrors.

JavaScript error: chrome://browser/content/debugger-controller.js, line 265: TypeError: invalid 'in' operand window.parent
Attached patch v2Splinter Review
Removed previous remote/chrome checks with something much much simpler.
Fixed tests.

Attachment #660459 - Attachment is obsolete: true
Attachment #660459 - Flags: review?(past)
Attachment #660775 - Flags: review?(past)
Priority: -- → P2
Comment on attachment 660775 [details] [diff] [review]

Review of attachment 660775 [details] [diff] [review]:

::: browser/devtools/debugger/DebuggerUI.jsm
@@ +494,5 @@
>      }
>      if (result == 2) {
>        DebuggerServer.closeListener();
>        Services.prefs.setBoolPref("devtools.debugger.remote-enabled", false);
> +      Services.prefs.setBoolPref("", false);

As discussed in IRC, I don't think we want to do this.
Attachment #660775 - Flags: review?(past) → review+
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 18
Whoops, switched the wrong pref when landed, fixed:
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.