Chrome flags are never set on unparented XUL windows

VERIFIED FIXED in mozilla1.9.3a1

Status

()

Core
General
--
major
VERIFIED FIXED
8 years ago
2 years ago

People

(Reporter: aakashd, Assigned: neil@parkwaycc.co.uk)

Tracking

1.9.1 Branch
mozilla1.9.3a1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MozMill1.2Testday])

Attachments

(3 attachments)

(Reporter)

Description

8 years ago
Created attachment 393277 [details]
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.
(Reporter)

Updated

8 years ago
Whiteboard: [MozMill1.2Testday]
(Reporter)

Updated

8 years ago
Blocks: 509125
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?
Summary: Modal Dialog hangs on traversing the menu API to get to Clear Recent History → Modal dialog "Clear Recent History" is not listed by wm.getXULWindowEnumerator
I have no idea.  Neil might know...  In general, is the issue just that it's app-modal?
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.
Component: Mozmill → General
Product: Testing → Core
QA Contact: mozmill → general
Version: unspecified → 1.9.1 Branch
See also bug 309406 (which added the ifdef for Mac) and bug 464930 (CRH shouldn't be a modal dialog).
(Assignee)

Comment 5

8 years ago
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.
OS: All → Mac OS X
Comment on attachment 393277 [details]
testFormManager/testClearFormHistory.js

Forgive my ignorance ... but how do you run this test?
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.
Keywords: testcase-wanted
OS: Mac OS X → All
Created attachment 393397 [details]
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.
(Assignee)

Comment 9

8 years ago
Can you call nsIWebBrowserChrome::IsWindowModal perhaps?
(QI to nsIInterfaceRequestor and getInterface an nsIWebBrowserChrome)
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?
No longer blocks: 509125
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?
Summary: Modal dialog "Clear Recent History" is not listed by wm.getXULWindowEnumerator → Chrome flags for modal dialogs without a parent are ignored
(Assignee)

Comment 12

8 years ago
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.
Summary: Chrome flags for modal dialogs without a parent are ignored → Chrome flags are never set on unparented XUL windows
(Assignee)

Comment 13

8 years ago
Created attachment 393487 [details] [diff] [review]
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.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #393487 - Flags: review?(jst)

Updated

8 years ago
Attachment #393487 - Flags: review?(jst) → review+
(Assignee)

Comment 14

8 years ago
Pushed changeset 5798118100e1 to mozilla-central.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
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?
Status: RESOLVED → VERIFIED
Target Milestone: --- → mozilla1.9.3a1
(Assignee)

Comment 16

8 years ago
Mozmill is the only consumer that I know of, so it's up to you, basically.
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.
Attachment #393487 - Flags: approval1.9.2?
I suspect that this caused bug 514920
Depends on: 514920
Comment on attachment 393487 [details] [diff] [review]
Proposed patch

Caused a regression, can't approve without a complete rollup.
Attachment #393487 - Flags: approval1.9.2? → approval1.9.2-
(Assignee)

Updated

6 years ago
Depends on: 651659
Keywords: testcase-wanted
You need to log in before you can comment on or make changes to this bug.