Last Comment Bug 406375 - In modal dialog (window.open with modal=yes) child windows and dialogs do not populate with expected content
: In modal dialog (window.open with modal=yes) child windows and dialogs do not...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XBL (show other bugs)
: Trunk
: All All
: P3 major with 9 votes (vote)
: ---
Assigned To: Olli Pettay [:smaug]
:
Mentors:
javascript:window.showModalDialog('...
: 578351 (view as bug list)
Depends on: 1224790
Blocks:
  Show dependency treegraph
 
Reported: 2007-12-01 16:56 PST by Jefferson
Modified: 2015-11-14 00:06 PST (History)
20 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
betaN+
-
.14-fixed
-
.17-fixed


Attachments
like this (2.46 KB, patch)
2010-12-03 11:08 PST, Olli Pettay [:smaug]
jst: feedback+
Details | Diff | Splinter Review
patch (2.62 KB, patch)
2010-12-08 04:24 PST, Olli Pettay [:smaug]
jst: review+
Details | Diff | Splinter Review
+test (4.53 KB, patch)
2010-12-09 07:30 PST, Olli Pettay [:smaug]
dveditz: approval1.9.2.14-
dveditz: approval1.9.1.17-
Details | Diff | Splinter Review

Description Jefferson 2007-12-01 16:56:54 PST
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.
Comment 1 Tyler Downer [:Tyler] 2010-07-09 09:39:16 PDT
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
Comment 2 Jefferson 2010-07-09 17:42:55 PDT
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.
Comment 3 Hong Tang 2010-10-07 20:08:34 PDT
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.
Comment 4 Mike Beltzner [:beltzner, not reading bugmail] 2010-10-08 06:02:34 PDT
*** Bug 578351 has been marked as a duplicate of this bug. ***
Comment 5 Mike Beltzner [:beltzner, not reading bugmail] 2010-10-08 06:21:10 PDT
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?
Comment 6 Boris Zbarsky [:bz] 2010-10-08 06:51:21 PDT
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.
Comment 7 Boris Zbarsky [:bz] 2010-10-08 06:59:28 PDT
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).
Comment 8 Jefferson 2010-10-08 10:43:28 PDT
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
Comment 9 Vladimir Vukicevic [:vlad] [:vladv] 2010-10-21 01:53:54 PDT
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?
Comment 10 Vladimir Vukicevic [:vlad] [:vladv] 2010-10-21 02:00:31 PDT
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?
Comment 11 Olli Pettay [:smaug] 2010-10-21 02:09:28 PDT
Dialog's thread is the main thread.
Comment 12 Vladimir Vukicevic [:vlad] [:vladv] 2010-10-21 02:11:59 PDT
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 :)
Comment 13 Olli Pettay [:smaug] 2010-10-21 02:21:08 PDT
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?
Comment 14 Vladimir Vukicevic [:vlad] [:vladv] 2010-10-21 02:31:13 PDT
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.
Comment 15 Boris Zbarsky [:bz] 2010-10-21 06:37:41 PDT
> the dialog itself is actually never initialized;

Yes, see comment 7.  Is the dialog binding even loaded in this case?
Comment 16 Hong Tang 2010-11-30 19:19:37 PST
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.
Comment 17 Johnny Stenback (:jst, jst@mozilla.com) 2010-12-02 14:28:28 PST
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.
Comment 18 Hong Tang 2010-12-03 01:47:21 PST
Thanks for te update. 
Understand. let's wait for 2.0 then.
Comment 19 Olli Pettay [:smaug] 2010-12-03 06:41:27 PST
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.
Comment 20 Hong Tang 2010-12-03 06:56:47 PST
oh, great. thanks !
Comment 21 Olli Pettay [:smaug] 2010-12-03 10:00:06 PST
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.
Comment 22 Olli Pettay [:smaug] 2010-12-03 11:08:03 PST
Created attachment 495057 [details] [diff] [review]
like this

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.
Comment 23 Johnny Stenback (:jst, jst@mozilla.com) 2010-12-06 15:40:55 PST
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.
Comment 24 Olli Pettay [:smaug] 2010-12-08 04:24:48 PST
Created attachment 496129 [details] [diff] [review]
patch

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?
Comment 25 Olli Pettay [:smaug] 2010-12-09 07:30:27 PST
Created attachment 496492 [details] [diff] [review]
+test
Comment 26 Olli Pettay [:smaug] 2010-12-09 11:29:42 PST
http://hg.mozilla.org/mozilla-central/rev/888943a30131
Comment 27 Olli Pettay [:smaug] 2010-12-13 14:47:21 PST
We may want to get this to 1.9.2 and 1.9.1
Comment 28 Olli Pettay [:smaug] 2010-12-13 15:34:21 PST
FYI, there are now 3.6 tryserver builds 
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/opettay@mozilla.com-610b926483df/
Comment 29 Pin Zhang [:pzhang] (inactive) 2010-12-14 15:48:22 PST
I have tested in CCB online banking system, it is oK!
Thanks!
Comment 30 Olli Pettay [:smaug] 2010-12-14 15:50:15 PST
Comment on attachment 496492 [details] [diff] [review]
+test

We should consider this to branches.
Comment 31 Hong Tang 2010-12-15 00:18:02 PST
Thanks all for the effort !!!
Comment 32 christian 2010-12-20 10:13:17 PST
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-.
Comment 33 Daniel Veditz [:dveditz] 2011-01-12 10:47:59 PST
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.
Comment 34 Olli Pettay [:smaug] 2011-01-12 11:00:44 PST
Argh, 
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/fd40740804fe
Comment 35 Olli Pettay [:smaug] 2011-01-12 11:02:35 PST
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/47b407e667cb

I totally forgot to update this bug.
Comment 36 Hong Tang 2011-02-21 01:01:40 PST
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
Comment 37 Olli Pettay [:smaug] 2011-02-21 01:51:10 PST
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/

Note You need to log in before you can comment on or make changes to this bug.