343921 - blank window left after download closes too late

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Marria Nazif]

The fix for bug 241972 closes the blank window only after the download is finished.  This is annoying for long downloads.  We should move the window closing earlier.  Ideally we should close it when the dialog first pops up, but at least we should close it immediately after the user chooses an action in the dialog.

Comment 1

[Marria Nazif]

Attachment: close the window earlier, immediately retargeting load notifications

This closes the window right before the dialog pops up.

One concern with this approach is that I'm not sure if it would break other embeddors that implement nsIHelperAppLauncherDialog.  Specifically, I'm now passing null in as the window context if I intend to close the window, so that no dialogs are made dependent on that window.  I think it's important to send this message to other embeddors anyway, though.  Otherwise if they make a dialog depend on the window, when the external helper app service closes the window, the dependant dialog will also be closed.

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

cc'ing some embeddors. but the window context can be used for more than just dialog parenting... maybe that's the main use for it in this function though.

Comment 3

[Christian Persch (GNOME) (away; not receiving bug mail)]

Without a parent nsIDOMWindow, Epiphany (or any gtk embedder) cannot position the content handler dialogue on the right window, or even on the right monitor. So the content handler dialogue could show up under some other window, or on another monitor where the user cannot see it, and the user will wonder why nothing happened, and click the link again and again...

Is there no way to get the nsIDOMWindow of the document that opened the temporary window that spawns the content handler dialogue? IMHO that would be the right window to use in this case.

Comment 4

[Marria Nazif]

Comment on attachment 228744 [details] [diff] [review]
close the window earlier, immediately retargeting load notifications

I discovered that this doesn't work in the case where the user has: 1. decided to save to disk always for a certain download type and 2. wants to be prompted for where to save the file.  In this case the file picker needs a parent window and this patch doesn't work because there is no other dialog to be the parent in that case.  I'll try to look into another approach for this sometime this weekend.

Comment 5

[Mike Schroepfer]

Since this is a nice improvement from bug 241972 we'd probably take a patch if it was available - but will not block the release on it.

Comment 6

[Marria Nazif]

Attachment: replace the window context with the opener

Since it seems that removing the window context entirely causes problems, this patch replaces the window context with the window opener before closing the window.

Comment 7

[Marria Nazif]

I believe that it's important to fix this bug for this release.  I discovered some undesirable behavior with the current window closing behavior which this would fix.  Currently, if a download is very long the window will only be closed at the end of the download.  However, in the meantime the user may have used the blank window and navigated to a new page, and then we would close a window which they are currently using.

Comment 8

[Marria Nazif]

fyi I'm on vacation from August 4-August 13, but I can probably be accessible to respond to review comments through this weekend.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 231730 [details] [diff] [review]
replace the window context with the opener

+    if (opener != nsnull) {

 if (opener) {

matches the style here better :)

+      nsCOMPtr<nsIInterfaceRequestor> new_context(do_GetInterface(opener));
+      mWindowContext = new_context;

why not:
  mWindowContext = do_GetInterface(opener);
?

Comment 10

[Darin Fisher]

Comment on attachment 231730 [details] [diff] [review]
replace the window context with the opener

>Index: uriloader/exthandler/nsExternalHelperAppService.cpp

>+  // Check to see if there is a refresh header on the original channel.
>+  if (mOriginalChannel) {
>+    nsresult rv;
>+    nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(mOriginalChannel,
>+                                                          &rv));
>+    if (NS_SUCCEEDED(rv)) {
>+      nsCAutoString refreshHeader;
>+      rv = httpChannel->GetResponseHeader(NS_LITERAL_CSTRING("refresh"),
>+                                          refreshHeader);
>+      if (!refreshHeader.IsEmpty()) {
>+        mShouldCloseWindow = PR_FALSE;
>+      }
>+    }
>+  }

It looks like this code doesn't really need to capture |rv|.

You could just null check |httpChannel| and check if |refreshHeader| is empty.


sr=darin with that nit fixed

Comment 11

[Marria Nazif]

Attachment: address review comments, add timeout

