Bug 48865

Summary: [GTK] Link with target name set does not work correctly
Product: WebKit Reporter: Nicolas Dufresne <nicolas>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, mrobinson, ossy, xan.lopez
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
HTML document that can be use to demonstrate the bug
none
Set global PageGroup name to all created pages
none
Set global PageGroup name to all created pages
none
Set global PageGroup name to all created pages
mrobinson: review-
Set global PageGroup name to all created pages
none
Set global PageGroup name to all created pages none

Description Nicolas Dufresne 2010-11-02 13:15:33 PDT
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.
Comment 1 Nicolas Dufresne 2010-11-02 13:25:29 PDT
Created attachment 72727 [details]
Set global PageGroup name to all created pages
Comment 2 Nicolas Dufresne 2010-11-02 14:46:31 PDT
Sorry, wrong reference for WebKitWebGroup API, see https://bugs.webkit.org/show_bug.cgi?id=32789
Comment 3 Nicolas Dufresne 2010-11-03 13:45:31 PDT
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.
Comment 4 Martin Robinson 2010-11-03 23:30:56 PDT
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.
Comment 5 Nicolas Dufresne 2010-11-04 05:42:38 PDT
(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.
Comment 6 Martin Robinson 2010-11-04 06:56:28 PDT
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.
Comment 7 Nicolas Dufresne 2010-11-04 09:56:59 PDT
Created attachment 72951 [details]
Set global PageGroup name to all created pages
Comment 8 Nicolas Dufresne 2010-11-04 10:04:58 PDT
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).
Comment 9 Martin Robinson 2010-11-04 14:58:54 PDT
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.
Comment 10 Nicolas Dufresne 2010-11-05 11:59:47 PDT
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).
Comment 11 Martin Robinson 2010-11-05 22:25:59 PDT
Comment on attachment 73097 [details]
Set global PageGroup name to all created pages

Great!
Comment 12 WebKit Commit Bot 2010-11-05 22:52:32 PDT
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>
Comment 13 WebKit Commit Bot 2010-11-05 22:52:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Csaba Osztrogonác 2010-11-08 04:00:12 PST
Reverted r71466 for reason:

It

Committed r71510: <http://trac.webkit.org/changeset/71510>
Comment 15 Csaba Osztrogonác 2010-11-08 04:02:19 PST
(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
Comment 16 Nicolas Dufresne 2010-11-08 05:22:27 PST
(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.
Comment 17 Csaba Osztrogonác 2010-11-08 05:38:41 PST
(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.
Comment 18 Nicolas Dufresne 2010-11-08 10:11:39 PST
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.
Comment 19 Martin Robinson 2010-11-08 10:36:25 PST
Comment on attachment 73248 [details]
Set global PageGroup name to all created pages

Okay. Let's try this again.
Comment 20 WebKit Commit Bot 2010-11-08 21:06:07 PST
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>
Comment 21 WebKit Commit Bot 2010-11-08 21:06:13 PST
All reviewed patches have been landed.  Closing bug.