Bug 23432 - window.open scripts do not trigger newWindowDelegate callback.
Summary: window.open scripts do not trigger newWindowDelegate callback.
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 525.x (Safari 3.2)
Hardware: Mac (Intel) OS X 10.5
: P2 Normal
Assignee: Nobody
URL: http://code.google.com/p/pandoraboy/s...
Keywords:
: 34715 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-01-20 07:54 PST by Dirk Theisen
Modified: 2014-04-08 18:32 PDT (History)
14 users (show)

See Also:


Attachments
Simple test case, that show new-window-policy-decision-requested is not emited on JS window.open() (4.01 KB, text/x-csrc)
2010-10-14 14:53 PDT, Juan Pablo Ugarte
no flags Details
[PATCH 1/4] Allow creating hidden window (3.18 KB, patch)
2010-11-09 14:21 PST, Nicolas Dufresne
no flags Details | Formatted Diff | Diff
[PATCH 2/4] Show window only after initial location request (5.02 KB, patch)
2010-11-09 14:22 PST, Nicolas Dufresne
no flags Details | Formatted Diff | Diff
[PATCH 3/4] Don't duplicate code from FrameTree::find() (5.47 KB, patch)
2010-11-09 14:22 PST, Nicolas Dufresne
no flags Details | Formatted Diff | Diff
[PATCH 4/4] Trigger newWindowDelegate when window.open() is called (7.65 KB, patch)
2010-11-09 14:23 PST, Nicolas Dufresne
no flags Details | Formatted Diff | Diff
Patch (15.65 KB, patch)
2010-11-29 22:32 PST, Nicolas Dufresne
abarth: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Theisen 2009-01-20 07:54:50 PST
By WebKit delegate implements

- (void)webView:(WebView *)sender decidePolicyForNewWindowAction:(NSDictionary *)actionInformation request:(NSURLRequest *)request newFrameName:(NSString *)frameName decisionListener:(id < WebPolicyDecisionListener >)listener;

It gets called for href links an target set, but not for javascript window.open(url) links. Instead, a new window is silently requested and the new url is requested to load into that new window.

I'd expect the above delegate to be called prior to new window/frame generation with the url given in the window.open call.


In addition, on window.open the delegate method

- (WebView *)webView:(WebView *)sender createWebViewWithRequest:(NSURLRequest *)request

