[Webkit-unassigned] [Bug 34773] [GTK] Hits assertion on history back, with page cache enabled, in specific conditions

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=34773





--- Comment #7 from Gustavo Noronha (kov) <gns at gnome.org>  2010-02-10 12:08:22 PST ---
(In reply to comment #6)
> (In reply to comment #4)
> > > This one seems to be needed because although the view indeed restores the
> > > values it does it before we are able to restore our adjustments, so it's
> > > useless. If this is the case there should be some FIXME/comment in the code
> > > about it.
> > 
> > Yeah, exactly. There could be other ways of triggering this, but I think this
> > is a good solution that has low risk of unwanted side effects. I'll add a
> > comment.
> 
> So, the thing is that this seems really weird. The CachedFrame is restored, and
> the old FrameView set to the Frame, *after* the transition method is called, so
> it seems to me there's no way WebCore could be setting the scroll values before
> our method is called. Can you tell me where was that happening exactly?

The problem is that it doesn't set the scroll values again, at all, from all I
could find. The saved frameview does not have a change in size, so our
adjustment's upper is never updated.

And that is also why we get incorrect scroll offsets, or lose scrollbars when
page cache is enabled, with the current situation - it does not update the
upper limit of the adjustment when loading a cached page, so any setting of the
value is wrong, because it will likely be bound by incorrect values. And we are
the only ones that need this, since we are the only ones that use specific
platform structure to track these changes (adjustments).

Also, that bug that had the scroll return to the top after a while is a symptom
of cached frame views interacting with our adjustments.

I think it is likely that causing a notification like frameRectsChanged would
have the same result, but I think it might cause unwanted side-effects as well.
Like I said on jabber, this looks like we are doing work that should be
automatically done by WebCore, but this is just because we are duplicating
functionality that WebCore provides to be a better citizen with arbitrary
scrolling widgets.

If you want to do some testing, and to clear up any misunderstandings in my
explanation about what is missing without that change, add this patch:

diff --git a/WebCore/platform/gtk/ScrollViewGtk.cpp
b/WebCore/platform/gtk/ScrollViewGtk.cpp
index 848f8c9..6b5071a 100644
--- a/WebCore/platform/gtk/ScrollViewGtk.cpp
+++ b/WebCore/platform/gtk/ScrollViewGtk.cpp
@@ -99,6 +99,8 @@ void ScrollView::setGtkAdjustments(GtkAdjustment* hadj,
GtkAdjustment* vadj, 
         // code path, so we make sure they are up-to-date here. This
         // is needed for the parent scrolling widget to be able to
         // report correct values.
+        resetValues = true;
+
         m_horizontalAdjustment->lower = 0;
         m_horizontalAdjustment->upper = resetValues ? 0 : frameRect().width();
         m_horizontalAdjustment->value = resetValues ? 0 :
scrollOffset().width();

That restores the original behavior of this part of the code. You will notice
that the scrollbar behavior is normal when you browse through pages using
Epiphany, but you may eventually lose the scrollbar or something, because the
GtkScrolledWindow does not really know the info it should (you can check this
running the testwebview unit test), or maybe you won't get visible problems,
but having wrong information in the GtkScrolledWindow is where the problem is
at.

The second question will be why the hell this worked before. Well, it didn't
work very well before, with the page cache enabled - we did get weird
behaviour, and part of it was because the adjustment values that were being
used were not correct for that page.

Again, I understand you think it's weird, but I have come to expect our
scrolling code to need special behaviour, because we're so different when
compared to other ports, and the problem gets worse when we have ScrollViews
being cached, and reused (which is the base of all the problems here), while
our adjustments were not planned to be set in more than a ScrollView at once.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.