Bug 44784 - [GTK] fast/history/history-subframe-with-name.html fails with GTK DRT
: [GTK] fast/history/history-subframe-with-name.html fails with GTK DRT
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Tools / Tests
: 528+ (Nightly build)
: All Linux
: P2 Normal
Assigned To: Martin Robinson
: Gtk
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-08-27 12:28 PDT by Mihai Parparita
Modified: 2011-02-07 09:44 PST (History)
10 users (show)

See Also:


Attachments
Patch (1.21 KB, patch)
2010-08-27 13:23 PDT, Mihai Parparita
no flags Details | Formatted Diff | Diff
Patch (3.37 KB, patch)
2011-02-06 09:14 PST, Martin Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mihai Parparita 2010-08-27 12:28:00 PDT
http://trac.webkit.org/changeset/66238 added fast/history/history-subframe-with-name.htm, and it fails with this diff:

--- /home/slave/webkitgtk/gtk-linux-64-debug/build/layout-test-results/fast/history/history-subframe-with-name-expected.txt	2010-08-27 11:51:06.357907230 -0700
+++ /home/slave/webkitgtk/gtk-linux-64-debug/build/layout-test-results/fast/history/history-subframe-with-name-actual.txt	2010-08-27 11:51:06.357907230 -0700
@@ -9,6 +9,6 @@
 PASS 2 is currentPageId
 PASS 3 is currentPageId
 PASS 2 is currentPageId
-PASS 3 is currentPageId
+FAIL 3 should be 2. Was 3.
 PASS Complete: navigated through all the states
Comment 1 Mihai Parparita 2010-08-27 13:23:08 PDT
Created attachment 65757 [details]
Patch
Comment 2 Dimitri Glazkov (Google) 2010-08-27 13:24:04 PDT
Comment on attachment 65757 [details]
Patch

ok.
Comment 3 Dimitri Glazkov (Google) 2010-08-27 13:44:41 PDT
Comment on attachment 65757 [details]
Patch

Clearing flags on attachment: 65757

Committed r66250: <http://trac.webkit.org/changeset/66250>
Comment 4 Dimitri Glazkov (Google) 2010-08-27 13:44:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Mihai Parparita 2010-08-27 13:48:20 PDT
Unfortunately I'm not really able to investigate the GTK test failure (I don't have a machine that can run the GTK tests).

Martin, do you think you could look into this? Let me know if you need any help with the changes that r66238 made or the new test (given that there's a bunch of other skipped history tests for GTK, it might just be missing DRT features).
Comment 6 Martin Robinson 2010-08-27 14:17:18 PDT
Sure. I'll take a look later today.
Comment 7 Martin Robinson 2010-09-04 11:21:27 PDT
I still haven't gotten very far on this. The WebKitGTK behavior is bewildering, to say the least.
Comment 8 Mihai Parparita 2010-09-08 11:13:37 PDT
(In reply to comment #7)
> I still haven't gotten very far on this. The WebKitGTK behavior is bewildering, to say the least.

Would there be any race conditions from doing a history.forward() in an onload handler (since presumably the GTK DRT uses the GTK implementation of the back-forward list)?
Comment 9 Martin Robinson 2011-02-04 17:10:17 PST
(In reply to comment #8)
> (In reply to comment #7)
> > I still haven't gotten very far on this. The WebKitGTK behavior is bewildering, to say the least.
> 
> Would there be any race conditions from doing a history.forward() in an onload handler (since presumably the GTK DRT uses the GTK implementation of the back-forward list)?

The issue here seems to be this:

1. When the history item is for the second subframe is creatied, it has a unique name like "<!--framePath //<!--frame1-->-->".

2. window.name of the subframe changes and its unique name in the tree becomes "foo" (since there is no collision -- see the first conditional in FrameTree::uniqueChildName).

3. When HistoryController::itemsAreClones runs during back/forward navigation, the two history items do not look like clones because their frame trees contain a different set of names.

So it appears that the problem is that the HistoryItem gets out of sync with the actual frame tree. I'm not sure why this does cause a problem on other ports.
Comment 10 Martin Robinson 2011-02-04 17:10:42 PST
CCing Daniel, since he has tangled recently with unique frame names.
Comment 11 Martin Robinson 2011-02-05 14:03:44 PST
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > I still haven't gotten very far on this. The WebKitGTK behavior is bewildering, to say the least.
> > 
> > Would there be any race conditions from doing a history.forward() in an onload handler (since presumably the GTK DRT uses the GTK implementation of the back-forward list)?
> 
> The issue here seems to be this:

This turned out to be a huge red herring, since other ports function even with this issue. It may point to some ineffeciency though. 

The problem that GTK+ is having is that it is calling FrameLoader::loadURLIntoChildFrame on the child frame's loader as opposed to the parent frame. Frankly, I'm amazed that this worked at all. I should have a patch soon.
Comment 12 Martin Robinson 2011-02-06 09:10:46 PST
*** Bug 50678 has been marked as a duplicate of this bug. ***
Comment 13 Martin Robinson 2011-02-06 09:14:48 PST
Created attachment 81412 [details]
Patch
Comment 14 Daniel Bates 2011-02-06 16:37:37 PST
Comment on attachment 81412 [details]
Patch

This makes sense to me.
Comment 15 Daniel Bates 2011-02-06 17:05:58 PST
Note, both the Haiku and EFL ports have a similar issue. That is, they use the child frame's loader to load the child frame instead of the parent frame's loader.
Comment 16 Martin Robinson 2011-02-07 08:35:41 PST
Comment on attachment 81412 [details]
Patch

Clearing flags on attachment: 81412

Committed r77818: <http://trac.webkit.org/changeset/77818>
Comment 17 Martin Robinson 2011-02-07 08:35:46 PST
All reviewed patches have been landed.  Closing bug.
Comment 18 WebKit Review Bot 2011-02-07 09:30:34 PST
http://trac.webkit.org/changeset/77818 might have broken Qt Linux Release, Qt Linux Release minimal, Qt Linux ARMv5 Release, Qt Linux ARMv7 Release, Qt Windows 32-bit Release, and Qt Windows 32-bit Debug
Comment 19 Csaba Osztrogonác 2011-02-07 09:44:30 PST
(In reply to comment #18)
> http://trac.webkit.org/changeset/77818 might have broken Qt Linux Release, Qt Linux Release minimal, Qt Linux ARMv5 Release, Qt Linux ARMv7 Release, Qt Windows 32-bit Release, and Qt Windows 32-bit Debug

77819 is the culprit