Closed
Bug 787181
Opened 12 years ago
Closed 12 years ago
DebuggerController._isChromeDebugger is broken
Categories
(DevTools :: Debugger, defect, P2)
DevTools
Debugger
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 18
People
(Reporter: vporof, Assigned: vporof)
Details
Attachments
(1 file, 1 obsolete file)
7.87 KB,
patch
|
past
:
review+
|
Details | Diff | Splinter Review |
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);
Assignee | ||
Comment 1•12 years ago
|
||
Actually a regression from bug 751949.
Summary: ChromeDebuggerProcess._allowConnection is broken → DebuggerController._isChromeDebugger is broken
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #660459 -
Flags: review?(past)
Assignee | ||
Comment 3•12 years ago
|
||
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.
Assignee | ||
Comment 4•12 years ago
|
||
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
Assignee | ||
Comment 5•12 years ago
|
||
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)
Assignee | ||
Updated•12 years ago
|
Priority: -- → P2
Comment 6•12 years ago
|
||
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+
Assignee | ||
Comment 7•12 years ago
|
||
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 18
Assignee | ||
Comment 8•12 years ago
|
||
Whoops, switched the wrong pref when landed, fixed:
https://hg.mozilla.org/integration/fx-team/rev/3f7d16ee5712
Comment 9•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/69848e5d3672
https://hg.mozilla.org/mozilla-central/rev/3f7d16ee5712
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•