Bug 3308

Summary: Pop-up blocking blocks window.open for already open windows
Product: WebKit Reporter: Ryan Kaldari <kaldari>
Component: JavaScriptCoreAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: alice.barraclough, darin
Priority: P1 Keywords: InRadar
Version: 312.x   
Hardware: Mac   
OS: OS X 10.3   
URL: http://www.angelblade.com/bugs/popup/index.html
Attachments:
Description Flags
no test case yet, but would like a first pass of review anyway
none
patch with detailed change log and a layout test ggaren: review+

Description Ryan Kaldari 2005-06-07 11:37:50 PDT
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.
Comment 1 Joost de Valk (AlthA) 2005-09-04 23:03:29 PDT
Increasing priority since this bug is breaking some dutch banking sites.
Comment 2 Alice Liu 2005-11-04 10:27:04 PST
<rdar://problem/3853181> is assigned to vicki with P2
Comment 3 Darin Adler 2006-01-21 20:35:42 PST
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:].
Comment 4 Vicki Murley 2006-02-03 15:50:42 PST
I have the Radar for this, so I'm going to grab the Bugzilla too!
Comment 5 Darin Adler 2006-03-06 09:01:00 PST
Created attachment 6898 [details]
no test case yet, but would like a first pass of review anyway
Comment 6 Darin Adler 2006-03-09 23:37:04 PST
Created attachment 6976 [details]
patch with detailed change log and a layout test
Comment 7 Geoffrey Garen 2006-03-10 10:19:12 PST
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 :).
Comment 8 Darin Adler 2006-03-10 11:29:16 PST
(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.