Closed Bug 787181 Opened 8 years ago Closed 8 years ago

DebuggerController._isChromeDebugger is broken

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 18

People

(Reporter: vporof, Assigned: vporof)

Details

Attachments

(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("devtools.debugger.chrome-enabled", false);
Actually a regression from bug 751949.
Summary: ChromeDebuggerProcess._allowConnection is broken → DebuggerController._isChromeDebugger is broken
Assignee: nobody → vporof
Status: NEW → ASSIGNED
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.

Investigating...

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.

JAVASCRIPT!
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]
v2

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("devtools.debugger.chrome-enabled", false);

As discussed in IRC, I don't think we want to do this.
Attachment #660775 - Flags: review?(past) → review+
https://hg.mozilla.org/integration/fx-team/rev/69848e5d3672
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 18
https://hg.mozilla.org/mozilla-central/rev/69848e5d3672
https://hg.mozilla.org/mozilla-central/rev/3f7d16ee5712
Status: ASSIGNED → RESOLVED
Closed: 8 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.