509124 - Chrome flags are never set on unparented XUL windows

Status: VERIFIED FIXED

(Core :: General, defect)

Description

[Aakash Desai [:aakashd]]

Attachment: testFormManager/testClearFormHistory.js

using the attached test case, the modal dialog API hangs and does not respond to the instancing of the clear recent history dialog via the tools menu.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

That CRH dialog is weird! I tried to get a fix for that but I have no idea where to start. Here some observations:

When I let go output from within the getDialog function go to the console I never see an entry for this window. I have three windows open main browser window, Mozmill window, and the Error Console. That means I get the following BrowserChrome flags: 4094, 2147487422, and 2147485198. There is no flag listed for the opened Clear Recent History window.

Comparing the opener to other instances of modal dialogs this one uses null for its parent window. But the modal attribute is set. So could it be that when null is specified the window is not listed by getXULWindowEnumerator("")?

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/sanitize.js#445

As result we hang in our getDialog function:
http://hg.mozilla.org/qa/mozmill-tests/file/ac1da292c3b9/shared-modules/testModalDialogAPI.js#l141

Boris, do you know why this window is not listed by the enumerator?

Comment 2

[Boris Zbarsky [:bzbarsky]]

I have no idea.  Neil might know...  In general, is the issue just that it's app-modal?

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

Ok, I have flipped null against aParentWindow in sanitize.js and everything works fine. The CRH dialog is now listed when retrieving an enumerator with getXULWindowEnumerator. I know nothing about how the window list is build but by using no parent window seems to create an independent window outside of the window list which will not be detected when the construction of the list happens.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

See also bug 309406 (which added the ifdef for Mac) and bug 464930 (CRH shouldn't be a modal dialog).

Comment 5

[[e-mail]]

I don't know about the Mac (which is the only OS that uses a null parent for
the CPH dialog). I can't reproduce the problem on Windows.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 393277 [details]
testFormManager/testClearFormHistory.js

Forgive my ignorance ... but how do you run this test?

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

Steven, just clone the mozmill-test repository (http://hg.mozilla.org/qa/mozmill-tests/) and save the test e.g. to firefox/helperClasses. Open Mozmill and run the test. You have to put it into the correct folder due to shared modules dependencies.

The same situation can be observed on Windows when changing the first parameter of the window opener to null. It would be helpful to have a better testcase here which doesn't rely on Mozmill. I'll try to create one.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

Attachment: simple window checker

This XUL file checks all the open windows and displays their chromeFlags.

While writing this code I noticed that I was wrong. The window enumerator returns the correct window instances but opening the CRH with null as parent window seems to ignore the modal flag given to the openWindow function. The flag is not added to the windows chromeFlags but let the window be opened modal. That means we cannot evaluate if this window is modal or not.

Comment 9

[[e-mail]]

Can you call nsIWebBrowserChrome::IsWindowModal perhaps?
(QI to nsIInterfaceRequestor and getInterface an nsIWebBrowserChrome)

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

Thanks Neil! In my testings on Friday I forgot the nsiInterfaceRequestor part so that function wasn't working for me. Now we get the correct modal state of the CRH window. I will incorporate the patch for Mozmill in bug 492136.

Does it mean the behavior of the underlying component is ok and will not be changed?

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

As Neil mentioned on IRC chrome flags should only be used for content windows (pop-ups). So is there anything we can do here or should this bug be closed?

Comment 12

[[e-mail]]

When opening a window from another window (either modal or modeless) then the flags calculated by the window watcher are explicitly set on the newly created chrome window. However if the window is opened without a parent then no chrome flags are set, the XUL window's chromeFlags property has a default value and the window always opens with all of its chrome. Usually nobody cares since the only parentless windows are either the startup browser (which always has all of its chrome) or a random XUL window which doesn't care about chrome flags.

Comment 13

[[e-mail]]

Attachment: Proposed patch

The simpler patch would have been to add a call to SetChromeFlags to the one caller of CreateTopLevelWindow that doesn't do it itself, but since we're passing the chrome flags around anyway I thought I'd centralise it.

Comment 14

[[e-mail]]

Pushed changeset 5798118100e1 to mozilla-central.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

Verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20090821 Minefield/3.7a1pre ID:20090821030931

Neil, do you think it's worth to have this patch in 1.9.2?

Comment 16

[[e-mail]]

Mozmill is the only consumer that I know of, so it's up to you, basically.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

Comment on attachment 393487 [details] [diff] [review]
Proposed patch

At the moment our Mozmill tests don't use the flags anymore but it could happen that we have to base on them. So it would be fantastic to get it into 1.9.2.

Comment 18

[Dave Townsend [:mossop]]

I suspect that this caused bug 514920

Comment 19

[Benjamin Smedberg]

Comment on attachment 393487 [details] [diff] [review]
Proposed patch

Caused a regression, can't approve without a complete rollup.