Closed Bug 465993 Opened 11 years ago Closed 11 years ago

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

Categories

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

1.9.1 Branch
x86
All
defect

Tracking

()

RESOLVED FIXED
mozilla1.9.1

People

(Reporter: johnjbarton, Assigned: bzbarsky)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [firebug-p1])

Attachments

(3 files)

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);
Flags: wanted-firefox3.1?
Depends on: 465998
Blocks: 453978
No longer depends on: 465998
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).
This problem block Chromebug which is my tool for developing Firebug 1.4 for FF3.1
Whiteboard: [firebug p1]
OS: Linux → All
Flags: blocking-firefox3.1?
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.
OS: All → Linux
This sounds like it might be a duplicate of bug 458898.
Component: General → DOM
Flags: wanted-firefox3.1?
Flags: blocking-firefox3.1?
Product: Firefox → Core
QA Contact: general → general
Flags: blocking1.9.1?
Keywords: testcase
OS: Linux → All
Looks like bug 458898 was just fixed. If this is still a problem, please re-nominate.
Flags: blocking1.9.1?
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.
Flags: blocking1.9.1?
Well, now this might be a duplicate of bug 469203 :)
Well, I'd say that 469203 duplicates this earlier bug report ;-)
Attached image screenshot
That test extension works for me in my trunk WinXP build. See this screenshot.
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.
Version: Trunk → 1.9.1 Branch
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.
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.
Priority: -- → P1
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.
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.
Blocks: 112294
So what's the resolution here?
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.
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...
Assignee: nobody → bzbarsky
Flags: blocking1.9.1? → blocking1.9.1+
Priority: P1 → P2
Target Milestone: --- → mozilla1.9.1
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.
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.
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()?
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.
Whiteboard: [firebug p1] → [firebug-p1]
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.
Can we check whether the window enumerator is returning zero windows or some such?
How about setting a shutdown flag in "quit-application-requested" or one of her friends?
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?
Er, I meant window.showModalDialog.
(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.
Attached patch Proposed fixSplinter Review
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.  :(
Attachment #356223 - Flags: superreview?(jst)
Attachment #356223 - Flags: review?(jst)
Attachment #356223 - Flags: review?(emaijala)
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.
Summary: window.open() gives zero size window with no toolbars → [FIX]window.open() gives zero size window with no toolbars
Attachment #356223 - Flags: review?(emaijala) → review+
Blocks: 472886
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 :(
Attachment #356223 - Flags: superreview?(jst)
Attachment #356223 - Flags: superreview+
Attachment #356223 - Flags: review?(jst)
Attachment #356223 - Flags: review+
Um, that comment was in reply to your XXXbz in the patch, which somehow got lost there.
Pushed http://hg.mozilla.org/mozilla-central/rev/8dae102faa15
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Depends on: 473962
Somehow this patch broke printing mails in SeaMonkey/Thunderbird, see Bug 473962.
Depends on: 553936
Depends on: 681636
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.