Last Comment Bug 674562 - Web Console windows don't close on osx
: Web Console windows don't close on osx
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Rob Campbell [:rc] (:robcee)
:
: J. Ryan Stinnett [:jryans] (use ni?)
Mentors:
Depends on: 674589
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-27 09:19 PDT by Dave Camp (:dcamp)
Modified: 2011-08-04 09:58 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
[in-fx-team] zombie hud fix (8.74 KB, patch)
2011-07-27 10:23 PDT, Rob Campbell [:rc] (:robcee)
mihai.sucan: review+
Details | Diff | Splinter Review
zombie hud fix for aurora/beta (8.81 KB, patch)
2011-07-28 06:55 PDT, Rob Campbell [:rc] (:robcee)
gavin.sharp: review-
asa: approval‑mozilla‑aurora-
asa: approval‑mozilla‑beta-
Details | Diff | Splinter Review

Description Dave Camp (:dcamp) 2011-07-27 09:19:07 PDT
They become immortal zombie windows.
Comment 1 Rob Campbell [:rc] (:robcee) 2011-07-27 09:19:54 PDT
immortal, brain-eating zombie windows.
Comment 2 Rob Campbell [:rc] (:robcee) 2011-07-27 10:23:38 PDT
Created attachment 548823 [details] [diff] [review]
[in-fx-team] zombie hud fix

fix for zombied panels on os x.

Some loss of functionality with this. No longer setting window positions on close. We'll need another mechanism to do that reliably on OS X.
Comment 3 Mihai Sucan [:msucan] 2011-07-27 12:36:59 PDT
Comment on attachment 548823 [details] [diff] [review]
[in-fx-team] zombie hud fix

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

The patch is fine, but I believe we need to store the top/left xul:panel location in some other way.

