Closed
Bug 344808
Opened 19 years ago
Closed 19 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)
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)
142 bytes,
text/html
|
Details | |
2.11 KB,
patch
|
Biesinger
:
review+
darin.moz
:
superreview+
mtschrep
:
approval1.8.1+
|
Details | Diff | Splinter Review |
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
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
Comment 3•19 years ago
|
||
This might be a side-effect of bug 241972.
Comment 4•19 years ago
|
||
*** Bug 345325 has been marked as a duplicate of this bug. ***
Comment 5•19 years ago
|
||
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
Comment 6•19 years ago
|
||
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
Comment 7•19 years ago
|
||
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
Comment 8•19 years ago
|
||
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
Comment 9•19 years ago
|
||
Something along the lines of "If session history is available, make sure that it's empty"?
Comment 10•19 years ago
|
||
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.
Comment 11•19 years ago
|
||
(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)
Comment 12•19 years ago
|
||
why is the flag on the channel set to true when no new window was actually opened?
Comment 13•19 years ago
|
||
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+
Updated•19 years ago
|
Severity: normal → major
Target Milestone: --- → mozilla1.8.1beta2
Comment 14•19 years ago
|
||
(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?
Comment 15•19 years ago
|
||
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)
Updated•19 years ago
|
Attachment #230560 -
Flags: review?(cbiesinger) → review-
Comment 16•19 years ago
|
||
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+
Updated•19 years ago
|
Attachment #230560 -
Attachment is obsolete: true
Attachment #230560 -
Flags: superreview?(darin)
Updated•19 years ago
|
Whiteboard: [has patch][needs sr darin]
![]() |
||
Comment 17•19 years ago
|
||
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?
![]() |
||
Comment 18•19 years ago
|
||
And frankly, I would rather remove this pref value (given all the other bugs it has) than add a hack like this to docshell.
![]() |
||
Comment 19•19 years ago
|
||
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.
![]() |
||
Comment 20•19 years ago
|
||
One last request: Please do move bugs on docshell stuff into one of the embedding components, so I'll actually see them.... :(
Comment 21•19 years ago
|
||
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)?
![]() |
||
Comment 22•19 years ago
|
||
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).
Comment 23•19 years ago
|
||
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)
Comment 24•19 years ago
|
||
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?
![]() |
||
Comment 25•19 years ago
|
||
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.
![]() |
||
Comment 26•19 years ago
|
||
(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 27•19 years ago
|
||
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)
![]() |
||
Comment 28•19 years ago
|
||
> 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.
Comment 29•19 years ago
|
||
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
Assignee | ||
Comment 30•19 years ago
|
||
Is there something wrong with a simple check that win is not equal to newWin?
![]() |
||
Comment 31•19 years ago
|
||
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.
Assignee | ||
Comment 32•19 years ago
|
||
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)
Updated•19 years ago
|
Attachment #231671 -
Flags: review?(cbiesinger) → review+
Assignee | ||
Comment 33•19 years ago
|
||
(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.
Updated•19 years ago
|
Attachment #231671 -
Flags: superreview?(darin) → superreview+
Assignee | ||
Updated•19 years ago
|
Attachment #231671 -
Flags: approval1.8.1?
Assignee | ||
Updated•19 years ago
|
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment 34•19 years ago
|
||
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.)
Assignee | ||
Comment 35•19 years ago
|
||
(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.
Comment 36•19 years ago
|
||
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+
*** Bug 344795 has been marked as a duplicate of this bug. ***
Updated•19 years ago
|
Attachment #230887 -
Attachment is obsolete: true
Attachment #230887 -
Flags: superreview?(darin)
Updated•9 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•