User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040207 Firefox/0.8 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040207 Firefox/0.8 Situation : - "Tools"->"Options"->" "Block Popup Windows" is NOT enabled - the HTML page viewed contains a link to download a file (.zip, .exe, ...) : * EITHER : window.open("filename.zip"); * OR : <A HREF="filname.zip" TARGET="name_of_a_new_window"> download file </A> Result : - A large (fullscreen ?) blank window shows up and don't close automatically. - A dialog box shows up to ask the user to open or save the file - After the download begins, progress & ends, the large window's still here and the user have to close it manually by clicking the close box at the right top corner NOTE !!!! This bug does NOT appear when using : <A HREF="filname.zip" TARGET="name_of_an_existing_window"> download file </A> OR when not using 'target' <A HREF="filname.zip"> download file </A> Reproducible: Always Steps to Reproduce: 1.standard firefox 0.8 setup on Windows or Linux 2.popup blocking DISABLED 3.buggy URL : http://telecharger.01net.com/windows/Programmation/creation/fiches/1288.shtml Actual Results: see details Expected Results: not to show a large (fullscreen ?) blank window or (at least) close it automatically

Well sure, this link <A HREF="filname.zip" TARGET="name_of_a_new_window"> download file </A> opens a new window before the download begins. Popup blocker settings will have no effect on this. The URL you gave contains no such links; it doesn't even contain a reference to a .zip file. It does try to open popup windows when the page is first loaded. In short, I don't get it. Are you simply saying that the new window opened by the above link is full-screen? I would also expect that if you were browsing with a full-screen browser window. Can you attach a simplified test case, or give instructions on what to do with the sample URL? Would you clarify what's different if the popup blocker is enabled?

As requested by email : how to reproduce it ? 1) Start Firefox 0.8, say under Windows 2K 2) Disable popup blocking (restart the browser to be sure popup aren't blocked ???) 3) Paste the following URL into the URL bar and hit ENTER : http://telecharger.01net.com/windows/Programmation/creation/fiches/1288.shtml RESULT : - a "small" (normal) popup should show up - a big blank window should show up - a download should start (a shareware, nervermind you can cancel it afterward) - even when the download finish, the big blank window stay opened This does not happen with IE6, but happen also with NS 7.1 and Mozilla 1.6 under Win2K. This could be _really annoying_ for end users... Michel

Oh. The complaint is simply this: upon executing this script window.open("xxx.exe") (assuming xxx.exe exists) Mozilla/Firefox opens a new blank browser window and leaves it open. IE also opens a new blank browser window, but closes it as soon as the download dialog opens. (The telecharger site mentioned above happens to execute a window.open of an .exe in its load handler.) Adjusting summary.

Dup of bug 102380, unless the fix would be different for Seamonkey and Firefox.

(In reply to comment #4) > Dup of bug 102380, unless the fix would be different for Seamonkey and Firefox. now there is a bug for Firefox, see bug 251140

*** Bug 251140 has been marked as a duplicate of this bug. ***

If clicking a link to a file download opens a new window/tab, we should close that new window/tab upon starting the download, assuming that the new window/tab has no actual content. There are three cases (detailed here, but there are probably more) where this occurs: a) Left-clicking a link to a file where the target=_blank . b) [Middle|Ctrl]-clicking a link to a file (new tab opens in addition to file download). c) Shift-clicking a link to a file (new window opens in addition to file download). In all these cases, Firefox should detect that the window/tab has no content, and close it upon starting the download. We should obviously not do this if the spawned window has content which is *sometimes* the case. This bug exhibits itself particularly on webmail sites (based on user reports). IE seems to handle this well (in many, but not all cases), and closes the spurious content-less window.

*** Bug 265842 has been marked as a duplicate of this bug. ***

