Jefferson Reporter
	Description • 17 years ago

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.11) Gecko/20071127 Firefox/2.0.0.11
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.11) Gecko/20071127 Firefox/2.0.0.11

In Firefox 2, user displays a modal window using window.open with modal feature. Child windows of the modal window do not behave as expected if generated from: (i) links that target a blank window (shows only about:blank) or (ii) links that cause a download (the Open/Save dialog lacks all file information, OK and Cancel buttons do not work).

Reproducible: Always

Steps to Reproduce:
1. Visit the above site and click Modal dialog. Click Allow to enable UniversalBrowserWrite (else you will not get a modal window.)
2. Click link to view in a new window. Close blank window.
3. Click link to download. Close dialog using the "X" button on the window frame.
4. Close Modal dialog and click Normal dialog in the original window.
5. Try the links again to confirm that they actually should work.
Actual Results:  
Link with target="_blank" displays about:blank and does not load the content.
Link that causes a download displays a largely blank Open/Save dialog. OK and Cancel buttons do not work.

Expected Results:  
Link with target="_blank" should display an image.
Link that causes a download should display a fully populated and functional Open/Save dialog.

This problem was raised in the forum thread "Big problem - download dialog is empty" http://forums.mozillazine.org/viewtopic.php?t=608423 .

Note: test case apparently causes problems in Minefield. Not suggested for testing in Minefield.

	Tyler Downer [He/Him]
	Comment 1 • 14 years ago

This bug was originally reported on Firefox 2.x or older, which is no longer supported and will not be receiving any more updates. I strongly suggest that you update to Firefox 3.6.6 or later, update your plugins (flash, adobe, etc.), and retest in a new profile. If you still see the issue with the updated Firefox, please post here. Otherwise, please close as RESOLVED > WORKSFORME
http://www.mozilla.com
http://support.mozilla.com/kb/Managing+profiles
http://support.mozilla.com/kb/Safe+mode

	Jefferson Reporter
	Comment 2 • 14 years ago

I made a screencast for this. Flash required. Buffering is a bit slow...
http://www.screencast.com/t/ZDEyM2FhYzU

During the process of testing a new profile, I noticed there is a slight difference in behavior depending on browser.link.open_newwindow.

(1) Download dialog - no difference

Actual Results:  Link that causes a download displays a largely blank Open/Save dialog. OK and Cancel buttons do not work.

Expected Results:  Link that causes a download should display a fully populated and functional Open/Save dialog.

(2) Display URI in a new window -

browser.link.open_newwindow = 3 (Default "New tab")
Actual Results:  Link with target="_blank" displays image in a new tab in the preview window.

browser.link.open_newwindow = 2 ("New window")
Actual Results:  Link with target="_blank" displays about:blank and does not load the content.

Expected Results:  Link with target="_blank" should display an image.

So this still is a problem.

	Hong Tang
	Comment 3 • 14 years ago

Currently we have encountered this problem during online banking support in China, 
China Construction Bank (CCB) has started working on supporting Firefox since last year. Now they are in their final testing stage.  Not sure how difficult to get this problem fixed for next 3.6 release.  But it is really critical for us. Thanks in advance.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 5 • 14 years ago

I think this is more a Core bug than a Firefox one, but not sure exactly where it belongs.

Is this due to a new policy in terms of what modal windows are allowed to do?

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 14 years ago

I don't think there's "new" policy here; this bug report dates back to 1.8.1.

Note that the mode of operation comment 0 describes is unsupported; I'd love a testcase for whatever issue comment 3 is about; preferably in a separate bug.

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 14 years ago

Ah, bug 578351 was that separate bug!  Complete with a testcase that wasn't copied here...

javascript:window.showModalDialog('http://www.google.com.br/search?q=filetype:xls');

for posterity.

Simpler to use testcase:

  javascript:window.showModalDialog('data:text/html,<a href="data:application/octet-stream,">Click me to hang your browser</a>');

The issue is almost certainly that the wrong event loops get spun somewhere (and possibly the creation of non-modal dialogs parented to modal ones is the underlying cause of that), leading to XBL not loading properly.  Let's try xbl for a start, though this may be necko, xpcom, or even widget.... (though the problem is present on both mac and Windows, which makes this last unlikely).

	Jefferson Reporter
	Comment 8 • 14 years ago

I don't understand the new test case, but am reluctant overwrite it if Boris finds it more useful.

I re-tested and updated my example (context for Comment 2) here: http://dev.jeffersonscher.com/bug/406375.html

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 9 • 14 years ago

