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
: WebKit
Tools / Tests
: 528+ (Nightly build)
: All Linux
: P2 Normal
Assigned To:
:
: Gtk
:
:
  Show dependency treegraph
 
Reported: 2010-08-27 12:28 PST by
Modified: 2011-02-07 09:44 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-08-27 12:28:00 PST
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 From 2010-08-27 13:23:08 PST -------
Created an attachment (id=65757) [details]
Patch
------- Comment #2 From 2010-08-27 13:24:04 PST -------
(From update of attachment 65757 [details])
ok.
------- Comment #3 From 2010-08-27 13:44:41 PST -------
(From update of attachment 65757 [details])
Clearing flags on attachment: 65757

Committed r66250: <http://trac.webkit.org/changeset/66250>
------- Comment #4 From 2010-08-27 13:44:45 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #5 From 2010-08-27 13:48:20 PST -------
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 From 2010-08-27 14:17:18 PST -------
Sure. I'll take a look later today.
------- Comment #7 From 2010-09-04 11:21:27 PST -------
I still haven't gotten very far on this. The WebKitGTK behavior is bewildering, to say the least.
------- Comment #8 From 2010-09-08 11:13:37 PST -------
(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 From 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 From 2011-02-04 17:10:42 PST -------
CCing Daniel, since he has tangled recently with unique frame names.
------- Comment #11 From 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 From 2011-02-06 09:10:46 PST -------
*** Bug 50678 has been marked as a duplicate of this bug. ***
------- Comment #13 From 2011-02-06 09:14:48 PST -------
Created an attachment (id=81412) [details]
Patch
------- Comment #14 From 2011-02-06 16:37:37 PST -------
(From update of attachment 81412 [details])
This makes sense to me.
------- Comment #15 From 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 From 2011-02-07 08:35:41 PST -------
(From update of attachment 81412 [details])
Clearing flags on attachment: 81412

Committed r77818: <http://trac.webkit.org/changeset/77818>
------- Comment #17 From 2011-02-07 08:35:46 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #18 From 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 From 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