263844 - window.close() not working on new windows opened into tabs

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Ger Teunis]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20041010 Firefox/0.10
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20041010 Firefox/0.10

A javascript popup-tab can't be closed using window.close();
Popups open in tabs when tabbed browsing option "Force links that open new
windows to open in:" is set to "a new tab".

Reproducible: Always
Steps to Reproduce:
1. Configure "Force links that open new windows to open in:" to "a new tab"
2. Go to above mentioned URL
3. Click on Google Print image (which opens a java popup in a tab)
4. Click on the image to close the page (it uses window.close();)
Actual Results:  
A javascript-error saying: 
"Scripts may not close windows that were not opened by script." and window is
not closed.

Expected Results:  
The java-popup-tab should be closed.

Comment 1

[Ger Teunis]

I must make a note: the popup IS opened using a script. So it should be closable
using a window.close().

Comment 2

[Pavel Penaz]

Confirming with the latest branch build on WinXP Pro.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20041012
Firefox/0.10

Comment 3

[Bradley Chapman (not reading bugmail, still gone but not forever)]

An extension developer named rue created a hackish solution to this problem by
building a function union inside the tab created by his window diversion code
and placing a close() call inside the union which called the tab's close()
function. The follwoing code is similar to what he did (note that he also
added a focus() command):

if (!newTab.close) {^M
  // add higher-level "window controls" so our opener can handle us
  newTab.TBPcfFunc = {
    close: function { window.getBrowser().removeTab(this); }, 
                    // where "this" is the tab we've been called on
    focus: function() { window.getBrowser().selectedTab = this; }
  };
  newTab.close = newTab.TBPcfFunc.close;
  newTab.focus = newTab.TBPcfFunc.focus;
}
newTab.opener = this; // set tab's opener to current (sub)document context

Comment 4

[Boris Zbarsky [:bzbarsky]]

Requesting blocking.  See bug 172962 comment 181.

Comment 5

[Dan M]

comment 3: If I understand correctly, that this solution allows window.close to
work even on windows not opened from script, that's a terrible solution and rue
should be taken out and shot for introducing such an astonishing corruption into
the browser. If I misunderstood, then what a clever fellow rue is.

Comment 6

[Bradley Chapman (not reading bugmail, still gone but not forever)]

