Bug 153404 – REGRESSION(r188659): Non main frame scrollable areas don't work for pages restored from the page cache

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

Since r188659 FrameView scrollable areas are cleared when a page is cached in the history cache, but they are not restored on resume. The problem can be reproduced with safari:

 - Visit a page containing non frame scrollable areas, like a menu list, for example: http://www.robinlionheart.com/stds/html4/forms this page has a lot of scrollable areas
 - Note that you can scroll the examples and menu lists with the scroll wheel
 - Go back and then go forward
 - Try to scroll any of the areas using the scroll wheel, without using the scrollbars. The main frame is scrolled instead.

If you enable overlay scrollbars and repeat the steps you will notice that scrollbars never appear again for those areas, so it's impossible to scroll them. When the page is suspended, scrollbars are locked (see Page::lockAllOverlayScrollbarsToHidden), but they are not unclocked on resume, because the frame view has an empty ScrollabeAreas set.

[Simon Fraser (smfr)]

I've been investigating this too. The issue is that the FrameView's scrollableAreaSet is not updated with overflow and subframe regions when coming out of the page cache. I have a provisional patch.

[Simon Fraser (smfr)]

Created attachment 269884 [details]
WIP patch

Work-in-progress patch. It would be nice to do without the RenderLayer walk, but I'm not sure there's a way.

[Carlos Garcia Campos]

Created attachment 270922 [details]
Test

