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

• Component: Page Loading
• Status: RESOLVED FIXED

[Gustavo Noronha (kov)]

Created attachment 48436 [details]
test case

I am attaching a (reduced as much as I could) test case. It seems like we hit this assertion when going back to a page that has been removed from the page cache, but only after scrolling one of the pages, and only if one of the pages has frames. Some of these may actually not be necessary, but those were the conditions we could reproduce the crash more consistently with. We use 3 pages as the page cache limit; raising that limit to 5 makes this specific test case for the crash not work, which indicates it only happens when going to a non-cached page.

[Gustavo Noronha (kov)]

Created attachment 48496 [details]
Disconnect adjustments, and reconnect them when needed

[Xan Lopez]

(In reply to comment #1)
> Created an attachment (id=48496) [details]
> Disconnect adjustments, and reconnect them when needed

This looks very good to me, I have just a few questions/doubts:

- I don't think the test really needs to open new windows? I certainly got the crashes without doing that.
- Do you really need the fourth page just for the PASS? Can't you just print that  when you are back at the beginning or something? Or just get rid of it completely, since the tests passes if you don't crash anyway.
- Do we really need the bit about saving the values in the adjustments? Shouldn't that be set by the view when it's restored automatically through the usual path? Or was that dependent on a new view being created?

[Xan Lopez]

(In reply to comment #2)

Just to update this after some chat through jabber.

> - I don't think the test really needs to open new windows? I certainly got the
> crashes without doing that.

Apparently this was only done to organize the test, but it's not needed for it to crash. Guess we can try to get rid of it.

> - Do you really need the fourth page just for the PASS? Can't you just print
> that  when you are back at the beginning or something? Or just get rid of it
> completely, since the tests passes if you don't crash anyway.

Didn't discuss this :P

> - Do we really need the bit about saving the values in the adjustments?
> Shouldn't that be set by the view when it's restored automatically through the
> usual path? Or was that dependent on a new view being created?

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.

[Gustavo Noronha (kov)]

(In reply to comment #3)
> (In reply to comment #2)
> 
> Just to update this after some chat through jabber.

Too quick, because I was soo hungry =D

> > - I don't think the test really needs to open new windows? I certainly got the
> > crashes without doing that.
> 
> Apparently this was only done to organize the test, but it's not needed for it
> to crash. Guess we can try to get rid of it.

Yes, but I don't think it's worth the effort. The test works fine like it is, and it uses the same pattern as fast/harness/use-page-cache.html, so unless you have something ready I think we can leave it as it is.

> > - Do you really need the fourth page just for the PASS? Can't you just print
> > that  when you are back at the beginning or something? Or just get rid of it
> > completely, since the tests passes if you don't crash anyway.
> 
> Didn't discuss this :P

The fourth page was only needed to cause the first one to be evicted from the page cache. I guess we could replace this with a first page that is never put in the page cache instead. Would be a better solution overall.  I'll see if I can do that.

> > - Do we really need the bit about saving the values in the adjustments?
> > Shouldn't that be set by the view when it's restored automatically through the
> > usual path? Or was that dependent on a new view being created?
> 
> 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.

[Gustavo Noronha (kov)]

Created attachment 48507 [details]
Second version with suggestions by Xan addressed

[Xan Lopez]

(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?

[Gustavo Noronha (kov)]

(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.

[Gustavo Noronha (kov)]

(In reply to comment #7)
> 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.

That is a bit wrong, so I should rephrase before you catch me lying =D - it does try setting the scroll offset (which is why we do get the actual scroll on the view), but with an incorrect upper this goes wrong for the GtkScrolledWindow.

[Gustavo Noronha (kov)]

Comment on attachment 48507 [details]
Second version with suggestions by Xan addressed

Landed as r54620, after xan r=me'd on IRC.