gets passed a nil request which makes it impossible to take appropriate action as a workaround. This might qualify for a different bug.
Comment 1 Joone Hur 2010-03-21 06:24:07 PDT
In WebKitGtk port,  "new-window-policy-decision-requested"  signal isn't also triggered when using the window.open call.
It seems like a bug.
Comment 2 Lucas De Marchi 2010-05-27 21:47:37 PDT
(In reply to comment #1)
> In WebKitGtk port,  "new-window-policy-decision-requested"  signal isn't also triggered when using the window.open call.
> It seems like a bug.

For javascript it will call ChromeClient::createWindow() instead. In EFL port I've implemented both ChromeClientEfl::createWindow() and ChromeClient::decidePolicyForNewWindowAction() by calling our ewk_view_window_create() with different parameters in order to be able to differentiate between javascript ones and link ones.

Unfortunately, sending upstream this code is being blocked by other bugs that are not reviewed yet. As soon as they are r+ cq+ I'll send them.
Comment 3 Juan Pablo Ugarte 2010-10-14 14:53:23 PDT
Created attachment 70783 [details]
Simple test case, that show new-window-policy-decision-requested is not emited on JS window.open()
Comment 4 Nicolas Dufresne 2010-10-18 12:03:51 PDT
For the record, this has something to do with a fixme in JSDOMWindowCustom.cpp line 678:

// FIXME: It's much better for client API if a new window starts with a URL, here where we
// know what URL we are going to open. Unfortunately, this code passes the empty string
// for the URL, but there's a reason for that. Before loading we have to set up the opener,
// openedByDOM, and dialogArguments values. Also, to decide whether to use the URL we currently
// do an allowsAccessFrom call using the window we create, which can't be done before creating it.
// We'd have to resolve all those issues to pass the URL instead of "".
Comment 5 Nicolas Dufresne 2010-10-19 11:14:59 PDT
After more analyses, I concluded that this missing newWindowDelegate callback hides a bigger problem. The problem is that checking the window policy is an asynchronous call but window.open() calls are handled synchronously. From my point of view, if new window is denied by the client, window.open() should return undefined. This cannot be achieved with current API.

As exposed by Lucas, one solution is to check the policy when ChromeClient::createWindow() is called. I'm currently writing a patch for the Gtk platform. There is other issues that will need fixing before it actually work.

1. The FrameLoadRequest passed to createWindow() always contains an empty URL ("").
2. There is only NavigationTypeOther that seems to fit, maybe a NavigationTypeJavascript would be better. (optional I guess)
Comment 6 Nicolas Dufresne 2010-10-19 13:07:31 PDT
(In reply to comment #5)
> I'm currently writing a patch for the Gtk platform. There is other issues that > will need fixing before it actually work.

Sorry for replying to my own comment, but it's not possible to implement the check in Gtk's ChromeClient::createWindow(). The reason is that Gtk port allow to queue the decisions for later.

I've compared window.open() with other similar API like history.go() and found that they all return void which make it simple to delay their execution. For window.open(), the return value is a JavaScript Window object, which is currently bound at creation time to the native frame/page object. This function is implemented as one synchronous native call, and thus cannot be interrupted and resumed later.

Base on these facts, I think a potential solution would be to delay the creation of native frame until the navigation policy decision has been taken. The side effect is that call to window.open() will return a JavaScript window even if the window creation is denied. Is this acceptable ?
Comment 7 Nicolas Dufresne 2010-11-09 08:52:59 PST
After more work on this I've finally managed to implement a solution that I will soon attach. The solution I've found is to allow creating page without making it visible (see Page::chrome().show() and WebCore::createWindow()). This enables two things: 1) you can actually request for location change before displaying the window, which allow making popup blocking decision/triage based on URI before the window is displayed and 2) for synchronous scripted popup (e.g. window.open()), you can simply create an hidden window, fire NewWindowPolicy on initial location change and show only if decision was positive (close if not).

At first I was concern about show() call being flipped with ChangeLocationPolicy, but I found that there exist cases where we already get this order. Stay tunned ...
Comment 8 Nicolas Dufresne 2010-11-09 14:21:29 PST
Created attachment 73415 [details]
[PATCH 1/4] Allow creating hidden window

Allow creating hidden window that can be destroyed if it's creation was
refused by NewWindowPolicy. This is required to fullfill the synchronous
requirement of JS call to window.open().
Comment 9 Nicolas Dufresne 2010-11-09 14:22:05 PST
Created attachment 73416 [details]
[PATCH 2/4] Show window only after initial location request

To fix this bug we enabled creating hidden windows. This patch takes
benifit of this new feature and queue location change before calling
Page::chrome()->show(). This mean URI can be considered sooner by
non-destructive popup blocker like the one in Chrome.
Comment 10 Nicolas Dufresne 2010-11-09 14:22:40 PST
Created attachment 73417 [details]
[PATCH 3/4] Don't duplicate code from FrameTree::find()

In JSDOMWindow::open() we duplicate some of the code from
FrameTree::find(). This patch also removes one call to FrameTree::find().
This code cleanup is attached to this bug since it ensure that createWindow
is actually called to create new window, reducing side effect risk.
Comment 11 Nicolas Dufresne 2010-11-09 14:23:12 PST
Created attachment 73418 [details]
[PATCH 4/4] Trigger newWindowDelegate when window.open() is called

This is a bit tricky since newWindowDelegate() is normally triggered before
creating the actual window. But since window.open() returns a window, we have
no choice but to create it. In this patch, we play with the window visibility,
creating the window and launching the newWindowDelegate() through
changeLocation() and a special optional parameter. The result is that the
window will only be showed if the policy is accepted, and will be closed
Comment 12 Nicolas Dufresne 2010-11-09 14:27:10 PST
Comment on attachment 73418 [details]
[PATCH 4/4] Trigger newWindowDelegate when window.open() is called

