605023 - nsChromeRegistry::ReloadChrome() not working properly on builds after 2010-10-15

Status: VERIFIED FIXED

(Core :: General, defect)

Description

[Pardal Freudenthal (:ShareBird)]

I've noticed that the 16th build from Firefox broke my "Switch Themes" extension (www.silvermel.net/dev/xpi/switchthemes_0.2.0pre_trunk_r3835.xpi)... 
Debugging the script I've found out that the nsChromeRegistry::ReloadChrome() isn't working properly.

There are three different behaviors of it depending of the Firefox (or Thunderbird) versions:

On Firefox 3.6.x it reloads chrome without closing any windows.

On Firefox 4.0b8pre until 2010-10-15 it closes all opened windows, reloads the chrome and reopen the closed windows. (I have to say it was a fantastic change, I was astonished how flawlessly my extension worked with it).

On Firefox 2010-10-16 and 2010-10-17, it closes all windows and reopen only the error console. If I close the error console, it doesn't close the application at all (it is still the process running). On the error console only unrelated (I guess)errors (Error: document is null
Source File: chrome://global/content/consoleBindings.xml
Line: 237)

Steps to reproduce:
It's possible to observe those behaviors on each of those versions from Firefox:
1. Open the Error Console
2. Copy and Paste this code and evaluate it: Components.classes["@mozilla.org/chrome/chrome-registry;1"].getService(Components.interfaces.nsIXULChromeRegistry).reloadChrome()

Comment 1

[Pardal Freudenthal (:ShareBird)]

The regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=19cb42fa4554&tochange=397c458b40a4

Comment 2

[Boris Zbarsky [:bzbarsky]]

Hmm.  When did the "close all windows" behavior start?

Jim, could this be due to not calling Show() on toplevel windows anymore?

Comment 3

[Pardal Freudenthal (:ShareBird)]

(In reply to comment #2)
> Hmm.  When did the "close all windows" behavior start?

It started with the 2010-06-25 http://hg.mozilla.org/mozilla-central/rev/3a9f81291788 build. The 2010-06-24 didn't close the windows

The regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=68d98f30eda0&tochange=3a9f81291788

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah, that has the patches for bug 513162...

Comment 5

[Benjamin Smedberg]

We don't use this API at all in-product, so I don't think this blocks. I'd love for somebody to pick it up and fix it, though. I'm pretty sure we want to maintain the Firefox 3.6.x behavior.

Comment 6

[Jim Mathies [:jimm]]

Attachment: preserve top level windows patch v.1

nsDocumentViewer's Hide is the problem here.  I need to do some testing and run this through try before seeking reviews but this looks to address the problem.

Comment 7

[Jim Mathies [:jimm]]

I guess we don't have a test for this either, I'll see if I can work one up.

Comment 8

[Jim Mathies [:jimm]]

renominating after discussing with bsmedberg. This call leaves a zombie process running so it should be fix for the release.

Comment 9

[Benjamin Smedberg]

Yeah, zombies are bad.

Comment 10

[Jim Mathies [:jimm]]

(In reply to comment #7)
> I guess we don't have a test for this either, I'll see if I can work one up.

Hmm, a test for this doesn't seem possible since reloading chrome blows away everything in the window, including harness code.

Comment 11

[Pardal Freudenthal (:ShareBird)]

First of all, thank you very much for taking care of this bug!

This is now (with the improvements on Firefox 4.0) a powerful tool for extensions
developers. They are able to see changes on the chrome practically "on the fly"
with it. On my extension I use also the session store so I can reload the
chrome without loosing tabs and windows. It's so useful that I have to stay
with the 2010-10-15 build to develop my extensions...
Combined with dss and the session store API (as I do on my extension) I can
switch themes on my old PIII 1.1Ghz in just two seconds using the nightlies
(before 2010-10-16).

Maybe you could use the extension (comment #1) to test it. It has a "Reload Chrome" toolbarbutton (available on "Customize") that reloads the chrome and restore the browser windows.

Comment 12

[Jim Mathies [:jimm]]

Comment on attachment 483944 [details] [diff] [review]
preserve top level windows patch v.1

This passed try fine. I've also confirmed these changes and the changes in bug 574690 aren't the cause bug 606217.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Jim, can you explain what this patch is really doing?  Why is mAttachedToParent the right test here?

Comment 14

[Jim Mathies [:jimm]]

(In reply to comment #13)
> Jim, can you explain what this patch is really doing?  Why is mAttachedToParent
> the right test here?

If mAttachedToParent is true, mWindow points to the top level window and there are no widget children. We don't want to hide that window, but we do want to go through all the normal tear down that Hide() does. We reset mWindow to the top level later in MakeWindow, which gets called in InitInternal.

FWIW, we rarely seem to call Hide(). I was looking for other uses but couldn't find any in the code. It's called once on startup when we're transitioning from about:blank to the chrome document, and then that's it. ReloadChrome is the only trigger I've found thus far. It's not called on shutdown. If you have any ideas how it might get called I'd love to test them with this patch.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 483944 [details] [diff] [review]
preserve top level windows patch v.1

Ah, I see.   Just wasn't sure how mAttachedToParent fit in from the comments on it; reading the code that sets it makes that clearer... ;)

r=me

Comment 16

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/fc2352fe2c8e

Comment 17

[Pardal Freudenthal (:ShareBird)]

Thank you.