(In reply to comment #5)
> comment 3: If I understand correctly, that this solution allows window.close to
> work even on windows not opened from script, that's a terrible solution and rue
> should be taken out and shot for introducing such an astonishing corruption into
> the browser. If I misunderstood, then what a clever fellow rue is.

You misunderstood - he only applies this modification inside a function which
his extension calls whenever it detects a window.open() call being made.
Ordinary pages DO NOT suddenly get the ability to close themselves; only the
tabs created from diverted windows can achieve that feat.

Clever, huh?

Comment 7

[Bradley Chapman (not reading bugmail, still gone but not forever)]

As per my comment in bug 264395, would setting window.opener and window.focus()
as well as window.close() fall within the scope of this bug? The code snippet in
comment 3 handles all three cases, and as mentioned previously, was first
thought up by rue.

If that code snippet does fall within this bug, a better summary might be
"JavaScript windows cannot call common window functions when diverted into tabs
(.opener, .focus(), .close())".

Comment 8

[Brendan Eich [:brendan]]

Dan says he can fix this in five lines, to set opener.

/be

Comment 9

[David Brown]

I have observed this error today...using Yahoo Mail, I created a printable view
of my message in the open tab, but the window would not close.  The browser did
not crash or do anything else, just couldn't back to my original message view
without closing the tab completely and opening a new one.  Hope this helps.

David Brown
XP Pro, SP2

(I LOVE the browser!)

Comment 10

[Dan M]

Attachment: set opener on opened windows diverted into tabs

It's easy to set the opener for windows diverted into tabs from left-clicks and
script. It turns out it's quite difficult to do this for middle-click windows.
Practically impossible, I think.

I would have liked to fix all of these windows, but this patch fixes only the
more visible kind revealed by bug 172962.

The patch is two sets of five lines each, by the way.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 162257 [details] [diff] [review]
set opener on opened windows diverted into tabs

r+sr=bzbarsky.	Dan, thanks for doing this!

I really wish we could factor some of this code that's being copied from
windowwatcher all over, thoug.	Maybe we need some utility functions somewhere
that this and windowwatcher would both call?

Comment 12

[Dan M]

Yes this patch has seen a lot of code copying. I'm not too concerned about this
patch: there's just something you have to do to link a window to its opener and
SetOpenerWindow is the utility routine. That 2004-10-07 patch to bug 172962
though is pretty icky in the clone way.

Patch is checked into the Aviary branch.

Comment 13

[Dan M]

Uh... after re-reading my previous comment, I think I need to clarify that I
meant that bug 172962 has caused code copying but that the patch in this bug
wasn't so bad.

Comment 14

[chris hofmann]

there is somethinking that this patch open us up to the crash in
https://bugzilla.mozilla.org/show_bug.cgi?id=232356

Comment 15

[chris hofmann]

er, really https://bugzilla.mozilla.org/show_bug.cgi?id=265790

Comment 16

[Brian Ryner (not reading)]

I backed out this patch due to crasher bug 265790.  I've posted some findings 
on the crash over there.

Comment 17

[Johnny Stenback (:jst)]

Attachment: Danm's fix plus a tabbrowser.xml fix to prevent the crashes introduced in the initial landing of the fix for this bug.

Comment 18

[Brian Ryner (not reading)]

Comment on attachment 163514 [details] [diff] [review]
Danm's fix plus a tabbrowser.xml fix to prevent the crashes introduced in the initial landing of the fix for this bug.

>+            // There's only one browser left. If a window is being
>+            // closed and the window is *not* the window in the
>+            // browser that's still around, prevent the events default

nit: "event's".  r+sr=me with that fix.

Comment 19

[Johnny Stenback (:jst)]

Comment on attachment 163514 [details] [diff] [review]
Danm's fix plus a tabbrowser.xml fix to prevent the crashes introduced in the initial landing of the fix for this bug.

To my best knowledge this fixes this bug *and* the crash that the initial fix
caused. Dan, can you give this a spin too? If all looks good to all here, we
should probably take this on the branch again...

Comment 20

[Johnny Stenback (:jst)]

Comment on attachment 163514 [details] [diff] [review]
Danm's fix plus a tabbrowser.xml fix to prevent the crashes introduced in the initial landing of the fix for this bug.

Er, ?, not +

Comment 21

[Dan M]

Johnny: Your patch works really well but it doesn't fix the popuptest.com crash;
the third example in bug 232356 comment 9. That's the one that caused me the
most trouble, too. It's still a nice patch and helpful -- you should probably
check it in as a mostly fix.

Comment 22

[Brendan Eich [:brendan]]

Comment on attachment 163514 [details] [diff] [review]
Danm's fix plus a tabbrowser.xml fix to prevent the crashes introduced in the initial landing of the fix for this bug.

a=me for aviary branch.

/be

Comment 23

[Johnny Stenback (:jst)]

Fix landed on the aviary branch.

Comment 24

[Jay Patel [:jay]]

I wanted to find out if others are seeing what I am seeing with today's build
(20041028):

After testing both RC1 (20041026) and today's build (20041028) against this bug,
bug 263844 and bug 232356, I see the following:

1. RC1
  - All 3 testcases in https://bugzilla.mozilla.org/show_bug.cgi?id=232356#c9
work fine without a crash (bug 265790 after checkin)
  - window.close() in http://www.pcmweb.nl/nieuws.jsp?id=381901 for the Google
News image does not work (bug 263844 after bryner's backout of orig. patch)
  - Running parent.close() in JS Console crashes (bug 232356)

2. Today's 20041028 build
  - All 3 testcases in https://bugzilla.mozilla.org/show_bug.cgi?id=232356#c9
lead to a crash (bug 265790 after checkin)
  - window.close() in http://www.pcmweb.nl/nieuws.jsp?id=381901 for the Google
News image does work as expected (bug 263844 after jst's new patch)
  - Running parent.close() in JS Console still crashes (Bug 232356)

So it looks like the new patch here might have introduced some regressions that
still break bug 265790.  Anyone else crashing with today's build?

Comment 25

[Jim Ginn]

Jay, I see the same thing using the latest nightly branch build (20041028).

Comment 26

[Mike Kaply [:mkaply]]

1. Should this patch be in 1.7 so we have in sync Geckos?

2. Was it landed on the trunk?

3. Why is this bug still open?

Comment 27

[Dan M]

1: This patch isn't helpful in 1.7 (or on any branch) without the rest of
   single window mode (bug 172962)
3: See #2

Fixed Trunk 1.8a6, Aviary/Firefox 1.0