Thanks guys, this addresses your review comments.  Also, I noticed that a couple of random exceptions were being thrown when the window was closed, apparently because the window was being closed before it had fully loaded.  Adding a timeout for the close call fixed this.  Could you take another look?

Comment 12

[Darin Fisher]

Comment on attachment 232217 [details] [diff] [review]
address review comments, add timeout

>Index: uriloader/exthandler/nsExternalHelperAppService.cpp

>+      // Now close the old window.  Do it on a timer so that we don't run
>+      // into issues trying to close the window before it has fully opened.
>+      nsresult rv;
>+      mTimer = do_CreateInstance("@mozilla.org/timer;1", &rv);
>+      if(NS_FAILED(rv)) {
>+        return NS_ERROR_FAILURE;
>+      }
>+
>+      mTimer->InitWithCallback(this, 0, nsITimer::TYPE_ONE_SHOT);
>+      mWindowToClose = internalWindow;

Since you aren't doing anything with the |rv| returned by
do_CreateInstance, you can again avoid capturing it and just
null-test mTimer.

A 0-interval, one-shot timer is just a thread event.  Maybe you
should use NS_DispatchToCurrentThread and implement nsIRunnable
instead.  One downside is that it is not so easy to implement a
thread event on the 1.8 branch, so if this patch is destined for
FF2, then you probably want to stick with the timer approach
even though it is a bit-heavyweight.

Other questions:

1- how do we know that something hasn't happened to mWindowToClose
between the time this timer is created and the time Notify is
called?  how do we ensure that we aren't destroying some window
that shouldn't be destroyed?

2- is it possible for the timer to be created twice before Notify
is called?  it seems like you should at least assert that mTimer
is null before creating a new one.

3- maybe related to the above questions: is there anything that
could happen between the time the timer is created and the time
Notify runs that could cause you to want to prevent the timer
from firing?


