465993 - [FIX]window.open() gives zero size window with no toolbars

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[John J. Barton]

When I run 
firefox.exe -chrome chrome://chromebug/content/chromebug.xul -firefox
with addons
http://getfirebug.com/releases/firebug/1.4X/firebug-1.4X.0a5.xpi
http://getfirebug.com/releases/chromebug/chromebug-0.4.0a5.xpi

FF3.0.4 opens Firefox normally
FF3.1b2pre I get a zero size firefox window with no toolbars. 

The chromebug code in question is:

var ff = window.open(opener.firefoxURL);

Comment 1

[John J. Barton]

Attachment: test extension opens window via setTimeout call

This test extension illustrates the problem of window.open(). Currently the open is called on setTimeout, calls via onLoad event also fail. Calls from the xul overlay FF > Tools > TestNow work correctly (full size window).

Comment 2

[John J. Barton]

This problem block Chromebug which is my tool for developing Firebug 1.4 for FF3.1

Comment 3

[John J. Barton]

More tidbits: if I open windows using 
firefox.exe -chrome <chromeurl> then I get a zero sized window mostly. 

In opening chromebug I use:
firefox.exe -chrome chrome://chromebug/content/chromebug.xul -firefox
chromebug comes up full size but when it opens firefox, firefox is zero size.

Comment 4

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

This sounds like it might be a duplicate of bug 458898.

Comment 5

[Johnny Stenback (:jst)]

Looks like bug 458898 was just fixed. If this is still a problem, please re-nominate.

Comment 6

[John J. Barton]

I just pulled mozilla-1.9.1 after roc said the fix was in on that tree. After build I have the same tiny window as before.

Comment 7

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

Well, now this might be a duplicate of bug 469203 :)

Comment 8

[John J. Barton]

Well, I'd say that 469203 duplicates this earlier bug report ;-)

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: screenshot

That test extension works for me in my trunk WinXP build. See this screenshot.

Comment 10

[John J. Barton]

Well when I entered the bug 1.9.1 was trunk. My test in comment 6 was my app which still fails. I'll test the extension against 1.9.1.

Comment 11

[John J. Barton]

Ok the test case does open a window now using 1.9.1 (The window is unusable because the sizing controls out drawn off screen).

But my application still fails.

If I run the test extension with :
-p bugzilla-465993 -jsconsole -chrome chrome://test/content/test.xul
the window comes up, but I also get a mystery window off screen, similar to what get in my app.

Comment 12

[John J. Barton]

Ok I don't agree that the test case passes. Yes, the window comes up, that's progress. But it its not firefox its just a window. As you can see from your screenshot the toolbars and stuff are off the display surface. After moving the window where I can see it, no toolbars, etc.

Comment 13

[John J. Barton]

I don't know if this helps, but now that the window comes up I can see it in chromebug. Its not like any thing I've see before, the content is just the site html (not browser.xul). I guess that matches the missing browser controls.

Comment 14

[Boris Zbarsky [:bzbarsky]]

So what we have here is chrome code doing a window.open() on the chrome window, right?  And passing in a URI?

