408268 - Closing the browser without dismissing the Customize Toolbar dialog leads to loss of all toolbar items

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P2)

Description

[(no longer active)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b1) Gecko/2007110904 Firefox/3.0b1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b1) Gecko/2007110904 Firefox/3.0b1

This is major.  Accidentally closing the browser during customization of the toolbar could lead to loss of everything on the toolbars, including any previous customizations.  (That's how I found this bug...)

Reproducible: Always

Steps to Reproduce:
1. Open a single browser window.
2. Right-click on the toolbar, and click Customize.
3. In the customization dialog, make a change (I removed the home button, but the actual change is not important.  This happens with any sort of change here).
4. Without dismissing the Customize Toolbar dialog, close the main browser window (which should be the only browser window).  Firefox exits.
5. Restart the browser.  Note that all toolbars are emptied, except from the menu bar.  This includes the throbber, location bar, search bar, toolbar buttons, and bookmarks toolbar items, if visible.
6. Invoke the Customize Toolbar again.  The removed items are there: start adding them again to make your browser more useful.
Actual Results:  
*All* toolbar items are removed.

Expected Results:  
The closing of browser window should be dealt with gratefully, and the changes made in step 3 should get saved, which is the same as the behavior of the case where the Customize Toolbar dialog is closed without clicking Done.

about:buildconfig

Build platform
target
i686-pc-mingw32

Build tools
Compiler 	Version 	Compiler flags
cl 	14.00.50727 	-TC -nologo -W3 -Gy -Fd$(PDBFILE)
cl 	14.00.50727 	-GR- -TP -nologo -Zc:wchar_t- -W3 -Gy -Fd$(PDBFILE)

Configure arguments
--enable-application=browser --enable-update-channel=nightly --enable-optimize --disable-debug --disable-tests --enable-update-packaging

Comment 1

[(no longer active)]

This is a regression from Firefox 2, which handles this gently by discarding the customizations made to the toolbars.

Comment 2

[Ria Klaassen (not reading all bugmail)]

Regression window is http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1196416980&maxdate=1196446319
so probably caused by bug 348156.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Please add dependencies when suspecting a regression. That makes it less likely to get lost.

Comment 4

[(no longer active)]

BTW, it was tested on Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3pre) Gecko/2007121305 Minefield/3.0b3pre, which I mis-entered into the URL field when reporting.

Comment 5

[Mike Connor [:mconnor]]

Jonas, what's the status here?  Are you working on this regression?  Is there something specific the front end needs to do to deal with your changes?

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Most likely the fix here is purely frontend. I'm walking through the frontend code right now with venkman to see where things fail.

I did run in to an unrelated crash while trying to reproduce. I'll attach a patch for that one in case someone wants to help me out with the frontend code.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Fix crash

This is an unrelated problem, but it fixes a crash that prevents reproducing the bug (at least in debug builds).

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Fix frontend code

So here's what happens:

1. The user closes the window
2. We tear down all the XBL bindings as part of the teardown code
   (this has always happened)
3. The customize-toolbar window is closed
4. The customize-toolbar code tries to read the data on the customized toolbars
   to persist them. However, since the XBL bindings are gone the data read is
   gone and we just get undefined values and/or empty strings
5. The customize-toolbar code tries to persist these bogus values.

It is in step 5 where things start behaving differently with the nsDocument::Destroy patch. It used to be that after we had closed a window (and called nsDocument::Destroy) you couldn't persist anything. So the fact that we tried to persist the bogus values was ok since persisting didn't work anyway.

After the nsDocument::Destroy fix persisting works fine and so we end up persisting the bogus values resulting in empty toolbars.

I can fix this 2 ways.
A) Make persist not work after a window has been closed
B) Make the customize-toolbar code not attempt to persist after a window has been closed since it can't get to the correct values to persist anyway.

Fix A would probably get us mostly back to how things were before nsDocument::Destroy was fixed. However it would also needlessly cripple the platform. This bug seems to be the only one filed on nsDocument::Destroy breaking things, so maybe this is the only instance of where this badness happens.

The attached patch does B.

I'd be grateful for input from the toolkit/frontend folks about if A or B is what we want to do.

Comment 9

[Neil Deakin]

> Fix A would probably get us mostly back to how things were before
> nsDocument::Destroy was fixed. However it would also needlessly cripple the
> platform.

In what way? Are they cases where we want persist() to work on a document in a closed window?

The patch looks ok though for this bug.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

I agree that we should do B), I think that patch looks fine. I don't really have any opinion about A).

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #9)
> Are they cases where we want persist() to work on a document in a
> closed window?

I'd imagine so. For example it would be quite possible to make dismissing the browser window while customizing actually save the current toolbars (this patch effectively cancels the customization), if we weren't relying on XBL bindings being there. However that does require persist to work.

Comment 12

[[e-mail]]

Comment on attachment 312352 [details] [diff] [review]
Fix frontend code

>+  if (!gToolboxChanged || gToolboxDocument.window.closed)
Don't you mean defaultView?

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Not sure, when is one or the other preferable? .defaultView I guess has an official spec, but .window should work as well in reality, no?

Comment 14

[Neil Deakin]

(In reply to comment #13)
> Not sure, when is one or the other preferable? .defaultView I guess has an
> official spec, but .window should work as well in reality, no?
> 

Should have caught that. Documents don't have a window property, so defaultView would be preferred.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Fix frontend code v2

Sorry guys, not sure where I got the document.window idea from.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 312107 [details] [diff] [review]
Fix crash

Neil, you seemed to have some opinions here.

Note that we currently crash on a debug build if you close the window while customizing the toolbar without this change.

Comment 17

[Neil Deakin]

(In reply to comment #16)
> (From update of attachment 312107 [details] [diff] [review])
> Neil, you seemed to have some opinions here.
> 
> Note that we currently crash on a debug build if you close the window while
> customizing the toolbar without this change.
> 

Yes, the patch looks ok, except that the condition block around the observer removal should include a check for aIsFinal == PR_TRUE

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Fix crash v2

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Neil, I assumed you wanted a new patch? It wasn't really clear from your comment.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 313729 [details] [diff] [review]
Fix crash v2

Neal, did you mean to sr this patch too?

Comment 21

[Neil Deakin]

(In reply to comment #20)
> (From update of attachment 313729 [details] [diff] [review])
> Neal, did you mean to sr this patch too?

I'm not a superreviewer. Maybe you meant the other Neil?

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

All patches are checked in. Thanks for reviews!

Comment 23

[(no longer active)]

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008050718 Minefield/3.0pre

Verified.

Comment 24

[Marcia Knous [:marcia]]

https://litmus.mozilla.org/show_test.cgi?id=5396 added to Litmus FFT.