Note: style check fails because this patch depends on cleanup patch to apply cleanly.
Comment 13 Early Warning System Bot 2010-11-09 14:52:50 PST
Attachment 73416 [details] did not build on qt:
Build output: http://queues.webkit.org/results/5528067
Comment 14 WebKit Review Bot 2010-11-09 15:45:48 PST
Attachment 73416 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/5496057
Comment 15 Build Bot 2010-11-09 19:29:25 PST
Attachment 73416 [details] did not build on win:
Build output: http://queues.webkit.org/results/5484069
Comment 16 Nicolas Dufresne 2010-11-09 19:57:18 PST
Ok, tell me what is the procedure here, because what the automated build does wont work, you can't try all patches separately, e.g. 2 depends on 1, and 4 depends on 3 and 1. Maybe I should have requested review for 1 and 3 first, and waited for them to be upstream ?

thanks, and sorry for noise.
Comment 17 Adam Barth 2010-11-10 10:30:30 PST
> Ok, tell me what is the procedure here, because what the automated build does wont work, you can't try all patches separately, e.g. 2 depends on 1, and 4 depends on 3 and 1. Maybe I should have requested review for 1 and 3 first, and waited for them to be upstream ?

The EWS doesn't handle dependent patch sequences.  You're welcome to add support for them if you like, but it doesn't come up that often.  Sorry for the bugmail spam.
Comment 18 WebKit Review Bot 2010-11-10 11:58:27 PST
Attachment 73416 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/5530077
Comment 19 Nicolas Dufresne 2010-11-11 11:29:38 PST
Comment on attachment 73418 [details]
[PATCH 4/4] Trigger newWindowDelegate when window.open() is called

It's too soon to request review, need to wait for patch 1 and 3.
Comment 20 Nicolas Dufresne 2010-11-11 11:30:11 PST
Comment on attachment 73416 [details]
[PATCH 2/4] Show window only after initial location request

Too soon to request view, need to wait for patch 1.
Comment 21 Nicolas Dufresne 2010-11-29 22:32:42 PST
Created attachment 75107 [details]
Patch
Comment 22 WebKit Review Bot 2010-11-29 23:22:26 PST
Attachment 75107 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6459013
Comment 23 Nicolas Dufresne 2010-11-30 07:57:19 PST
(In reply to comment #22)
> Attachment 75107 [details] did not build on chromium:
> Build output: http://queues.webkit.org/results/6459013

It seems that I broke V8 build, I'll fix soon, see log:

In file included from WebCore/bindings/v8/specialization/V8BindingDOMWindow.h:34,
                 from WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp:53:
WebCore/bindings/generic/BindingDOMWindow.h: In static member function ‘static WebCore::Frame* WebCore::BindingDOMWindow<Binding>::createWindow(WebCore::State<Binding>*, WebCore::Frame*, WebCore::Frame*, WebCore::Frame*, const WTF::String&, const WTF::String&, const WebCore::WindowFeatures&, typename Binding::Value)’:
WebCore/bindings/generic/BindingDOMWindow.h:128: error: ‘create’ was not declared in this scope
Comment 24 Adam Barth 2011-04-26 15:26:11 PDT
Comment on attachment 75107 [details]
Patch

This patch has an incorrect ChangeLog, does not contain tests, and does not compile.
Comment 25 Nicolas Dufresne 2011-04-27 08:00:48 PDT
(In reply to comment #24)
> (From update of attachment 75107 [details])
> This patch has an incorrect ChangeLog

Please specify so I can fix.

> does not contain tests,
As explain in the log, this is not visible from HTML side. As every port wraps this differently, I cannot come up with a test that works for all ports, at best I can provide a GTK port, base on the test case originally attached.

> and does not compile.

This can be fixed, please leave the bug open, I need to rebase against the new folder organization, and I'm doing that on free time, I have no more particular interest on my side anymore, aside correctness. If anyone needs it sooner, well take over the patch, the implementation is correct.
Comment 26 Martin Robinson 2014-04-08 18:32:27 PDT
*** Bug 34715 has been marked as a duplicate of this bug. ***