Bug 23432 – window.open scripts do not trigger newWindowDelegate callback.

• Component: WebKit API
• Status: UNCONFIRMED

[Dirk Theisen]

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.

[Joone Hur]

In WebKitGtk port,  "new-window-policy-decision-requested"  signal isn't also triggered when using the window.open call.
It seems like a bug.

[Lucas De Marchi]

(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.

[Juan Pablo Ugarte]

Created attachment 70783 [details]
Simple test case, that show new-window-policy-decision-requested is not emited on JS window.open()

[Nicolas Dufresne]

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 "".

[Nicolas Dufresne]

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)

[Nicolas Dufresne]

(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 ?

[Nicolas Dufresne]

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 ...

[Nicolas Dufresne]

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().

[Nicolas Dufresne]

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.

[Nicolas Dufresne]

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.

[Nicolas Dufresne]

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

[Nicolas Dufresne]

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.

[Early Warning System Bot]

Attachment 73416 [details] did not build on qt:
Build output: http://queues.webkit.org/results/5528067

[WebKit Review Bot]

Attachment 73416 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/5496057

[Build Bot]

Attachment 73416 [details] did not build on win:
Build output: http://queues.webkit.org/results/5484069

[Nicolas Dufresne]

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.

[Adam Barth]

> 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.

[WebKit Review Bot]

Attachment 73416 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/5530077

[Nicolas Dufresne]

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.

[Nicolas Dufresne]

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.

[Nicolas Dufresne]

Created attachment 75107 [details]
Patch

[WebKit Review Bot]

Attachment 75107 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6459013

[Nicolas Dufresne]

(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

[Adam Barth]

Comment on attachment 75107 [details]
Patch

This patch has an incorrect ChangeLog, does not contain tests, and does not compile.

[Nicolas Dufresne]

(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.

[Martin Robinson]

*** Bug 34715 has been marked as a duplicate of this bug. ***