On win32, here's where we're at (with bz's testcase):

USER32!NtUserWaitMessage
xul!nsAppShell::ProcessNextNativeEvent
xul!nsBaseAppShell::DoProcessNextNativeEvent
xul!nsBaseAppShell::OnProcessNextEvent
xul!nsThread::ProcessNextEvent
xul!NS_ProcessNextEvent_P
xul!nsXULWindow::ShowModal
xul!nsContentTreeOwner::ShowAsModal
xul!nsWindowWatcher::OpenWindowJSInternal
xul!nsWindowWatcher::OpenWindow
xul!NS_InvokeByIndex_P
xul!CallMethodHelper::Invoke
xul!CallMethodHelper::Call
xul!XPCWrappedNative::CallMethod
xul!XPC_WN_CallMethod
mozjs!js::Interpret
mozjs!js::RunScript
mozjs!js::Invoke
mozjs!js::ExternalInvoke
mozjs!js::ExternalInvoke
mozjs!JS_CallFunctionValue
xul!nsXPCWrappedJSClass::CallMethod
xul!nsXPCWrappedJS::CallMethod
xul!PrepareAndDispatch
xul!SharedStub
xul!nsTimerImpl::Fire
xul!nsTimerEvent::Run
xul!nsThread::ProcessNextEvent
xul!NS_ProcessNextEvent_P
xul!nsXULWindow::ShowModal
xul!nsContentTreeOwner::ShowAsModal
xul!nsWindowWatcher::OpenWindowJSInternal
xul!nsWindowWatcher::OpenWindow
xul!nsGlobalWindow::OpenInternal
xul!nsGlobalWindow::ShowModalDialog
xul!NS_InvokeByIndex_P
xul!CallMethodHelper::Invoke
xul!CallMethodHelper::Call
xul!XPCWrappedNative::CallMethod
xul!XPC_WN_CallMethod
mozjs!CallCompiler::generateNativeStub
mozjs!js::mjit::ic::NativeCall

So it looks like it's the right event loop -- it's the inner dialog's event loop (which is the download one).  But I don't understand why nothing actually closes that window; I think the issues are that events aren't being dispatched properly from xul events/clicks/etc?

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 10 • 14 years ago

My guess: in xpcom/build/nsThreadUtils.cpp -- in nsXULWindow::ShowModal, there's a while (mContinueModalLoop), where we call NS_ProcessNextEvent... but we call it on the main thread.  That's not the right thread, it should be whatever the current toplevel modal dialog's thread is, right?

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 11 • 14 years ago

Dialog's thread is the main thread.

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 12 • 14 years ago

Ignore that last bit (and I meant nsXULWindow.cpp); it's always going to spin the main thread.  But the issue here seems to be that nsXULWindow::Destroy is never being called when you hit Ok or Cancel, and that's the thing that calls ExitModalLoop.  If, on win32, I hit the close button on the download dialog, then the modal loop exits and I get back to the window.  That doesn't help on OSX, since you don't have a close button :)

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 13 • 14 years ago

Ah, that sounds a bit like the problem what sync XHR had in 
https://bugzilla.mozilla.org/show_bug.cgi?id=313646#c31
Some eventloop assumes that nothing inside it starts new eventloops?

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 14 • 14 years ago

We may be looking at the wrong problem here -- the dialog itself is actually never initialized; the onload handler isn't being called for the download dialog.  The dialog's buttons actually do execute expected code, but the dialog binding thinks that they're disabled (even though they're visually not), and so doesn't process them.  This is also why the dialog is empty.

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 14 years ago

> the dialog itself is actually never initialized;

Yes, see comment 7.  Is the dialog binding even loaded in this case?

	Hong Tang
	Comment 16 • 14 years ago

Hello
Any update on this problem ? 
The China Construction Bank are pareparing the release (fully support Firefox) in
February 2011, they are checking with us if we could get this problem fixed before that.
Thanks a lot for the support.

	Johnny Stenback (:jst)
	Comment 17 • 14 years ago

I think we should look into this for 2.0, especially since we've messed with dialog modality and we're more likely to have nested event loops going much more often now, and not knowing what's really going on here scares me in light of that.

	Hong Tang
	Comment 18 • 14 years ago

Thanks for te update. 
Understand. let's wait for 2.0 then.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 19 • 14 years ago

Hong, the decision to block mozilla2.0 doesn't mean that this couldn't be fixed in
firefox 3.x. I certainly try to figure out how to fix this also in mozilla1.9
(== firefox 3.x).
Blocking2.0+ means that this must be fixed in ff4.

	Hong Tang
	Comment 20 • 14 years ago

