Bug 3308 – Pop-up blocking blocks window.open for already open windows

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Ryan Kaldari]

When pop-up blocking is turned on in Safari, the browser blocks execution of the window.open 
function even if the target window is already open. In other words, if you try to use the window.open 
function to change the URL location of an already open window, pop-up blocking will prevent this from 
working (during onload or onunload events). There is no reason that pop-up blocking should interfere 
with this capability since it has nothing to do with popping up windows. Many web applications rely on 
this functionality and are broken by Safari's pop-up blocking. Mozilla's pop-up blocking does the right 
thing in this case and doesn't interfere with using window.open to change the URL location of already 
open windows.

See http://www.angelblade.com/bugs/popup/index.html for an example.

[Joost de Valk (AlthA)]

Increasing priority since this bug is breaking some dutch banking sites.

[Alice Liu]

<rdar://problem/3853181> is assigned to vicki with P2

[Darin Adler]

Here's the line of code that needs to change. In the window.open implementation, there's this:

      if (!allowPopUp(exec, window) && !(frame->findFrame(frameName)))
          return jsUndefined();

It says "if there's not already a frame by this name, and we won't allow a pop-up, then do nothing".

The bug is that findFrame only looks for frames inside the current frame tree, not in other windows. We just need to fix the code to use a frame-searching function that implements the whole frame naming algorithm, like -[WebCoreFrameBridge findFrameNamed:].

[Vicki Murley]

I have the Radar for this, so I'm going to grab the Bugzilla too!

[Darin Adler]

Created attachment 6898 [details]
no test case yet, but would like a first pass of review anyway

[Darin Adler]

Created attachment 6976 [details]
patch with detailed change log and a layout test

[Geoffrey Garen]

Comment on attachment 6976 [details]
patch with detailed change log and a layout test

Looks great. A few comments.

Is there a reason for the layout test not to dumpAsText()?

+    Frame* childFrame = parentFrame->tree()->child(m_name);
+    if (childFrame)
+        childFrame->openURL(getDocument()->completeURL(relativeURL.qstring()));

I would move the assignment here inside the if statement, to match other code in your patch.

I would call "nameForNewChild" "uniqueNameForNewChild" or just "uniqueNameForChild". "New" is misleading since you call this function on old frames inside FrameTree::setName. Maybe you left out "unique" in order to hide within the Frame class the details of how new children are named, but doing so made it difficult for me to understand why you would have to call a method like this at all.

+    char suffix[40]; // 16 + maximum width of an unsigned integer
+    sprintf(suffix, "/<!--frame%u-->-->", childCount());

This looks like it works, but it's shady. I would *strongly* prefer it if we handled sprintf with thick rubber gloves. Also, "maximum" on what system? 32 bit? 64? 128? (By my calculations, it's 20 characters on a 64 bit system: 18,446,744,074,000,000,000 sans commas.) And what about the null terminator? I suggest something like:

ASSERT(sizeof(unsigned) <= 64);
char suffix[sizeof("/<!--frame-->-->") + 20]; // 20 is the number of characters required to print the largest unsigned integer on a 64 bit system
snprintf(suffix, sizeof(suffix), "/<!--frame%u-->-->", childCount());

DumpRenderTree needs a ChangeLog.

r=me, but given the sprintf business, I'm kinda rounding up :).

[Darin Adler]

(In reply to comment #7)
> Is there a reason for the layout test not to dumpAsText()?

I tried it, and it just dumped as the empty string. I don't think frame contents are included in dumpAsText.

> +    Frame* childFrame = parentFrame->tree()->child(m_name);
> +    if (childFrame)
> +       
> childFrame->openURL(getDocument()->completeURL(relativeURL.qstring()));
> 
> I would move the assignment here inside the if statement, to match other code
> in your patch.

OK.

> I would call "nameForNewChild" "uniqueNameForNewChild" or just
> "uniqueNameForChild". "New" is misleading since you call this function on old
> frames inside FrameTree::setName. Maybe you left out "unique" in order to hide
> within the Frame class the details of how new children are named, but doing so
> made it difficult for me to understand why you would have to call a method like
> this at all.

I'd like to find a better name. I think uniqueNameForChild is probably good.

> > +    char suffix[40]; // 16 + maximum width of an unsigned integer
> +    sprintf(suffix, "/<!--frame%u-->-->", childCount());
> 
> This looks like it works, but it's shady. I would *strongly* prefer it if we
> handled sprintf with thick rubber gloves. Also, "maximum" on what system? 32
> bit? 64? 128? (By my calculations, it's 20 characters on a 64 bit system:
> 18,446,744,074,000,000,000 sans commas.) And what about the null terminator? I
> suggest something like:
> 
> ASSERT(sizeof(unsigned) <= 64);
> char suffix[sizeof("/<!--frame-->-->") + 20]; // 20 is the number of characters
> required to print the largest unsigned integer on a 64 bit system
> snprintf(suffix, sizeof(suffix), "/<!--frame%u-->-->", childCount());

OK. I'll do something more like what you suggest.

> DumpRenderTree needs a ChangeLog.

Will do.