::: browser/devtools/webconsole/HUDService.jsm
@@ +3247,5 @@
>      }
>  
>      if (aPosition == "window") {
> +      let closeButton = this.consoleFilterToolbar.
> +        getElementsByClassName("webconsole-close-button")[0];

nit: I would favor querySelector(".webconsole-close-button").
Comment 4 Rob Campbell [:rc] (:robcee) 2011-07-28 05:48:44 PDT
(In reply to comment #3)
> Comment on attachment 548823 [details] [diff] [review] [review]
> zombie hud fix
> 
> Review of attachment 548823 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> The patch is fine, but I believe we need to store the top/left xul:panel
> location in some other way.
> 
> ::: browser/devtools/webconsole/HUDService.jsm
> @@ +3247,5 @@
> >      }
> >  
> >      if (aPosition == "window") {
> > +      let closeButton = this.consoleFilterToolbar.
> > +        getElementsByClassName("webconsole-close-button")[0];
> 
> nit: I would favor querySelector(".webconsole-close-button").

oh yeah. Good catch. Had a bit of a brain cramp there.

This is going to need a similar patch for aurora and possibly beta channels.
Comment 5 Rob Campbell [:rc] (:robcee) 2011-07-28 05:52:03 PDT
Comment on attachment 548823 [details] [diff] [review]
[in-fx-team] zombie hud fix

http://hg.mozilla.org/integration/fx-team/rev/27a921f53d73
Comment 6 Rob Campbell [:rc] (:robcee) 2011-07-28 06:55:56 PDT
Created attachment 549097 [details] [diff] [review]
zombie hud fix for aurora/beta

would like to get this in on aurora and beta channels as well. This patch should apply to both.
Comment 7 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-28 08:46:54 PDT
Comment on attachment 549097 [details] [diff] [review]
zombie hud fix for aurora/beta

Can you explain what was causing the bug, and why this fixes it?

I don't really understand why you need to remove the panel from the document at all (rather than just showing/hiding it). The comment about not storing the position doesn't make it clear _why_ you can't save the position (the bug reference doesn't help because this bug doesn't explain that either). What happens if people already have a stored position? Are they going to continue to be broken?

The querySelector call in this patch is missing a leading "." (though you seem to have fixed that in the patch you landed on fx-team). You also left in a getElementsByClassName, seems like you should be consistent.
Comment 8 Rob Campbell [:rc] (:robcee) 2011-07-28 11:30:04 PDT
(In reply to comment #7)
> Comment on attachment 549097 [details] [diff] [review] [review]
> zombie hud fix for aurora/beta
> 
> Can you explain what was causing the bug, and why this fixes it?

Speaking with Neil Deakin, he asked if we were doing anything in a popuphiding event listener. I said yes. He suggested that the operating system was causing the nsIFrame to detach from it in gecko before the event handler could complete. It's a bug in the underlying widget code. See bug 674589.

> I don't really understand why you need to remove the panel from the document
> at all (rather than just showing/hiding it). The comment about not storing
> the position doesn't make it clear _why_ you can't save the position (the
> bug reference doesn't help because this bug doesn't explain that either).
> What happens if people already have a stored position? Are they going to
> continue to be broken?

We would end up polluting the XULDocument with a bunch of leftover console panels. Probably wouldn't be a huge deal, but it's consistent with what we do for the "docked" version.

People with a stored position should see their window open in the previously saved position. It just won't update anymore.

This is preferably, imo, to not being able to close the window at all.

> The querySelector call in this patch is missing a leading "." (though you
> seem to have fixed that in the patch you landed on fx-team). You also left
> in a getElementsByClassName, seems like you should be consistent.

Oops, thanks for that. I manually applied the patch and will update it.
Comment 9 Asa Dotzler [:asa] 2011-07-28 14:47:16 PDT
Comment on attachment 549097 [details] [diff] [review]
zombie hud fix for aurora/beta

not likely that any patch here would make Beta. We'll track this and if you can produce a safe-looking positively reviewed patch, please nominate for Aurora.
Comment 10 Rob Campbell [:rc] (:robcee) 2011-07-29 08:39:36 PDT
An alternative could be to wrap the windowing position code behind a pref and turn it off. It's fairly isolated code and should be easy to do. Does that sound like it'd be within the realms of acceptable risk? I don't want to release this with the windowing code with this bug in place.
Comment 11 Rob Campbell [:rc] (:robcee) 2011-07-29 09:02:32 PDT
landed in m-c:
http://hg.mozilla.org/mozilla-central/rev/27a921f53d73
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-08-01 10:46:58 PDT
I don't think there's anything necessarily wrong with the previous patch such that it wouldn't be suitable for beta/aurora, I just didn't understand what it does. I'm still not clear on what is changing and why. A summary of the order of actions/events and how they're changing would be useful, otherwise I'll need to sort it out myself.

> We would end up polluting the XULDocument with a bunch of leftover console
> panels.

I just missed the fact that these panels were dynamically created.
Comment 13 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-08-01 10:55:45 PDT
Just to elaborate on what isn't clear, I see several different changes:
- panel is now removed in popuphidden, rather than popuphiding
- an added if (panel.parentNode) check before removing the panel
- disabling of the position saving
- we now hide the close button in some cases
- width pref sanity check is now > 0 instead of > -1

Which of those changes is necessary for fixing the bug as it is summarized? If "all", why are they all needed? If not "all", why are the other changes wrapped into this patch? (I'm not implying that they shouldn't be, I just want to understand the reasoning)

Sorry if this stuff is somehow really obvious and annoying to spell out, but just a short sentence describing why each of these changes were made would be extremely helpful to me (as someone who wasn't involved with the debugging or IRC discussion about this bug).
Comment 14 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-08-01 11:02:51 PDT
(In reply to comment #13)
> - panel is now removed in popuphidden, rather than popuphiding

Er, sorry, that isn't quite right. More accurately:
- bunch of teardown stuff is now done in popuphidden, rather than popuphiding

Is this what broke saving the position (position can't be saved that late)?
Comment 15 Rob Campbell [:rc] (:robcee) 2011-08-02 04:48:15 PDT
(In reply to comment #14)
> (In reply to comment #13)
> > - panel is now removed in popuphidden, rather than popuphiding
> 
> Er, sorry, that isn't quite right. More accurately:
> - bunch of teardown stuff is now done in popuphidden, rather than popuphiding
> 
> Is this what broke saving the position (position can't be saved that late)?

Yes. The panel's already gone (in popuphiding) by the time we retrieve and try to set the preferences for the panel coordinates. Moving that teardown code to popuphidden is what fixes this method.

if (panel.parentNode) check is done in case the Web Console's notification box has been removed. I think this can happen if the tab is removed and we're in windowed mode, but I'd have to go through this patch again to remember exactly why that check is needed, or indeed if it is needed. This is the one chunk of this patch we might be able to ditch, but I'd have to re-verify that.

The position saving couldn't work on OS X. It was better to remove it. We need to file a follow-up to reintroduce this based on some other event (e.g., panelmove or resize).

We hide the close button on the Toolbar when the Web Console is in the windowed position. There are problems with this close button on Mac and Windows. Hiding it and forcing users to use the supplied titlebar close button on the web console fixes this.

Width is initialized to 0 in the function. The sanity check should account for that. Might also be optional.
Comment 16 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-08-03 14:54:03 PDT
Which of the things done in popuphiding were causing the zombie behavior? Does just removing the popuphiding listener entirely (and making no other changes) make the panel closeable again?
Comment 17 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-08-03 14:56:04 PDT
Just saw bug 674589 comment 3, which presumably describes this situation. That seems very odd.
Comment 18 Rob Campbell [:rc] (:robcee) 2011-08-04 09:58:30 PDT
(In reply to comment #16)
> Which of the things done in popuphiding were causing the zombie behavior?
> Does just removing the popuphiding listener entirely (and making no other
> changes) make the panel closeable again?

it would, though we'd lose the behavior that we were doing in that listener.

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