Bug 48865 – [GTK] Link with target name set does not work correctly

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Nicolas Dufresne]

Created attachment 72725 [details]
HTML document that can be use to demonstrate the bug

When a link with attribute target="myTarget" is clicked twice, two new windows are being opened. This bug is cause by all new pages being created in there own PageGroup. Until a WebKitGTK+ provide an API for PageGroup (see https://bugs.webkit.org/show_bug.cgi?id=16977) we should fix this bug by using a global page group name for all pages.

[Nicolas Dufresne]

Created attachment 72727 [details]
Set global PageGroup name to all created pages

[Nicolas Dufresne]

Sorry, wrong reference for WebKitWebGroup API, see https://bugs.webkit.org/show_bug.cgi?id=32789

[Nicolas Dufresne]

Some people ask on IRC if that was fixing some tests (or required additional tests). This bug is covered buy LayoutTests/http/tests/navigation/target-frame-from-window.html and works in the GTK DRT but not in GtkLauncher. The DRT uses private API function webkit_web_view_set_group_name() to set a constant group name to every newly created View/Page. The propose solution is to adopt a default constant group name to all newly created page.

While the best final solution would be to give control to user over group names (probably useful for Desktop widgets and/or privacy mode), we should still be able to get correct result with minimum effort, which is why I'm proposing this one liner fix.

[Martin Robinson]

Comment on attachment 72727 [details]
Set global PageGroup name to all created pages

r-, because, if I recall correctly you are working on a new version of this patch which also unskips a test.

[Nicolas Dufresne]

(In reply to comment #4)
> (From update of attachment 72727 [details])
> r-, because, if I recall correctly you are working on a new version of this patch which also unskips a test.

Not exactly. What I was trying to say in previous message is that LayoutTests/http/tests/navigation/target-frame-from-window.html is the test that covers that bug (no need to write a new one). What I found is that the test pass in DumpRenderTree but fails in any other WebKitGTK+ application. The reason is because DRT uses a non-public API to do just like my patch does locally, which is to use same PageGroup name for all pages.

Please reconsider reviewing this patch.

[Martin Robinson]

One way we can ensure adequate testing here remove webkit_web_view_set_group_name(view, "org.webkit.gtk.DumpRenderTree"); from the DRT and the webkit_web_view_set_group_name private API.

[Nicolas Dufresne]

Created attachment 72951 [details]
Set global PageGroup name to all created pages

[Nicolas Dufresne]

Created attachment 72955 [details]
Set global PageGroup name to all created pages

This one removes the set_group_name() call in DRT and removed the uneeded extern declaration for the private set_group_name() method (not sure if it's used anywhere else now).

[Martin Robinson]

Comment on attachment 72955 [details]
Set global PageGroup name to all created pages

View in context: https://bugs.webkit.org/attachment.cgi?id=72955&action=review

Great! Is it possible to go one step further and remove webkit_web_view_set_group_name from webkitwebview.cpp entirely, since, if I understand correctly, it is now dead code.

> WebKit/gtk/webkit/webkitwebview.cpp:3250
> +    priv->corePage->setGroupName("WebKitGTK");

I think we should have a comment here explaining why this is necessary.

[Nicolas Dufresne]

Created attachment 73097 [details]
Set global PageGroup name to all created pages

As requested, I added a comment and removed the set_group_name() method from private API (and all reference to it).

[Martin Robinson]

Comment on attachment 73097 [details]
Set global PageGroup name to all created pages

Great!

[WebKit Commit Bot]

Comment on attachment 73097 [details]
Set global PageGroup name to all created pages

Clearing flags on attachment: 73097

Committed r71466: <http://trac.webkit.org/changeset/71466>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Reverted r71466 for reason:

It

Committed r71510: <http://trac.webkit.org/changeset/71510>

[Csaba Osztrogonác]

(In reply to comment #14)
> Reverted r71466 for reason:
> Committed r71510: <http://trac.webkit.org/changeset/71510>

It broke layout tests on all GTK bots. 
After an inspector test, run-webkit-tests timed out:

inspector ...................
command timed out: 1200 seconds without output, killing pid 12840
process killed by signal 9
program finished with exit code -1

[Nicolas Dufresne]

(In reply to comment #15)
> (In reply to comment #14)
> > Reverted r71466 for reason:
> > Committed r71510: <http://trac.webkit.org/changeset/71510>
> 
> It broke layout tests on all GTK bots. 
> After an inspector test, run-webkit-tests timed out:
> 
> inspector ...................
> command timed out: 1200 seconds without output, killing pid 12840
> process killed by signal 9
> program finished with exit code -1

Please provide additionnal information, this patch does not change the code bahaviour, it just moves the group name setting somewhere else. It most likely the wrong patch being reverted, or simply a luck that you don't get deadlock without it.

Thanks.

[Csaba Osztrogonác]

(In reply to comment #16)
> Please provide additionnal information, this patch does not change the code bahaviour, it just moves the group name setting somewhere else. It most likely the wrong patch being reverted, or simply a luck that you don't get deadlock without it.

I don't have any additional information, I only saw that GTK bots were sick 2 days ago. I didn't revert wrong patch, it was the culprit. Maybe a hidden bug was revealed by this patch, I don't know. But I think well functioning buildbots are more important than one patch.

[Nicolas Dufresne]

Created attachment 73248 [details]
Set global PageGroup name to all created pages

Found that when the inspector as same PageGroup as the page being debugged we may deadlock. In this updated patch I reset the Inspector page group to empty string, which result in creating a private group for the inspector. I also updated the changelogs.

[Martin Robinson]

Comment on attachment 73248 [details]
Set global PageGroup name to all created pages

Okay. Let's try this again.

[WebKit Commit Bot]

Comment on attachment 73248 [details]
Set global PageGroup name to all created pages

Clearing flags on attachment: 73248

Committed r71604: <http://trac.webkit.org/changeset/71604>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.