>+NS_IMETHODIMP
>+nsExternalAppHandler::Notify(nsITimer* timer) {
>+  if (!mWindowToClose) {
>+    return NS_ERROR_UNEXPECTED;

returning an error code from Notify doesn't mean much to the caller.
maybe this code should output a warning (NS_WARNING) so that we'll
see this unexpected condition in debug builds should it even appear.

maybe you should also clear mTimer when Notify is called.

Comment 13

[Marria Nazif]

(In reply to comment #12)
> (From update of attachment 232217 [details] [diff] [review] [edit])
> >Index: uriloader/exthandler/nsExternalHelperAppService.cpp
> 
> >+      // Now close the old window.  Do it on a timer so that we don't run
> >+      // into issues trying to close the window before it has fully opened.
> >+      nsresult rv;
> >+      mTimer = do_CreateInstance("@mozilla.org/timer;1", &rv);
> >+      if(NS_FAILED(rv)) {
> >+        return NS_ERROR_FAILURE;
> >+      }
> >+
> >+      mTimer->InitWithCallback(this, 0, nsITimer::TYPE_ONE_SHOT);
> >+      mWindowToClose = internalWindow;
> 
> Since you aren't doing anything with the |rv| returned by
> do_CreateInstance, you can again avoid capturing it and just
> null-test mTimer.

ok, will make this change

> A 0-interval, one-shot timer is just a thread event.  Maybe you
> should use NS_DispatchToCurrentThread and implement nsIRunnable
> instead.  One downside is that it is not so easy to implement a
> thread event on the 1.8 branch, so if this patch is destined for
> FF2, then you probably want to stick with the timer approach
> even though it is a bit-heavyweight.

This is destined for FF2, so I guess we'll have to stick with the timer approach for now.

> Other questions:
> 
> 1- how do we know that something hasn't happened to mWindowToClose
> between the time this timer is created and the time Notify is
> called?  how do we ensure that we aren't destroying some window
> that shouldn't be destroyed?

Hmm, I'm not sure what could happen to make it a window that shouldn't be destroyed.  By the time we set up the timer we have already changed the window context and the window isn't being used for anything.  Really the only reason why I added the timer is because of some exceptions that seem to be related to closing the window before it is initialized (i.e. a removeObserver call that fails, presumably because the observer was not added yet).

> 2- is it possible for the timer to be created twice before Notify
> is called?  it seems like you should at least assert that mTimer
> is null before creating a new one.

Good idea, I'll add that.

> 3- maybe related to the above questions: is there anything that
> could happen between the time the timer is created and the time
> Notify runs that could cause you to want to prevent the timer
> from firing?

hmm, I can't think of anything - after the point when we set up the timer, the window shouldn't actually be in use for anything at all.

> 
> >+NS_IMETHODIMP
> >+nsExternalAppHandler::Notify(nsITimer* timer) {
> >+  if (!mWindowToClose) {
> >+    return NS_ERROR_UNEXPECTED;
> 
> returning an error code from Notify doesn't mean much to the caller.
> maybe this code should output a warning (NS_WARNING) so that we'll
> see this unexpected condition in debug builds should it even appear.
> 
> maybe you should also clear mTimer when Notify is called.
> 

ok, will make these changes.

Comment 14

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

(In reply to comment #13)
> Hmm, I'm not sure what could happen to make it a window that shouldn't be
> destroyed.  By the time we set up the timer we have already changed the window
> context and the window isn't being used for anything.  Really the only reason
> why I added the timer is because of some exceptions that seem to be related to
> closing the window before it is initialized (i.e. a removeObserver call that
> fails, presumably because the observer was not added yet).

That sounds like bug 310955. If you're going to keep the timer code for this, perhaps a followup bug should be filed about removing it, dependent on bug 310955?

Comment 15

[Marria Nazif]

(In reply to comment #14)
> (In reply to comment #13)
> 
> That sounds like bug 310955. If you're going to keep the timer code for this,
> perhaps a followup bug should be filed about removing it, dependent on bug
> 310955?
> 

Ah, yea, you're right, this is exactly what I'm seeing.  I can definitely file a bug to remove the timer code, dependent on that bug.  However, I'm almost wondering if it's better to check it in without the timer code, since those exceptions are already happening and are a known problem.  Perhaps that is better than cluttering up this code with a timer?  What do you guys think?

Comment 16

[Marria Nazif]

Attachment: address Darin's review comments

Comment 17

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

(In reply to comment #15)
> Ah, yea, you're right, this is exactly what I'm seeing.  I can definitely file
> a bug to remove the timer code, dependent on that bug.  However, I'm almost
> wondering if it's better to check it in without the timer code, since those
> exceptions are already happening and are a known problem.  Perhaps that is
> better than cluttering up this code with a timer?  What do you guys think?

I was originally going to suggest that, but then realized that those exceptions prevent a lot of the browser window's standard shutdown code not to run, so it's probably not a good idea to introduce code that will let that happen. The other testcase for that bug (window.open() immediately followed by a close()) is fairly unlikely to happen in normal browsing, and isn't triggered by any of the browser code itself, so it's less of an issue.

Comment 18

[Marria Nazif]

(In reply to comment #17)

Ok that makes sense, let's just stick with the patch I just posted then.  I'll file a followup bug when I get to the point of checking it in.

Comment 19

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 232472 [details] [diff] [review]
address Darin's review comments

   // Only close the page if there is no refresh header and if the window
   // was opened specifically for the download
-  if (!mHasRefreshHeader && mShouldCloseWindow) {
-    internalWindow->Close();
+  if (mShouldCloseWindow) {

hm, I realize now that this comment doesn't make so much sense now that the refresh header isn't checked here anymore. 

+NS_IMETHODIMP
+nsExternalAppHandler::Notify(nsITimer* timer) {

the style of this file puts the { on its own line

r=biesi with that fixed

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 232472 [details] [diff] [review]
address Darin's review comments

a=dbaron on behalf of drivers.  Please land on MOZILLA_1_8_BRANCH and add the fixed1.8.1 keyword once you have done so.

Comment 21

[Brian Ryner (not reading)]

checked in on trunk and branch