208862 - Popup blocker isn't fast enough or good enough for this page

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Brian 'netdragon' Bober]

2003050714 WinXP

Steps to reproduce:
1. Go to http://www.catholicninjas.org/superfuntime/ with popup blocking 
disabled.
2. Close the window, and some popups come up giving you the "Popup manager" 
dialog box, and try to click yes.

Expected results:
You don't get any popups after clicking yes

Actual results:
Preferences window comes up and soon you have like 20 or more popup windows 
after clicking Yes, with more appearing constantly. Maybe all the popups is 
causing the browser to become unresponsive to you clicking Yes?

Comment 1

[Dan M]

Neat site. Catchy tune.

Comment 2

[Dan M]

Attachment: don't name windows using reserved target names

This unlikely patch fixes the problem. catholicninjas isn't doing anything
tricky; it just opens windows in the unload handler. We were allowing them to
slip through because named windows that will open into an extant window aren't
blocked, and because the popup windows were named "_blank", and because there
was indeed an open window named "_blank". That last one is the bug.

By the way I was able to reproduce this bug only using a new profile, or at
least one that hasn't yet seen a popup window. Disabling popups after the popup
cascade has already begun seems a necessary condition. Otherwise, they were
blocked as expected.

Comment 3

[Dan M]

Comment on attachment 125342 [details] [diff] [review]
don't name windows using reserved target names

hang on. sounds like this patch will create an incompatibility with IE. better
one's in the works.

Comment 4

[Dan M]

Attachment: don't name windows using reserved target names

Like the previous patch except this one makes us behave like IE -- sigh. We
won't name a window _blank (that's the important part), _self retains the
original name, and _foo is as valid a name as any.

Comment 5

[jag (Peter Annema)]

Comment on attachment 125355 [details] [diff] [review]
don't name windows using reserved target names

Looks gppd to me, take r= or sr= as needed

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 125355 [details] [diff] [review]
don't name windows using reserved target names

r/sr=jst

Comment 7

[Daniel Veditz [:dveditz]]

Putting on the 1.4 blocking list

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 125355 [details] [diff] [review]
don't name windows using reserved target names

I'd still like mscott to sr= this, but I can stamp sr=[e-mail] in
case that helps get it landed in the branch for tomorrow's builds.

Danm, you around?

/be

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 125355 [details] [diff] [review]
don't name windows using reserved target names

a=dveditz for drivers

Comment 10

[Dan M]

checked in on the 1.4 branch, but not yet on the trunk

Comment 11

[Dan M]

Attachment: window naming testcase

Not what this bug was originally about, but the reason it got fasttracked into
the branch. Here's the testcase I've been using; I figure it'll save someone a
minute's effort. Unpatched builds do disappointing things.

Comment 12

[Paul Wyskoczka]

Updating qa contact to sarah

Adding keyword fixed1.4, since this has landed on the branch

Comment 13

[Dan M]

Comment on attachment 125355 [details] [diff] [review]
don't name windows using reserved target names

Restoring the sr request specifically for docshell sr mscott. The bug already
has reviews from three sr people but we wanted him as well.

Comment 14

[sairuh (rarely reading bugmail)]

vrfy'd fixed on the branch (tested on win2k and linux rh8.0, 2003.06.11-1.4
commercial) --i can set the pref to block the popups.

Comment 15

[Scott MacGregor]

Comment on attachment 125355 [details] [diff] [review]
don't name windows using reserved target names

so we want to only set the name on the docshell if the window is new. That
seems ok to me.

Comment 16

[Dan M]

Thanks all. Patch is checked in on the trunk and the 1.4 branch. I believe the
bug is fixed, though there's some question since I had to modify the original
steps to reproduce.

There's still a more general and similar case which is not fixed. For that see
bug 209134.

Comment 17

[Kai Engert [:KaiE:]]

This change caused regression bug 209082.

It seems to cause that unnamed new windows are no longer called "_blank", which
breaks the assumption of some existing code.