WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
153404
REGRESSION(
r188659
): Non main frame scrollable areas don't work for pages restored from the page cache
https://bugs.webkit.org/show_bug.cgi?id=153404
Summary
REGRESSION(r188659): Non main frame scrollable areas don't work for pages res...
Carlos Garcia Campos
Reported
2016-01-24 01:54:28 PST
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.
Attachments
WIP patch
(18.25 KB, patch)
2016-01-26 09:18 PST
,
Simon Fraser (smfr)
no flags
Details
Formatted Diff
Diff
Test
(1.29 KB, text/html)
2016-02-09 05:01 PST
,
Carlos Garcia Campos
no flags
Details
Different WIP approach
(1.66 KB, patch)
2016-02-15 09:48 PST
,
Carlos Garcia Campos
no flags
Details
Formatted Diff
Diff
Patch
(4.76 KB, patch)
2016-02-16 04:14 PST
,
Carlos Garcia Campos
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Simon Fraser (smfr)
Comment 1
2016-01-24 10:46:04 PST
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)
Comment 2
2016-01-26 09:18:10 PST
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
Comment 3
2016-02-09 05:01:59 PST
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
Comment 4
2016-02-15 00:09:55 PST
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)
Comment 5
2016-02-15 08:41:54 PST
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
Comment 6
2016-02-15 08:47:15 PST
(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
Comment 7
2016-02-15 09:48:51 PST
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 8
2016-02-15 10:36:46 PST
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
Comment 9
2016-02-15 11:33:39 PST
(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
Comment 10
2016-02-16 03:43:24 PST
(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
Comment 11
2016-02-16 03:44:04 PST
(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
Comment 12
2016-02-16 04:14:59 PST
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 13
2016-02-16 08:54:15 PST
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
Comment 14
2016-02-16 09:06:02 PST
(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)
Comment 15
2016-02-16 10:49:36 PST
Fixed by rollout of
r188659
in
https://trac.webkit.org/r196641
Simon Fraser (smfr)
Comment 16
2016-02-16 11:21:01 PST
Adding tests that detect the regression via
bug 154300
.
Simon Fraser (smfr)
Comment 17
2016-02-16 16:45:00 PST
The supplied patch doesn't work; details in
bug 148182
.
Carlos Garcia Campos
Comment 18
2016-02-16 23:21:26 PST
(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
Comment 19
2016-02-16 23:34:45 PST
Adding more tests in
bug #154333
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug