Last Comment Bug 702707 - Web console is buggy in window mode
: Web console is buggy in window mode
Status: RESOLVED FIXED
[qa+][qa!:10]
: verified-beta
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: x86 Mac OS X
: -- blocker (vote)
: Firefox 11
Assigned To: Rob Campbell [:rc] (:robcee)
:
Mentors:
Depends on: 674589
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-15 11:24 PST by Benjamin Stover (:stechz)
Modified: 2012-01-03 13:11 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified


Attachments
zombie killer (1.16 KB, patch)
2011-11-15 16:22 PST, Rob Campbell [:rc] (:robcee)
dcamp: feedback+
Details | Diff | Splinter Review
zombie killer with test (3.50 KB, patch)
2011-11-15 17:06 PST, Rob Campbell [:rc] (:robcee)
dcamp: review+
Details | Diff | Splinter Review
panel width adjustment (1.36 KB, patch)
2011-11-17 13:29 PST, Rob Campbell [:rc] (:robcee)
mihai.sucan: review+
Details | Diff | Splinter Review
zombie killer part deux (4.28 KB, patch)
2011-12-03 05:49 PST, Rob Campbell [:rc] (:robcee)
no flags Details | Diff | Splinter Review
the return of the zombie killer (5.96 KB, patch)
2011-12-06 11:54 PST, Rob Campbell [:rc] (:robcee)
mihai.sucan: review+
asa: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Benjamin Stover (:stechz) 2011-11-15 11:24:09 PST
At least on Mac, when going to windowed mode, if I use the keyboard shortcut to close it (mac-shift-K) the console remains and then is completely unresponsive until I quit Firefox.

STR:
1. Type mac shift K
2. Change position -> window
3. Type mac shift K

Expected:
1. Window should close
2. Window becomes zombie-like
Comment 1 Rob Campbell [:rc] (:robcee) 2011-11-15 16:22:54 PST
Created attachment 574722 [details] [diff] [review]
zombie killer

needs a test still, this could use a little round of feedback though.
Comment 2 Rob Campbell [:rc] (:robcee) 2011-11-15 16:24:58 PST
also worth noting that the titlebar on the panel appears to be stealing focus. If you drag the panel to a new position and hit Cmd-K, nothing happens. This is surprising. If you focus the text area in (or any other browser widget) the toggle method gets fired. Probably another focus-related panel bug.
Comment 3 Rob Campbell [:rc] (:robcee) 2011-11-15 17:06:22 PST
Created attachment 574744 [details] [diff] [review]
zombie killer with test

updated
Comment 4 Rob Campbell [:rc] (:robcee) 2011-11-17 09:26:19 PST
https://hg.mozilla.org/integration/fx-team/rev/efd4925dc649
Comment 5 Rob Campbell [:rc] (:robcee) 2011-11-17 11:19:02 PST
backout:

https://hg.mozilla.org/integration/fx-team/rev/a43d24581da0
Comment 6 Rob Campbell [:rc] (:robcee) 2011-11-17 13:29:53 PST
Created attachment 575283 [details] [diff] [review]
panel width adjustment

I think this'll help. Based on feedback from mihai in IRC.
Comment 7 Mihai Sucan [:msucan] 2011-11-17 13:37:23 PST
Comment on attachment 575283 [details] [diff] [review]
panel width adjustment

I am not sure if this is going to fix the memleak. Maybe go for a try push?
Comment 8 Benjamin Stover (:stechz) 2011-11-29 11:41:51 PST
Great work so far Rob :D
Comment 9 Rob Campbell [:rc] (:robcee) 2011-11-29 12:31:19 PST
No, I haven't forgotten about this. Thanks.
Comment 10 Rob Campbell [:rc] (:robcee) 2011-12-03 05:49:43 PST
Created attachment 578830 [details] [diff] [review]
zombie killer part deux

combined patches, fixed leak.

try results here: https://tbpl.mozilla.org/?tree=Try&rev=c407cf03b45b
Comment 11 Rob Campbell [:rc] (:robcee) 2011-12-05 07:10:40 PST
Comment on attachment 578830 [details] [diff] [review]
zombie killer part deux

canceling review for one more little fix. Need to remove the consolePanel reference and node in positionConsole: to account for repositioning.
Comment 12 Rob Campbell [:rc] (:robcee) 2011-12-06 11:54:06 PST
Created attachment 579395 [details] [diff] [review]
the return of the zombie killer

updated and refreshed. Now with 100% more zombie-killing awesomeness!
Comment 13 Mihai Sucan [:msucan] 2011-12-06 12:47:47 PST
Comment on attachment 579395 [details] [diff] [review]
the return of the zombie killer

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

Thanks for the patch update. This looks better to me now. Just a couple of comments below...

::: browser/devtools/webconsole/HUDService.jsm
@@ -3335,5 @@
>  
> -      /*
> -       * Removed because of bug 674562
> -       * Services.prefs.setIntPref("devtools.webconsole.top", panel.panelBox.y);
> -       * Services.prefs.setIntPref("devtools.webconsole.left", panel.panelBox.x);

Probably we can revive all this now, just by changing from popuphidden to popuphiding.

(this would also probably save us from commenting-out parts of browser_webconsole_position_ui.js)

@@ +6193,4 @@
>  
>      if (hudRef && hud) {
>        if (hudRef.consolePanel) {
> +        hudRef.consolePanel.hidePopup();

Why this change here? This is the only one that doesn't seem to make sense for me. The web console won't close, it will only hide itself...
Comment 14 Rob Campbell [:rc] (:robcee) 2011-12-07 06:06:58 PST
hidePopup() calls deactivateHUDForContext if necessary.
Comment 15 Rob Campbell [:rc] (:robcee) 2011-12-07 06:31:08 PST
(In reply to Mihai Sucan [:msucan] from comment #13)
> Comment on attachment 579395 [details] [diff] [review] [diff] [details] [review]
> the return of the zombie killer
> 
> Review of attachment 579395 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch update. This looks better to me now. Just a couple of
> comments below...
> 
> ::: browser/devtools/webconsole/HUDService.jsm
> @@ -3335,5 @@
> >  
> > -      /*
> > -       * Removed because of bug 674562
> > -       * Services.prefs.setIntPref("devtools.webconsole.top", panel.panelBox.y);
> > -       * Services.prefs.setIntPref("devtools.webconsole.left", panel.panelBox.x);
> 
> Probably we can revive all this now, just by changing from popuphidden to
> popuphiding.

nope.

Error: panel.panelBox is undefined
Source File: resource:///modules/HUDService.jsm
Line: 3336

> (this would also probably save us from commenting-out parts of
> browser_webconsole_position_ui.js)

also, nope.

I think we're going to be better off landing this as-is. Further mucking around with the preferences setting in a panel that's being torn down isn't going to work.
Comment 16 Rob Campbell [:rc] (:robcee) 2011-12-07 06:40:09 PST
https://hg.mozilla.org/integration/fx-team/rev/bb8169ddc939
Comment 17 Mihai Sucan [:msucan] 2011-12-07 07:42:23 PST
(In reply to Rob Campbell [:rc] (robcee) from comment #15)
> (In reply to Mihai Sucan [:msucan] from comment #13)
> > Comment on attachment 579395 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > the return of the zombie killer
> > 
> > Review of attachment 579395 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]:
> > -----------------------------------------------------------------
> > 
> > Thanks for the patch update. This looks better to me now. Just a couple of
> > comments below...
> > 
> > ::: browser/devtools/webconsole/HUDService.jsm
> > @@ -3335,5 @@
> > >  
> > > -      /*
> > > -       * Removed because of bug 674562
> > > -       * Services.prefs.setIntPref("devtools.webconsole.top", panel.panelBox.y);
> > > -       * Services.prefs.setIntPref("devtools.webconsole.left", panel.panelBox.x);
> > 
> > Probably we can revive all this now, just by changing from popuphidden to
> > popuphiding.
> 
> nope.
> 
> Error: panel.panelBox is undefined
> Source File: resource:///modules/HUDService.jsm
> Line: 3336

Ah, ugly. We will switch some day to a real xul:window and no longer rely on the panel, anyway.
Comment 18 Blair McBride [:Unfocused] (UNAVAILABLE) 2011-12-07 17:19:15 PST
https://hg.mozilla.org/mozilla-central/rev/bb8169ddc939
Comment 19 Rob Campbell [:rc] (:robcee) 2011-12-09 06:50:29 PST
Comment on attachment 579395 [details] [diff] [review]
the return of the zombie killer

I'd like to get this into aurora. This fixes a critical bug where a user can end up with a zombie panel on their screen on OS X computers. It is impossible to close this panel without resorting to deep JavaScript juju.

Risk is relatively low-risk, has been tested on all platforms and comes with its own test.
Comment 21 Vlad [QA] 2012-01-03 08:40:22 PST
I have tried using the steps from the description on:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:10.0) Gecko/20100101 Firefox/10.0 beta 2
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:10.0) Gecko/20100101 Firefox/10.0 beta 2

The window is closing as expected on both OSes.

Setting resolution to Verified Fixed on Beta.

Note You need to log in before you can comment on or make changes to this bug.