Closed Bug 344808 Opened 15 years ago Closed 14 years ago

Canceling a [download whose link tries to open a new window] causes current tab to close, when browser.link.open_newwindow is 1

Categories

(Core Graveyard :: File Handling, defect)

1.8 Branch
defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.8.1beta2

People

(Reporter: tommyjb, Assigned: marria)

References

Details

(Keywords: dataloss, fixed1.8.1, regression, Whiteboard: [has patch][needs sr darin])

Attachments

(2 files, 5 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060714 BonEcho/2.0b1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060714 BonEcho/2.0b1

1. Set browser.link.open_newwindow to 1 (forces links that try to open new windows to be opened in the current tab).
2. Open the attached HTML file.
3. Left-click the link on it.
4. When the download prompt comes up, click CANCEL.

This causes the current tab to close, closing the entire browser if there's no other tab open.

When browser.link.open_newwindow is set to its default value, however, a new tab is opened first and canceling the download closes *that* tab.

Reproducible: Always
Summary: Following a link that tries to open a download in a new window Cancelling a [download whose link tries to open a new window] causes current tab to close, when browser.link.open_newwindow is 1 → Cancelling a [download whose link tries to open a new window] causes current tab to close, when browser.link.open_newwindow is 1
Attached file test page
Summary: Cancelling a [download whose link tries to open a new window] causes current tab to close, when browser.link.open_newwindow is 1 → Canceling a [download whose link tries to open a new window] causes current tab to close, when browser.link.open_newwindow is 1
Problem also present with:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060714 Minefield/3.0a1
This might be a side-effect of bug 241972. 
*** Bug 345325 has been marked as a duplicate of this bug. ***
Indeed a regression caused by bug 241972.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-firefox2?
Keywords: dataloss, regression
Hardware: PC → All
Version: unspecified → 2.0 Branch
Fwiw, browser.link.open_newwindow=1 has other known bugs, which is why UI for it was removed. I don't think an issue with an "unsupported" hidden pref is going to block the release at this stage.
Blocks: 241972
Wouldn't it be sufficient to make sure that the window's history's length is 0 before actually closing it in MaybeCloseWindow:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp&rev=1.290.4.7&mark=2567#2559
That was discussed in bug 241972 - perhaps it would be a good idea to add that as an additional check.
Assignee: nobody → file-handling
Component: Tabbed Browser → File Handling
Flags: blocking-firefox2?
Product: Firefox → Core
QA Contact: tabbed.browser → ian
Version: 2.0 Branch → 1.8 Branch
Attached patch possible fix (untested) (obsolete) — Splinter Review
Something along the lines of "If session history is available, make sure that it's empty"?
that was discussed in bug 241972. I think the problem was that opening about:blank and writing content to it via DOM manipulations would fail this check and make the window close.
(In reply to comment #10)
Of course, the current tests remain - this is just an additional check as proposed by comment #8. With this patch Firefox (and all other Gecko apps) should behave as expected.
Assignee: file-handling → zeniko
Attachment #229976 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #230560 - Flags: superreview?(darin)
Attachment #230560 - Flags: review?(cbiesinger)
why is the flag on the channel set to true when no new window was actually opened?
It seems to me as though this ought to block the FF2 release since some people do run with this pref set and will be very surprised and very unhappy by this bug.
Flags: blocking1.8.1+
Severity: normal → major
Target Milestone: --- → mozilla1.8.1beta2
(In reply to comment #12)
> why is the flag on the channel set to true when no new window was actually
> opened?

In case you're talking about mShouldCloseWindow being set, I suppose this is because we assume we get a new window without making sure that it's not actually an existing one at

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/docshell/base/nsDocShell.cpp&rev=1.804&mark=6385,6389-6392#6368

Is there any way of testing whether the opened window isn't actually the current tab?
This patch introduces the same check as the other patch but already at docShell level where - as mentioned above - I suppose that the error happens. There's surely an even better way of fixing this, but this patch and the alternative above should at least be harmless enough to make it into Firefox 2.0.
Attachment #230887 - Flags: superreview?(darin)
Attachment #230887 - Flags: review?(cbiesinger)
Attachment #230560 - Flags: review?(cbiesinger) → review-
Comment on attachment 230887 [details] [diff] [review]
don't flag windows with session history as new

I guess this is ok then. I can't currently think of a better way than this. But please file a bug on improving this.
Attachment #230887 - Flags: review?(cbiesinger) → review+
Attachment #230560 - Attachment is obsolete: true
Attachment #230560 - Flags: superreview?(darin)
Whiteboard: [has patch][needs sr darin]
Comment on attachment 230887 [details] [diff] [review]
don't flag windows with session history as new

This patch is wrong, in my opinion.  Why is the code that actually knows whether the window is new (which is the XUL code that actually provides the window) not handling this?
And frankly, I would rather remove this pref value (given all the other bugs it has) than add a hack like this to docshell.
One other note:  This hack fails if session history is disabled.  This is clearly not a concern for Firefox, but in general embedding environments it is.
One last request:  Please do move bugs on docshell stuff into one of the embedding components, so I'll actually see them....  :(
Anybody having a cleaner solution to this problem, feel free to take this bug.

(In reply to comment #17)
> Why is the code that actually knows
> whether the window is new (which is the XUL code that actually provides the
> window) not handling this?

I don't know why this path was chosen in bug 241972, but this seems to have been planned to live in docShell from the beginning (introduced in bug 65777 and left there by yourself in bug 211816 - although apparently non-functional). I don't see a correct fix for this which could still make it into Firefox 2, so in the short term I'd rather have a hack like this (or a back-out of bug 241972) than nothing.

Removing the option to ignore _blank targets would mean a functional regression for many advanced users (and make e.g. phpBB forums a pain to use without software like Proxomitron or Greasemonkey). Even if slightly buggy, I prefer to not let web authors dictate when a new window/tab should be opened without my consent.

(In reply to comment #19)
> One other note:  This hack fails if session history is disabled.  This is
> clearly not a concern for Firefox, but in general embedding environments it is.

Would this work better if we checked for the window opener instead (which should be the current window if the load was diverted into a new window/tab)?
I'm not sure what bug 211816 has to do with this; I was certainly not involved in that bug...

> Removing the option to ignore _blank targets would mean a functional regression

Given the various crashes that option also causes, I would see that as the lesser evil, myself.

> I prefer to not let web authors dictate when a new window/tab should be
> opened without my consent.

You prefer to let them crash your browser?  ;)  In any case, apparently a lot of people, including yourself do; this has happened every time I bring up this issue.

> Would this work better if we checked for the window opener instead (which
> should be the current window if the load was diverted into a new window/tab)?

I would be happier with that, yes.  I'd also want a followup bug to remove that code and do this in a sane way., and someone actually working on said bug (instead of our usual "file a followup and forget about it" modus operandi).
Attached patch use owner-check instead (obsolete) — Splinter Review
This patch makes sure that the opening window is in fact the opener of the new window before marking it as new. This unfortunately won't work without modifying the owner setting code. Currently we always set the opening window as the opener of the returned window - even if it's the opening window itself. This would obviously make the opener check useless.

This patch simply prevent such a circular opener chain. An alternative would be to explicitly set the opener to null in the case where the opened window is identical to the opening one, since to the users of nsIDOMWindow::Open this looks as if the opening window had been closed when the new window was opened. This will break code one way or the other anyway, since I don't suppose web authors take the open_newwindow prefs into account. Nonetheless, that's simply the downside for users of that pref (such as myself).

(In reply to comment #22)
> I'm not sure what bug 211816 has to do with this; I was certainly not involved
> in that bug...

Sorry, I meant bug 323810. That's what you get without a post preview option. :(

> You prefer to let them crash your browser?  ;)

Never seen such a crash. ;) Now although ignorance is bliss, would you mind pointing me to such crash reports (a quick search didn't turn up any).

> I'd also want a followup bug to remove that code and do this in a sane way.

I prefer leaving this to somebody able to directly assign that bug to a dev capable of fixing it (which won't be me).
Attachment #231343 - Flags: superreview?(darin)
Attachment #231343 - Flags: review?(bzbarsky)
this doesn't ensure that the window was opened in this call. couldn't it have been opened in a previous call and the load diverted there?
If the owner check involves changing the current owner logic it's no good....  I thought we had a way to check for the "original" opener, but it looks like we don't.
(In reply to comment #23)
> I prefer leaving this to somebody able to directly assign that bug to a dev
> capable of fixing it (which won't be me).

In general, if you create a bug (including by checking in code that needs a bug to remove later), it's kinda your responsibility to either fix it or find someone else to do so...
Comment on attachment 231343 [details] [diff] [review]
use owner-check instead

(In reply to comment #24)
> this doesn't ensure that the window was opened in this call. couldn't it have
> been opened in a previous call and the load diverted there?

Indeed. Bad luck.

(In reply to comment #26)
> In general, if you create a bug (including by checking in code that needs a bug
> to remove later), it's kinda your responsibility to either fix it or find
> someone else to do so...

In that case, please back out the patch from bug 241972 which caused this regression in the first place. I don't feel like taking more responsibility than just getting a fix for this annoying bug into Firefox 2 (mainly because I'm not qualified to fix the underlying design issue nor do I know who would be).
Attachment #231343 - Attachment is obsolete: true
Attachment #231343 - Flags: superreview?(darin)
Attachment #231343 - Flags: review?(bzbarsky)
> In that case, please back out the patch from bug 241972 which caused this
> regression in the first place.

If you feel that that's the way to go, please feel free to file a bug on that (or reassign this one to the patch author from bug 241972, or whatever).

Feel free to ask the module owners for advice on who would be qualified to fix issues, for what it's worth.
OK. Unless that patch Boris doesn't like gets sr+ and a+, this won't be my bug.
-> Marria (what about backing out the patch to bug 241972 - at least on Trunk - and find a solution for both that bug and bug 346561 together?)
Assignee: zeniko → marria
Status: ASSIGNED → NEW
Is there something wrong with a simple check that  win is not equal to newWin?
A window provider can return an existing window not equal to the current one, in general.  For the purposes of Firefox the win != newWin check might be ok, dunno.
Darin and I discussed this approach and agree that it is at least an improvement to what is already there and seems to fix the bug.
Attachment #231547 - Attachment is obsolete: true
Attachment #231671 - Flags: superreview?(darin)
Attachment #231671 - Flags: review?(cbiesinger)
Attachment #231671 - Flags: review?(cbiesinger) → review+
(In reply to comment #32)
> Created an attachment (id=231671) [edit]

Oops, I just realized that I left this patch with 2 space indent - I'll fix that before submitting but I won't bother posting another patch here.
Attachment #231671 - Flags: superreview?(darin) → superreview+
Attachment #231671 - Flags: approval1.8.1?
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Patch checked in on trunk on 2006-08-02 23:22. (Marria, if you could comment in the bug when you check things in that would be great.)
(In reply to comment #34)

Will do, sorry about that.  I thought the protocol was just to mark it as fixed once it was checked in.
Blocks: 344795
Comment on attachment 231671 [details] [diff] [review]
protect against opened window being the same as the original window

a=schrep for drivers.
Attachment #231671 - Flags: approval1.8.1? → approval1.8.1+
checked in on branch
Keywords: fixed1.8.1
*** Bug 344795 has been marked as a duplicate of this bug. ***
Attachment #230887 - Attachment is obsolete: true
Attachment #230887 - Flags: superreview?(darin)
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.