You can use this test (depends on bug #153479). You can see that after history navigation only the main frame receives events. The expected result is what we get if we remove the line 
testRunner.overridePreference("WebKitUsesPageCachePreferenceKey", 1);

[Carlos Garcia Campos]

Any progress on this? I've tried the patch but it doesn't seem to fix the issue. Can we roll out r188659 until this is properly fixed? This is blocking us to enable overlay scrollbars in GTK+ port.

[Simon Fraser (smfr)]

r188659 was fixing a security bug, so I don't want to roll it out. I'll try to get back to this patch soon.

[Carlos Garcia Campos]

(In reply to comment #5)
> r188659 was fixing a security bug, so I don't want to roll it out. I'll try
> to get back to this patch soon.

Ah, right, ok, let me know if I can help somehow. I'll take a look at r188659 in more detail to try to understand the problem.

[Carlos Garcia Campos]

Created attachment 271347 [details]
Different WIP approach

So, according to bug #148182 crashes happen when restoring from page cache because of pointers to ScrollableAreas that has already been removed. So, I think the actual bug is that those ScrollableAreas haven't been unregistered themselves when they were destroyed. RenderListBox and RenderLayer already do that, so I think the bug is in Mac PDFPlugin, and maybe in FrameView. This patch could also fix the crash without having to clear/restore scrollable areas when entering/restoring from the page cache. What do you think?

[Simon Fraser (smfr)]

Comment on attachment 271347 [details]
Different WIP approach

Seems reasonable, but you should make sure that you can actually get back to the FrameView in these two destructors (might be too late).

[Brent Fulgham]

(In reply to comment #8)
> Comment on attachment 271347 [details]
> Different WIP approach
> 
> Seems reasonable, but you should make sure that you can actually get back to
> the FrameView in these two destructors (might be too late).

Disconnecting destroyed renderers from the FrameView seems like a good thing to be doing. So if this changes fixes your problem, without reintroducing the issues from Bug 148182, then I'm in favor of it.

However, I don't really like that various Render elements have to do work to keep things in proper balance. E.g., RenderListBox and RenderLayer have cleanup code, now DeprecatedPDFPlugin will have special code. Are there others we are missing?

Should ScrollableAreas know about their containing frames, so they can unregister themselves during destruction, rather than doing so in the individual destructors for ScrollableArea descendants?

[Carlos Garcia Campos]

(In reply to comment #9)
> (In reply to comment #8)
> > Comment on attachment 271347 [details]
> > Different WIP approach
> > 
> > Seems reasonable, but you should make sure that you can actually get back to
> > the FrameView in these two destructors (might be too late).
> 
> Disconnecting destroyed renderers from the FrameView seems like a good thing
> to be doing. So if this changes fixes your problem, without reintroducing
> the issues from Bug 148182, then I'm in favor of it.

I can't be sure it doesn't reintroduce the issues from bug #148182, because I haven't been able to reproduce those crashes. Based on the analysis made in that bug I assume the problem is only with PDFPlugin, and it should be fixed by unregistering the plugin in the destructor, but someone with a mac who can reproduce the issue should confirm it.

> However, I don't really like that various Render elements have to do work to
> keep things in proper balance. E.g., RenderListBox and RenderLayer have
> cleanup code, now DeprecatedPDFPlugin will have special code.

I think it makes sense that classes registering themselves as scrollable areas unregister themselves as well. Ideally the base class could do it, but ScrollableArea is in platform layer and FrameView in WebCore layer. So, I don't think DeprecatedPDFPlugin will have any special code, it's registering itself as scrollable area in the FrameView and should unregister if it's destructed.

> Are there
> others we are missing?

No, there aren't.

> Should ScrollableAreas know about their containing frames, so they can
> unregister themselves during destruction, rather than doing so in the
> individual destructors for ScrollableArea descendants?

That would be a layering violation.

[Carlos Garcia Campos]

(In reply to comment #8)
> Comment on attachment 271347 [details]
> Different WIP approach
> 
> Seems reasonable, but you should make sure that you can actually get back to
> the FrameView in these two destructors (might be too late).

I guess the only way to be sure would be to also use WeakPtr

[Carlos Garcia Campos]

Created attachment 271424 [details]
Patch

For now I'm not using WeakPtr, because I'm not sure it's actually needed, so it's probably not worth it the extra space of the WeakPtrFactory in ScrollabelArea (see the compile assert ScrollableArea_should_stay_small), and also because WeakPtr is not hashable. If it's really needed, I guess we can just make it hahsable and add it to ScrollabelArea.

[Simon Fraser (smfr)]

Comment on attachment 271424 [details]
Patch

Making a test doesn't require a fix for bug 153479. You can make a test that dispatches mouse wheel events after navigating then going back, I have one half-written.

I'll try to reproduce the original crash today and test this patch.

[Carlos Garcia Campos]

(In reply to comment #13)
> Comment on attachment 271424 [details]
> Patch
> 
> Making a test doesn't require a fix for bug 153479. You can make a test that
> dispatches mouse wheel events after navigating then going back, I have one
> half-written.

I meant a test for this bug, not for bug #148182, that is attached to this bug already, but that depends on bug #153479. With current trunk, scrollable areas do actually scroll after being restored from the page cache when they receive wheel events, but the scroll animator is not notified. That's not a problem at all for non-overlay scrollbars that are always visible, but for overlay scrollbars that are shown when the mouse enters the scrollable area, we never see them (unless hovered) when the page is restored from the page cache.

> I'll try to reproduce the original crash today and test this patch.

Great, thanks!

[Simon Fraser (smfr)]

Fixed by rollout of r188659 in https://trac.webkit.org/r196641

[Simon Fraser (smfr)]

Adding tests that detect the regression via bug 154300.

[Simon Fraser (smfr)]

The supplied patch doesn't work; details in bug 148182.

[Carlos Garcia Campos]

(In reply to comment #16)
> Adding tests that detect the regression via bug 154300.

The problem of this regression was not that scrollable areas were not scrollable after being restored from the page cache, that still worked (at least in GTK+ port), because the scrollable areas still existed and the event handler correctly passed the wheel events to them. The problem was that the scroll animator was not notified when the mouse entered or exited the scrollable areas, which is what we use to show/hide the overlay scrollbars. I'll add a new test for this particular case.

[Carlos Garcia Campos]

Adding more tests in bug #154333