(In reply to comment #8) > *** Bug 265842 has been marked as a duplicate of this bug. *** So, has anyone fixed this yet?

(In reply to comment #9) > So, has anyone fixed this yet? Please stop asking for status updates on bugs. Bugzilla is not an appropriate forum for this. Please read my response to your queries here: https://bugzilla.mozilla.org/show_bug.cgi?id=265842#c8

(In reply to comment #10) > (In reply to comment #9) > > So, has anyone fixed this yet? > Please stop asking for status updates on bugs. Bugzilla is not an appropriate > forum for this. Please read my response to your queries here: > https://bugzilla.mozilla.org/show_bug.cgi?id=265842#c8 And again...GET OFF MY ASS!! Find someone else to harass! You "tore me a knew one" on your other thread, so also take a look here! https://bugzilla.mozilla.org/show_bug.cgi?id=265842#c9 Only TWO go*damn times did I ask about any updates!!! If one cannot ask about "updates" to bugs here, then WHAT IS THE PURPOSE OF IT?? Only to **REPORT** and that's IT?? If it is expressly forbidden in the rules to inquire about updates, then EXCUSE ME!!! IT WON'T HAPPEN AGAIN! And again, FIND SOMEONE ELSE TO FOLLOW AROUND! Thank you.

(In reply to comment #11) > Only TWO go*damn times did I ask about any updates!!! If one cannot ask > about "updates" to bugs here, then WHAT IS THE PURPOSE OF IT?? Only to > **REPORT** and that's IT?? If it is expressly forbidden in the rules to > inquire about updates, then EXCUSE ME!!! IT WON'T HAPPEN AGAIN! And again, > FIND SOMEONE ELSE TO FOLLOW AROUND! > Thank you. I am "following you around" because you are filing bugs on components that I QA. Believe me, I get no joy out of following people around on Bugzilla. And yes, once you file a bug on Bugzilla, you do one of these things: a) Add additional information. b) Respond to questions by developers. c) Provide status updates of your own if the bug is fixed. The purpose of commenting is not to ask if a bug has been fixed or not. That is not proper Bugzilla ettiquete. It strikes me that if a bug is still open, and no patch has been written or checked in, it should be fairly obvious that the bug has not been fixed. There are ways and means of finding out the status of bugs. Learn them. I've always followed the mantra that when you join a new community, you take the time to learn its ettiquete before ripping on its members.

(In reply to comment #12) >There are ways and means of finding out the status of bugs. >Learn them. I've always followed the mantra that when you join a new community, >you take the time to learn its ettiquete before ripping on its members. Dude, the one doing the "ripping" was YOU upon ME. You're the one that "tore into me" FIRST. If there are "ways and means of finding out the status of bugs", then wouldn't it have been more helpful for you to mention them instead what you did? Please let me know what these are so I can look into them. Thanks.

Clint, just look at the top of the page. There you can read: "Status: NEW". But I would also be happy to see this bug beeing fixed. This shouldn't be very hard to do. Just give windows and tabs opened by a link or javascript a note, that is delted upon page rendering. When opening the dl-dialog check for that note, and if it exists, perform the same action as ctrl+w does (it closes the tab, and if it was the only tab in the window, closes the window). I would be very happy, if someone could do this, for I am not able to write C code.

(In reply to comment #14) > Clint, just look at the top of the page. There you can read: "Status: NEW". > But I would also be happy to see this bug beeing fixed. > This shouldn't be very hard to do. > Just give windows and tabs opened by a link or javascript a note, that is delted > upon page rendering. When opening the dl-dialog check for that note, and if it > exists, perform the same action as ctrl+w does (it closes the tab, and if it was > the only tab in the window, closes the window). > I would be very happy, if someone could do this, for I am not able to write C code. BTW, FWIW, incredibly this is still happening on RC1. I haven't tried RC2 yet. -Clint

*** Bug 268942 has been marked as a duplicate of this bug. ***

*** Bug 268481 has been marked as a duplicate of this bug. ***

Just to add weight to the 1.1 blocking request, I've had over 380,000 downloads of my 'Disable Targets For Downloads' extension from UMO and countless more from my homepage, and my extension only partially solves the problem, and in a somewhat hackish way. Also please note: this behaviour is also triggered by external applications (Thunderbird, Office, etc). If Firefox is not open, it would be semi-confusing to the end-user when they find that blank Firefox windows are open when they only wanted to download a binary file.

*** Bug 289670 has been marked as a duplicate of this bug. ***

in the interest of doing less stupid things in Deer Park, I'm going to poke this before branch.

Bug 264746 often crops up alongside this one, for example when opening an attachment from Gmail.

*** Bug 285319 has been marked as a duplicate of this bug. ***

*** Bug 300971 has been marked as a duplicate of this bug. ***

mconnor, are you reasonably going to be able to get to this before beta? is there someone else we might want to poke to help? /cb

pushing this out. Fixing this only for the new window case isn't worth the effort, and making it work for tabs is more work and risk than I want us to take on at this stage. I intend to fix this in 1.9 alpha, once 1.5 is largely wrapped up.

"accept bug" on behalf of marria@gmail.com. from my bugmail, this was the intended result of bug changes.

Marria, are you still looking to fix this? If you don't have cycles right now, please reassign to nobody@mozilla.org so we know to find resources.

Thanks for the reminder on this Mike. I'm still intending to fix this. I'll shoot for making progress on this in the next couple of weeks, but if that doesn't happen I'll reassign to nobody.

(In reply to comment #18) > Just to add weight to the 1.1 blocking request, I've had over 380,000 downloads > of my 'Disable Targets For Downloads' extension from UMO and countless more from > my homepage, Version 1.0.1 of this extension is still working OK for me in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.4) Gecko/20060601 Firefox/1.5.0.4 ID:2006060104. Isn't there a way to just get this code added to the core of the browser and be done with it? Can you post the code here?

This is a first try at closing the window after the external app handler has done its work. This seemed to work for my test cases. One thing I'm not sure about is the refresh handler case - I could use some help coming up with a test case that triggers that code path since simply setting a Refresh header didn't (mOriginalChannel seems to always be null in ProcessAnyRefreshTags).

I don't like testing for the URL "about:blank" as a way of determining whether a tab is empty. We do that all over the code, but here it would be especially bad, because I think it woould introduce a security hole that would allow web pages to close the tab they're in, by sending you to about:blank and then sending you to a download URL. (See bug 190515 for discussion of whether web pages should be allowed to close the tab they're in; currently they're not.)

(In reply to comment #31) > I don't like testing for the URL "about:blank" as a way of determining whether > a tab is empty. We do that all over the code, but here it would be especially > bad, because I think it woould introduce a security hole that would allow web > pages to close the tab they're in, by sending you to about:blank and then > sending you to a download URL. (See bug 190515 for discussion of whether web > pages should be allowed to close the tab they're in; currently they're not.) > Not to get off topic, but technically, I can't think of a functional reason why a website should even be able to link to "about:blank" in the first place, outside of some sort of completest philosophy in the browser code. So, remove the ability for a website to actually send you to or link you to "about:blank". This would NOT, mind you, remove your ability to type "about:blank" into the address bar yourself, or have an external program or internet shortcut call it. Then there would be no problem with closing about:blank tabs that have initiated after a window.open sequence, because these would only occur and initiate locally, not called from the active page. The only problem that might arise from this that I can foresee is some sort of wacky extension that uses an html link to call an about:blank tab, but I doubt there is something like that, especially one that has a window.open procedure directly preceding it.

Web pages have been allowed to link to about:blank for years, and it's useful for various reasons. For example, it's useful for putting stuff into an iframe or a new window with DOM2 without getting a server involved and without using data: URLs, which aren't supported in IE.

(In reply to comment #33) > Web pages have been allowed to link to about:blank for years, and it's useful > for various reasons. For example, it's useful for putting stuff into an iframe > or a new window with DOM2 without getting a server involved and without using > data: URLs, which aren't supported in IE. > Thanks Jesse, and I apologize for my assumption. There has got to be, however, some way of doing this without running into the about:blank security hole that you describe. I went to your comment on the other bug, describing how porn sites could use the dialog to condition the user into clicking onto a later, more serious dialog box. Can you provide a description of how this bug getting fixed could be potentially abused? An about:blank page cannot execute any code on close, correct? So whether or not it is manually closed or it is automatically closed by Firefox, no "on close" code could be executed in this particular case, right?

I imagine an onclose event could be added to an about:blank page using the DOM, but I don't understand why you're asking. The problem isn't a page's onclose code running; the problem is web pages being able to close the tab they're in.

(In reply to comment #35) > I imagine an onclose event could be added to an about:blank page using the DOM, > but I don't understand why you're asking. The problem isn't a page's onclose > code running; the problem is web pages being able to close the tab they're in. > I was missing the point as to how a website being allowed to close its own window or tab could be malicious. That is why I tried to rationalize what you were saying by coming up with the idea that the only way it could be problematic would be if it could run some type of code on close. However, contrary to that, what you are saying is that it is, by itself, a security problem for a website to be able to close the tab or window it is contained in. I guess it WOULD technically be a loss of data, because there is nothing forcing the website to properly label the link that the user clicks on as being a close button. However, this bug is referring to a case where a NEW window or tab is created, not when about:blank is opened in a current tab and a download is called. So, as long as this bug only fixes the case where a NEW window or tab is opened, then no website could use this to close itself. We would be searching for when a NEW window or tab is opened, a download is called, and about:blank remains, not when about:config is opened in the current window and a download is called. A third situation would be where if a website opens another page with window.open, and then that page redirects to a download and then redirects to about:blank. That window shouldn't be closed either by fixing this bug, because, for one thing, a downloadable file was not called by the window.open command, it was caused after a redirect. That would also not be the same situation, nor do I see a practical application as to why a website might do that. And, even then, the original page could not use this bug being fixed to close itself. Let me know if I am still not grasping what you are saying! Thanks

That would be one way to avoid introducing a security hole while fixing this bug. I don't think the patch is that careful, though.

(In reply to comment #31) > I don't like testing for the URL "about:blank" as a way of determining whether > a tab is empty. We do that all over the code, but here it would be especially > bad, because I think it woould introduce a security hole that would allow web > pages to close the tab they're in, by sending you to about:blank and then > sending you to a download URL. (See bug 190515 for discussion of whether web > pages should be allowed to close the tab they're in; currently they're not.) > Thanks for the feedback Jesse, you are right that I don't check whether the download is in a new window or not. I didn't know about this security issue. I was trying to think of what the best way is to fix this. One possibility would be to add state to the docshell when the window is opened to indicate that it was opened specifically for a download. This would require changing an interface though - it would be even better if we could tell from the existing state that we have. One idea that Christian had was to check whether there are any entries in session history. Can you think of any issues with that? I like this approach because we wouldn't have to clutter up any interfaces this way.

Marria, it seems like you should be able to check some state on the DocShell object to determine whether or not it corresponds to a newly created DocShell that has not yet had any content (other than about:blank) loaded in it. nsIDocShell is an internal interface implemented by DocShell that we can extend for this purpose if it helps to do so.

I also thought of that. It may be easier to avoid that for the 1.8 branch though. On the other hand that may be more reliable. I want to note that if it comes to extending nsIDocShell then another possibility would be to make docshell handle the window closing rather than exthandler, that may be the simpler solution and would allow not exposing this docshell state on an interface.

(In reply to comment #39) > Marria, it seems like you should be able to check some state on the DocShell > object to determine whether or not it corresponds to a newly created DocShell > that has not yet had any content (other than about:blank) loaded in it. > nsIDocShell is an internal interface implemented by DocShell that we can extend > for this purpose if it helps to do so. > Do you guys have a strong preference for the DocShell approach? I favor the session history approach a little bit because it allows us to reuse existing state and avoid changing interfaces. Let me know if you guys think that there are disadvantages with the session history approach that makes it unreliable. Thanks, Marria

(In reply to comment #41) > (In reply to comment #39) > > Marria, it seems like you should be able to check some state on the DocShell > > object to determine whether or not it corresponds to a newly created DocShell > > that has not yet had any content (other than about:blank) loaded in it. > > nsIDocShell is an internal interface implemented by DocShell that we can extend > > for this purpose if it helps to do so. > > > > Do you guys have a strong preference for the DocShell approach? I favor the > session history approach a little bit because it allows us to reuse existing > state and avoid changing interfaces. Let me know if you guys think that there > are disadvantages with the session history approach that makes it unreliable. I posted a patch with the session history approach, let me know what you think

It's possible for a gecko-based browser to be configured with no session history, either via preferences or via build-time configuration. That might matter, especially to someone embedding gecko on a small device. Another thing to consider: If you go the session history route, then it is probably better to get the nsISHistory object from the nsIWebNavigation. The DocShell object implements nsIWebNavigation, which I believe is accessible from the mWindowContext object.

Well, the question is: Is there any data loss if there is no session history in the window? It's certainly possible to make a window that had content loaded have no shistory entries... consider http://biesi.damowmow.com/win_no_shistory.html, you could then link to a download page.

There's no dataloss if there is no session history, but it's still weird and might make it easier to exploit certain security holes. (For example, a user might assume that if he only sees one Firefox window in the taskbar, its chrome is "real" and its address bar cannot be spoofed. If a web page is able to open a new window and then clobber the original, his assumption fails.)

Comment on attachment 225651 [details] [diff] [review] check that session history is empty Marria and I discussed this today. She's got a much better solution on the way...

Comment on attachment 225651 [details] [diff] [review] check that session history is empty ok then, r- per comment 46 and comment 47

I added something to the DocShell which tracks if it ever loaded any content that is not about:blank. Now this is used from the ExternalHelperAppService to determine whether it is safe to close the window. I also fixed my logic for checking whether a refresh header was present - before I was only checking for a handler, which was incorrect. (As a side note, currently the refresh handling in external helper app service is busted, I filed bug 341806 for that).

so I just thought about an edge case: - site does window.open(), about:blank is loaded there - site uses DOM methods to put content into the new window (doesn't load a new URL) - That content contains a download URL If I click that download URL, would, with this patch, the window/tab be closed?

(In reply to comment #50) > so I just thought about an edge case: > > - site does window.open(), about:blank is loaded there > - site uses DOM methods to put content into the new window (doesn't load a new > URL) > - That content contains a download URL > > If I click that download URL, would, with this patch, the window/tab be closed? > I think you are right Christian, my patch would probably cause the window to be closed, since I don't think EndPageLoad would be called in the case where you use the DOM to add content to the window. The particular case you pointed out doesn't seem like a security problem though since the site that opens a window is allowed to close it right? But I guess maybe one site could open the window and use DOM to add content to the window. Then a different site could add the download url. Was that the case you are worried about?

Actually I was not thinking of that as a security issue. I was thinking of the window having some content the user may want to read. But, if the user clicks the download link, the window is gone, so they may be annoyed :) I have no idea if that's a common thing for a site to do...

(In reply to comment #52) > Actually I was not thinking of that as a security issue. I was thinking of the > window having some content the user may want to read. But, if the user clicks > the download link, the window is gone, so they may be annoyed :) > > I have no idea if that's a common thing for a site to do... > Oh ok, I see. So the first question is whether this is common enough to worry about. Do other people have opinions on this? If so, then what are our options to fix it? I guess we could listen for DOM mutations and update the docShell state if we hear a mutation? That seems pretty heavyweight though. Another option might be to add a different piece of state that detects whether the window was specifically opened for the download. I'm not sure how hard it would be to find a place to grab this piece of state, and we'd also have to be sure to reset it if the window isn't closed. Thoughts?

This is another idea I had - we can pass state on the channel which indicates whether a new window was opened specifically for the download. However, unfortunately this patch only works for the target= case, not the window.open case. Thoughts on how to expand it to the window.open case? In a way I like the approach of the other patch better because both cases can be handled in the same manner.

(In reply to comment #50) > so I just thought about an edge case: > > - site does window.open(), about:blank is loaded there > - site uses DOM methods to put content into the new window (doesn't load a new > URL) > - That content contains a download URL > > If I click that download URL, would, with this patch, the window/tab be closed? > Hey Christian, I'm having trouble coming up with a test case for this, could you help? I tried using document.write to put content in the window, but that actually triggers a load, so it works fine with my patch. I also tried updating document.body.innerHTML but I can't seem to get that to work when trying to update the document in the newly opened window from the opener. thanks, Marria

This is a second iteration on the patch that stores state on the channel indicating whether the window was opened specifically for that load. This version works for target= as well as window.open

Comment on attachment 226606 [details] [diff] [review] store state in channel, works for target= as well as window.open >Index: docshell/base/nsDocShell.cpp >+ props->SetPropertyAsBool(NS_LITERAL_STRING("new-window-target"), I think it would be a good idea to preface this property name with the string "docshell." to avoid colliding with other names. We do the same for the "docshell.internalReferrer" property. >Index: docshell/base/nsIDocShell.idl >+ // This flag marks the first load in a new window. Currently it is >+ // used in the external helper app service to determine whether it is >+ // safe to close a window. >+ const long INTERNAL_LOAD_FLAGS_NEW_WINDOW = 0x8; While it is useful to know where this flag is used, this kind of comment can easily get out of sync with reality. Since LXR can tell us where the flag is used, it might be good to limit this comment to discussing the functionality of the flag alone. >Index: docshell/base/nsIWebNavigation.idl > /** >+ * This flag specifies that this load is the first load in a new window. >+ * Currently it is used to determine whether it is safe to close the >+ * window in the external helper app service, after downloading the >+ * content. >+ */ >+ const unsigned long LOAD_FLAGS_NEW_WINDOW = 0x4000; Ditto. >Index: embedding/components/windowwatcher/src/nsWindowWatcher.cpp ... >+ windowIsNew ? nsIWebNavigation::LOAD_FLAGS_NEW_WINDOW : >+ nsIWebNavigation::LOAD_FLAGS_NONE, PR_TRUE); Here's an example of the flag being used outside of exthandler ;-) >Index: uriloader/exthandler/Makefile.in > windowwatcher \ > embed_base \ >+ dom \ > $(NULL) Indentation nit. This Makefile appears to use tabs. In the wonderful world of Mozilla, Makefile.in files tend to use tabs whereas everything else uses spaces in place of tabs :-/ >Index: uriloader/exthandler/nsExternalHelperAppService.cpp > NS_IMETHODIMP nsExternalAppHandler::OnStopRequest(nsIRequest *request, nsISupports *aCtxt, > nsresult aStatus) > { >+ nsCOMPtr<nsIPropertyBag2> props = do_QueryInterface(mRequest); >+ NS_ENSURE_STATE(props); >+ props->GetPropertyAsBool(NS_LITERAL_STRING("new-window-target"), >+ &mShouldCloseWindow); Hmm... not every nsIRequest that comes through here will support nsIPropertyBag2. I don't think you want to error out if the QI fails. >Index: uriloader/exthandler/nsExternalHelperAppService.h > /** >+ * This flag is set if a refresh header was found. In this case, we >+ * don't want to close the dom window after handling the content. >+ */ >+ PRPackedBool mHasRefreshHeader; >+ >+ /** >+ * This is set based on whether the channel indicates that a new window >+ * was opened specifically for this download. If so, then we >+ * close it. >+ */ >+ PRBool mShouldCloseWindow; s/PRBool/PRPackedBool/ >Index: webshell/public/nsIRefreshURI.idl >+/** >+ * Hack Alert: We really want setupRefreshURI to return true if it found >+ * a refresh header on the channel. However, we can't modify these >+ * interfaces on the branch. So, instead we create a new success code >+ * that is used to indicate that a refresh header was found and successfully >+ * setup. >+ */ >+%{C++ >+#define NS_REFRESHURI_HEADER_FOUND \ >+ NS_ERROR_GENERATE_SUCCESS(NS_ERROR_MODULE_URILOADER, \ >+ 2) This change is going into the trunk as well as the branch, right? If so, then I think the trunk comment should not refer to an unnamed branch. Maybe it would be best to just make this be the API even though it is not as pretty as a PRBool return value :-/ sr=darin w/ suggested revisions

> >+ /** > >+ * This is set based on whether the channel indicates that a new window > >+ * was opened specifically for this download. If so, then we > >+ * close it. > >+ */ > >+ PRBool mShouldCloseWindow; > > s/PRBool/PRPackedBool/ I actually need to use PRBool here because the property bag function requires this type. Otherwise, I made all the changes you suggested. I also moved the line that looks for docshell.newWindowTarget into onStartRequest, instead of onStopRequest, since I noticed that onStopRequest happens after the MaybeCloseWindow call, in some circumstances where you can cancel the request.

Comment on attachment 227564 [details] [diff] [review] fixes in response to Darin's review >Index: docshell/base/nsDocShell.cpp > if (!refreshHeader.IsEmpty()) { > SetupReferrerFromChannel(aChannel); > rv = SetupRefreshURIFromHeader(mCurrentURI, refreshHeader); >+ if (NS_SUCCEEDED(rv)) { >+ return NS_REFRESHURI_HEADER_FOUND; >+ } > } > } whoops... 4 space indentation >+ if (aIsNewWindowTarget) { >+ nsCOMPtr<nsIWritablePropertyBag2> props = do_QueryInterface(channel); >+ if (props) { >+ props->SetPropertyAsBool(NS_LITERAL_STRING("docshell.newWindowTarget"), >+ PR_TRUE); >+ } >+ } ditto sr=darin

Comment on attachment 227564 [details] [diff] [review] fixes in response to Darin's review So this window is only closed when the user chooses an action, not as soon as the helper dialog appears? docshell/base/nsDocShell.cpp // Transfer the load to the target DocShell... Pass nsnull as the // window target name from to prevent recursive retargeting! // if (NS_SUCCEEDED(rv) && targetDocShell) { + aFlags |= INTERNAL_LOAD_FLAGS_NEW_WINDOW; Hm... isn't this case also hit if the load gets targeted to an existing window? webshell/public/nsIRefreshURI.idl \ No newline at end of file please add one :)

(In reply to comment #60) > (From update of attachment 227564 [details] [diff] [review] [edit]) > So this window is only closed when the user chooses an action, not as soon as > the helper dialog appears? Correct. I figured that one is no better than the other. Also, I believe the refresh header detection happens later as the code is now, so it's easier to make the window closing code later too. > docshell/base/nsDocShell.cpp > // Transfer the load to the target DocShell... Pass nsnull as the > // window target name from to prevent recursive retargeting! > // > if (NS_SUCCEEDED(rv) && targetDocShell) { > + aFlags |= INTERNAL_LOAD_FLAGS_NEW_WINDOW; > > Hm... isn't this case also hit if the load gets targeted to an existing window? > Yea, I think you are right, moved this up to where we detect that a new window needs to be opened. > > webshell/public/nsIRefreshURI.idl > \ No newline at end of file > > please add one :) > added :-) (In reply to comment #59) > (From update of attachment 227564 [details] [diff] [review] [edit]) > >Index: docshell/base/nsDocShell.cpp > > > if (!refreshHeader.IsEmpty()) { > > SetupReferrerFromChannel(aChannel); > > rv = SetupRefreshURIFromHeader(mCurrentURI, refreshHeader); > >+ if (NS_SUCCEEDED(rv)) { > >+ return NS_REFRESHURI_HEADER_FOUND; > >+ } > > } > > } > > whoops... 4 space indentation > > > >+ if (aIsNewWindowTarget) { > >+ nsCOMPtr<nsIWritablePropertyBag2> props = do_QueryInterface(channel); > >+ if (props) { > >+ props->SetPropertyAsBool(NS_LITERAL_STRING("docshell.newWindowTarget"), > >+ PR_TRUE); > >+ } > >+ } > > ditto done and done.

Comment on attachment 227973 [details] [diff] [review] tweaks in response to reviews Actually, I'm going to go ahead and check this in on the trunk, since these were just minor changes. If there are any follow up comments, I can post another patch to address.

Marking fixed, since Bonsai seems to say that it's checked in on trunk. Also marking qawanted, since we'd like to make sure that we're not causing any regressions here.

Comment on attachment 227973 [details] [diff] [review] tweaks in response to reviews We need something like this to bake for a few days before we can consider taking it. Please give it a thorough workout on trunk and assuming no regressions, renominate in a couple of days.

Comment on attachment 227973 [details] [diff] [review] tweaks in response to reviews actually I missed one thing in my review (sorry about that): uriloader/exthandler/nsExternalHelperAppService.h + PRBool mShouldCloseWindow; This should be a PRPackedBool like all the booleans around it

(In reply to comment #65) > (From update of attachment 227973 [details] [diff] [review] [edit]) > actually I missed one thing in my review (sorry about that): > > uriloader/exthandler/nsExternalHelperAppService.h > + PRBool mShouldCloseWindow; > > This should be a PRPackedBool like all the booleans around it > oh, actually I specifically made this PRBool, since the property bag function I'm using requires that type. Unless there is another function I don't know about?

oh... hrm... you could do it with an intermediate local variable...

(In reply to comment #67) > oh... hrm... you could do it with an intermediate local variable... > ok, if you think it is that important to make it consistent with the other bools, i can do that, it's a trivial change.

Comment on attachment 228317 [details] [diff] [review] PRBool -> PRPackedBool thanks, this is better. another option would've been to move it after the other PackedBools, but I prefer this :)

Comment on attachment 227973 [details] [diff] [review] tweaks in response to reviews This has been on the trunk for a couple days now with no reported regressions. I know you wanted to let it sit a little longer, but I'm renominating this now because otherwise it will definitely miss beta1. I think this patch would benefit from more exposure sooner, to catch any unexpected interactions with websites. I think the risk is relatively low because it's a pretty small patch and not a code path that gets hit often.

Comment on attachment 228317 [details] [diff] [review] PRBool -> PRPackedBool This is a small tweak that goes with the other patch, but it was just checked in today, so it hasn't been sitting for as long on the trunk.

What is the intended behaviour with this patch? If I follow the steps listed below I still get the empty window upon a canceled download: 1) Start Firefox 0.8, say under Windows 2K 2) Disable popup blocking (restart the browser to be sure pop-up aren't blocked ???) 3) Paste the following URL into the URL bar and hit ENTER : http://telecharger.01net.com/windows/Programmation/creation/fiches/1288.shtml 4) Click on "Retour à la fiche du logiciel" (Under the "Wise for Windows Installer") 5) Click the blue "Télécharger" button to initiate the download Upon clicking on the blue telecharger button the user is prompted to save the file but there is also a small 1x1" FF window that is launched along with the prompt for download. If you cancel the download and close the ff downloads dialog the 1x1" ff window remains and has to be closed manually. BUILD: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1a3) Gecko/20060706 BonEcho/2.0a3 ID:2006070615 ~B

(In reply to comment #73) > What is the intended behaviour with this patch? If I follow the steps listed > below I still get the empty window upon a canceled download: > > 1) Start Firefox 0.8, say under Windows 2K > 2) Disable popup blocking (restart the browser to be sure pop-up aren't > blocked ???) > 3) Paste the following URL into the URL bar and hit ENTER : > http://telecharger.01net.com/windows/Programmation/creation/fiches/1288.shtml > 4) Click on "Retour � la fiche du logiciel" (Under the "Wise for Windows > Installer") > 5) Click the blue "T�l�charger" button to initiate the download > > Upon clicking on the blue telecharger button the user is prompted to save the > file but there is also a small 1x1" FF window that is launched along with the > prompt for download. If you cancel the download and close the ff downloads > dialog the 1x1" ff window remains and has to be closed manually. > > BUILD: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1a3) > Gecko/20060706 BonEcho/2.0a3 ID:2006070615 > > ~B Hi Bryan, The intended behavior is that the small blank window should close when you cancel the download. I tried these repro steps with my own branch build, as well as the most recent tinderbox build (taken from here http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/pacifica-vm-mozilla1.8/, Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1a3) Gecko/20060706 BonEcho/2.0a3 ID: 2006070619 ). Can you try reproing with that build? thanks, Marria

Currently the window only disappears after canceling the download or completely download the file, if I'm correct. In the case of big downloads then the blank window will be there until it finishes, thus still show the blank windows for a great extend of time. Wouldn't it be better to close the blank window/tab as soon as the download dialog is displayed? (like other browsers seem to do).

(In reply to comment #75) > Currently the window only disappears after canceling the download or completely > download the file, if I'm correct. > > In the case of big downloads then the blank window will be there until it > finishes, thus still show the blank windows for a great extend of time. > > Wouldn't it be better to close the blank window/tab as soon as the download > dialog is displayed? (like other browsers seem to do). > Most of the testing I did was with small downloads so I didn't notice how annoying this can be. I agree, it should at least be moved up to the point where the user selects an action.

oh... when reviewing the patch, I didn't realize that ExecuteDesiredAction is only called at the end of the download. I agree that this isn't good.

I opened Bug 343921 for this. I'm looking into it.

(In reply to comment #75) > Currently the window only disappears after canceling the download or completely > download the file, if I'm correct. This was not my experience following the five steps I noted in my comment in this bug. Every time I canceled the download and closed the downloads dialog the little 1x1" ff window remained. It did however close automatically ONCE the download completely finished! ~B

(In reply to comment #75) > Currently the window only disappears after canceling the download or completely > download the file, if I'm correct. I just went over my steps again and in all but one case the small ff window was closed properly. I do agree that a better UE would be to close the window as soon as the user is presented with the download dialog. ~B

See also bug 346561, same thing but for windows/tabs created by shift/ctrl/middle clicking links rather than target="_blank" or window.open().

*** Bug 354696 has been marked as a duplicate of this bug. ***

(In reply to comment #29) > (In reply to comment #18) > > Just to add weight to the 1.1 blocking request, I've had over 380,000 downloads > > of my 'Disable Targets For Downloads' extension from UMO and countless more from > > my homepage, > > Version 1.0.1 of this extension is still working OK for me in Mozilla/5.0 > (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.4) Gecko/20060601 Firefox/1.5.0.4 > ID:2006060104. Isn't there a way to just get this code added to the core of > the browser and be done with it? Can you post the code here? > I see this is still broken in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20061008 BonEcho/2.0 ID:2006100803 I went to the "Disable Targets for Downloads" extension home page looking for an update, but it says it is no longer necessary with this version of Firefox. Try going to http://forums.mozillazine.org/viewtopic.php?t=448305 and click on the Download Manager Tweak extension and it will leave you with a blank tab. Requesting reopening this bug.

extension installs are not downloads. file a new bug.

Seeing this more and more recently in latest release and nightly (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.4pre) Gecko/20070410 BonEcho/2.0.0.4pre ID:2007041003), especially with .pdf files.