It looks like we get into nsWindowWatcher::OpenWindowJSInternal, and when we go to call CreateChromeWindow2 we have an invisible parent.  This causes us to pass in null as the parent.  Null parent means that the URI passed in is to be loaded as the chrome in the window instead of loading the standard chrome (or at least that's what nsAppStartup::CreateChromeWindow2 does with it).

So we get the observed behavior.  This looks like a regression from bug 112294, which added the visibility check.

There's the side issue of onload here firing before the window is visible; not sure whether that's correct.  But I definitely don't think the visibility check in question should be affecting chrome callers.  I also think that falling back on opening the target URI as the chrome of the child is not the right way to handle an invisible parent.

Comment 15

[Christopher Blizzard (:blizzard)]

So what's the resolution here?

Comment 16

[Boris Zbarsky [:bzbarsky]]

There isn't one yet.  We need to fix this bug, imo, and then depending on whether the resulting behavior differs from firefox 3.0 we might need to document how it differs.

Comment 17

[Johnny Stenback (:jst)]

Yeah, sounds like we need to fix this. Boris, can you work on this one? Sounds like you know more about what's needed here than anyone else. If not, let me know...

Comment 18

[Boris Zbarsky [:bzbarsky]]

Well...  I can't figure out what the fix in bug 112294 was trying to do, exactly, and why it thought that passing a null parent was a good idea.  My gut instinct would be to back the window watcher part of that patch out to fix this, and let that bug be fixed in a different way, whatever is appropriate there.  I'd love to hear what ere, roc, and smaug have to say on the matter.

Comment 19

[Ere Maijala (slow)]

The idea in bug 112294 was to prevent creating a child dialog for an invisible window, because the dialog would be invisible too (at least on Windows). In the test case there was an alert fired in onblur. Without the check this would seemingly hang Firefox and prevent a clean shutdown when the parent window was closed. If null parent is inappropriate, another way to do it is needed.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Does the child window actually need to get opened?  Does that need to happen if it's a content load?

My gut instinct is that at least content loads parented to an unvisible window should either throw or return null.  Not sure about chrome loads, but for consistency that should be the same.  The only question is whether web script can run into this situation, and if so what the behavior should be.  For example, what if instead of an alert that script had done a window.open()?

Comment 21

[Boris Zbarsky [:bzbarsky]]

OK, per bug 112294 the issue that was being fixed with the parent check is specific to modal dialogs being opened "during shutdown" (after the last window was closed on Windows; presumably the shutdown part is the key part there, though).

So sounds to me like we should at least restrict the check to cases when:

  1)  We're in shutdown
  2)  We're opening a modal dialog

at the very least.  And in those cases, if the chrome flag is set we should just use a null parent or whatever; if it's not set we should just throw.

Alternately, we could fix the Windows appshell logic so we don't have to hack around its bugs here, no?  That seems like a better solution to me, honestly.

Comment 22

[Ere Maijala (slow)]

I'm all for fixing appshell (this kind of obscure stuff will otherwise come up later again). But if that's too big, we can of course restrict the check for now. I'm not quite sure how to check if we're in shutdown, though.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Can we check whether the window enumerator is returning zero windows or some such?

Comment 24

[John J. Barton]

How about setting a shutdown flag in "quit-application-requested" or one of her friends?

Comment 25

[Boris Zbarsky [:bzbarsky]]

OK, I talked to Benjamin and the shutdown case is hard, especially because if I read things right the open can happen way before shutdown.

So new proposal is:

1)  System code can just do whatever it wants
2)  Content code can open non-chrome windows whenever it wants.
3)  Content code trying to parent a chrome window (e.g. an alert) to an
    invisible window will throw.

Ere, what do you think of that approach?  Or would this have issues with window.openDialog?

Comment 26

[Boris Zbarsky [:bzbarsky]]

Er, I meant window.showModalDialog.

Comment 27

[Ere Maijala (slow)]

(In reply to comment #24)
> How about setting a shutdown flag in "quit-application-requested" or one of her
> friends?

quit-application-requested is posted when quit is requested, but doesn't mean we don't have windows or are actually going to exit. quit-application-granted is only sent when shutting down and we won't be there yet when the dialog is displayed.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

This should "fix" the testcase in this bug (which means make it try to open an infinite number of windows), while not regressing the onblur alert issue ere was trying to fix.

Not sure how to write tests for this stuff, offhand.  :(

Comment 29

[Boris Zbarsky [:bzbarsky]]

Oh, so what this does is that any attempt to open a dependent child of a content window which is in invisible chrome will throw.

Comment 30

[Johnny Stenback (:jst)]

Comment on attachment 356223 [details] [diff] [review]
Proposed fix

One would think, but it's not clear from quickly looking over the code that it is :(

Comment 31

[Johnny Stenback (:jst)]

Um, that comment was in reply to your XXXbz in the patch, which somehow got lost there.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/8dae102faa15

Comment 33

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c074a1996d70

Comment 34

[Frank Wein [:mcsmurf]]

Somehow this patch broke printing mails in SeaMonkey/Thunderbird, see Bug 473962.