oh, great. thanks !

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 21 • 14 years ago

Ok, I think I found the reason.

When opening non-modal window in modal window, we open also the new window
as modal and run the event loop for that. But ofc code doesn't expect
non-modal window to work like modal.

So the fix, at least on linux seems to be to just mark the non-modal window
opened in modal window to be modal, but not start the event loop.

Needs to clean up the patch, verify that it works, and test other platforms.
I'm a bit worried that OSX might need something else.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 22 • 14 years ago

Jst, you've been hacking this code.
Can you see problems with this approach.
I tried to keep changes at minimum.

I pushed the patch to tryserver and will test on OSX and Windows tomorrow.
Will also write testcases.

	Johnny Stenback (:jst)
	Comment 23 • 14 years ago

Comment on attachment 495057 [details] [diff] [review]
like this

@@ -976,16 +981,28 @@ nsWindowWatcher::OpenWindowJSInternal(ns
   if (windowIsModal || windowIsModalContentDialog) {
     nsCOMPtr<nsIDocShellTreeOwner> newTreeOwner;
     newDocShellItem->GetTreeOwner(getter_AddRefs(newTreeOwner));
     nsCOMPtr<nsIWebBrowserChrome> newChrome(do_GetInterface(newTreeOwner));
 
     // Throw an exception here if no web browser chrome is available,
     // we need that to show a modal window.
     NS_ENSURE_TRUE(newChrome, NS_ERROR_NOT_AVAILABLE);
+    
+    if (!newWindowShouldBeModal && parentIsModal) {
+      nsCOMPtr<nsIBaseWindow> parentWindow(do_GetInterface(newTreeOwner));
+      if (parentWindow) {
+        nsCOMPtr<nsIWidget> parentWidget;
+        parentWindow->GetMainWidget(getter_AddRefs(parentWidget));
+        if (parentWidget) {
+          parentWidget->SetModal(PR_TRUE);
+        }
+      }
+      return NS_OK;

Looks ok to me, but I'd prefer not to have this early return here to avoid having future code that gets added at the end of this method get bypassed in this case.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 24 • 14 years ago

This one passes even tests.

Modal dialog handling related events are 
dispatched via nsAutoWindowStateHelper
(even though the opened window isn't truly modal, only modal in the widget 
level).

Because of backwards compatibility, I think this could be the safest option.
Comments?

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 26 • 14 years ago

http://hg.mozilla.org/mozilla-central/rev/888943a30131

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 27 • 14 years ago

We may want to get this to 1.9.2 and 1.9.1

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 28 • 14 years ago

FYI, there are now 3.6 tryserver builds 
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/opettay@mozilla.com-610b926483df/

	Pin Zhang [:pzhang] (inactive)
	Comment 29 • 14 years ago

I have tested in CCB online banking system, it is oK!
Thanks!

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 30 • 14 years ago

Comment on attachment 496492 [details] [diff] [review]
+test

We should consider this to branches.

	Hong Tang
	Comment 31 • 14 years ago

Thanks all for the effort !!!

	LegNeato
	Comment 32 • 14 years ago

Comment on attachment 496492 [details] [diff] [review]
+test

a=LegNeato for 1.9.2.14 and 1.9.1.17. We will not block on this though, so setting blocking-.

	Daniel Veditz [:dveditz]
	Comment 33 • 14 years ago

Comment on attachment 496492 [details] [diff] [review]
+test

Missed non-blocker code-freeze for 1.9.1.17 and 1.9.2.14 -- rescinding approval, you can try again next time.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 34 • 14 years ago

Argh, 
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/fd40740804fe

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 35 • 14 years ago

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/47b407e667cb

I totally forgot to update this bug.

	Hong Tang
	Comment 36 • 13 years ago

Hello Olli,
I would like to ask you a big favor at this critical moment: to make sure that this fix will be included in release 3.6.14.

The next CCB bank's major feature release day will be Mar.05 (they have only two major release a year, the next one will be in Aug.),  they have already packed their fix for Firefox support and passed their testing procedure,  but now they are holding the release unless they see our fix appears in the Firefox released version 3.6.14

Please Please help us to push it out with 3.6.14.

CCB bank Firefox support will be a big Milestone in China Internet history. We have been pushing them very hard for last two years. We really do not want to be missed at this moment.

Thank you very much and appreciate your support !

Hong

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 37 • 13 years ago

The patch did land to 1.9.1 and 1.9.2 branches, so whenever the next
3.5.x/3.6.x security update is released, that update should contain also
fix for this.

You could try http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/3.6.14-candidates/build3/