Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2015373002:
Fix webview crash on attempt to scroll. (Closed)
|
Created:
4 years, 6 months ago by khmel Modified:
4 years, 6 months ago CC:
chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, fsamuel, jam, nasko+codewatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix webview crash on attempt to scroll.
This fixes crash of webview when its content is scrolled using
2-fingers gesture via touchpad or using touchscreen.
BUG=615512
TEST=Manually on device using Arc++ OptIn UI, ToS is scrolled
as expected using touchscreen or touchpad, no crashes.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/d92694016e4cc9f7812894aa60eb4d36de6cd381
Cr-Commit-Position: refs/heads/master@{#397901}
Patch Set 1 #
Total comments: 4
Patch Set 2 : browser_tests fixed and extended #
Total comments: 2
Patch Set 3 : fixed content_tests (handle case when view is not available), added expected event into Fling test … #Messages
Total messages: 33 (15 generated)
Description was changed from
==========
Fix webview crash on attempt to scroll.
This fixes crash of webview when its content is scrolled using
2-fingers gesture via touchpad or using touchscreen.
BUG=615512
TEST=Manually on device using Arc++ OptIn UI, ToS is scrolled
as expected using touchscreen or touchpad, no crashes.
==========
to
==========
Fix webview crash on attempt to scroll.
This fixes crash of webview when its content is scrolled using
2-fingers gesture via touchpad or using touchscreen.
BUG=615512
TEST=Manually on device using Arc++ OptIn UI, ToS is scrolled
as expected using touchscreen or touchpad, no crashes.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========
Description was changed from
==========
Fix webview crash on attempt to scroll.
This fixes crash of webview when its content is scrolled using
2-fingers gesture via touchpad or using touchscreen.
BUG=615512
TEST=Manually on device using Arc++ OptIn UI, ToS is scrolled
as expected using touchscreen or touchpad, no crashes.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========
to
==========
Fix webview crash on attempt to scroll.
This fixes crash of webview when its content is scrolled using
2-fingers gesture via touchpad or using touchscreen.
BUG=615512
TEST=Manually on device using Arc++ OptIn UI, ToS is scrolled
as expected using touchscreen or touchpad, no crashes.
==========
khmel@chromium.org changed reviewers: + xiyuan@chromium.org
PTAL https://codereview.chromium.org/2015373002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/2015373002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_widget_host_view_guest.cc:77: UpdateScreenInfo(GetNativeView()); Direct reason of crash current_device_scale_factor_ = 0 (default value) from RenderWidgetHostViewBase. This field is updated on UpdateScreenInfo. When we handle gesture events, we temporary set scale device factor 1.0 and restore then to current_device_scale_factor_ value, which is 0. That is why scroll does not work. Update scale factor is here: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
xiyuan@chromium.org changed reviewers: + wjmaclean@chromium.org
The fix looks good to me. But I'll let the experts say. wjmaclean@, could you help with the review? fsamuel@, fyi but you are welcome to review too. :)
https://codereview.chromium.org/2015373002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/2015373002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_widget_host_view_guest.cc:77: UpdateScreenInfo(GetNativeView()); On 2016/05/27 20:49:26, khmel wrote: > Direct reason of crash current_device_scale_factor_ = 0 (default value) from > RenderWidgetHostViewBase. This field is updated on UpdateScreenInfo. When we > handle gesture events, we temporary set scale device factor 1.0 and restore then > to current_device_scale_factor_ value, which is 0. That is why scroll does not > work. > > Update scale factor is here: > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... > > > This looks fine, though I recall noting recently that we also don't properly set current_device_scale_factor_ for OOPIF WebView, i.e. when we use RenderWidgetHostViewChildFrame. Can this call be made instead in RWHVCF's constructor? If it can, that would prevent this problem from recurring when we transition to using just RWHVCF. I believe RWHVCF also has a unit test file somewhere, so we should add test coverage for this as well.
Description was changed from
==========
Fix webview crash on attempt to scroll.
This fixes crash of webview when its content is scrolled using
2-fingers gesture via touchpad or using touchscreen.
BUG=615512
TEST=Manually on device using Arc++ OptIn UI, ToS is scrolled
as expected using touchscreen or touchpad, no crashes.
==========
to
==========
Fix webview crash on attempt to scroll.
This fixes crash of webview when its content is scrolled using
2-fingers gesture via touchpad or using touchscreen.
BUG=615512
TEST=Manually on device using Arc++ OptIn UI, ToS is scrolled
as expected using touchscreen or touchpad, no crashes.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========
Hi, Thanks for you comments, PTAL https://codereview.chromium.org/2015373002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/2015373002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_widget_host_view_guest.cc:77: UpdateScreenInfo(GetNativeView()); On 2016/05/30 14:00:51, wjmaclean wrote: > On 2016/05/27 20:49:26, khmel wrote: > > Direct reason of crash current_device_scale_factor_ = 0 (default value) from > > RenderWidgetHostViewBase. This field is updated on UpdateScreenInfo. When we > > handle gesture events, we temporary set scale device factor 1.0 and restore > then > > to current_device_scale_factor_ value, which is 0. That is why scroll does not > > work. > > > > Update scale factor is here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... > > > > > > > > This looks fine, though I recall noting recently that we also don't properly set > current_device_scale_factor_ for OOPIF WebView, i.e. when we use > RenderWidgetHostViewChildFrame. Can this call be made instead in RWHVCF's > constructor? If it can, that would prevent this problem from recurring when we > transition to using just RWHVCF. > > I believe RWHVCF also has a unit test file somewhere, so we should add test > coverage for this as well. It is hard to put it to RWHVCF CTOR because RWHVCF has method gfx::NativeView RenderWidgetHostViewChildFrame::GetNativeView() const { NOTREACHED(); return nullptr; } Calling it from its CTORS goes to crash. I extended browser_tests to validate this. Without this fix new test fails. https://codereview.chromium.org/2015373002/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/platform_apps/web_view/scrollable_embedder_and_guest/guest.html (right): https://codereview.chromium.org/2015373002/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/platform_apps/web_view/scrollable_embedder_and_guest/guest.html:7: <html id='root' style="overflow-y: visible;"> Several tests use: EXPECT_EQ(gfx::Vector2dF(), guest_host_view->GetLastScrollOffset()); to validate that guest content is not scrolled. However if we have body overflow-y style, expression above is always true even if guest content is actually scrolled. I updated this test in order to be able to use guest_host_view->GetLastScrollOffset() in new test and make other tests valid according this check. https://codereview.chromium.org/2015373002/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/platform_apps/web_view/scrollable_embedder_and_guest/main.html (left): https://codereview.chromium.org/2015373002/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/platform_apps/web_view/scrollable_embedder_and_guest/main.html:9: "width: 400px; height: 400px; margin: 0px; padding: 0px; overflow-y: scroll"> Similar to my previous comment, overflow-y in body is redundant.
Hi, Thanks for you comments, PTAL https://codereview.chromium.org/2015373002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/2015373002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_widget_host_view_guest.cc:77: UpdateScreenInfo(GetNativeView()); On 2016/05/30 14:00:51, wjmaclean wrote: > On 2016/05/27 20:49:26, khmel wrote: > > Direct reason of crash current_device_scale_factor_ = 0 (default value) from > > RenderWidgetHostViewBase. This field is updated on UpdateScreenInfo. When we > > handle gesture events, we temporary set scale device factor 1.0 and restore > then > > to current_device_scale_factor_ value, which is 0. That is why scroll does not > > work. > > > > Update scale factor is here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... > > > > > > > > This looks fine, though I recall noting recently that we also don't properly set > current_device_scale_factor_ for OOPIF WebView, i.e. when we use > RenderWidgetHostViewChildFrame. Can this call be made instead in RWHVCF's > constructor? If it can, that would prevent this problem from recurring when we > transition to using just RWHVCF. > > I believe RWHVCF also has a unit test file somewhere, so we should add test > coverage for this as well. It is hard to put it to RWHVCF CTOR because RWHVCF has method gfx::NativeView RenderWidgetHostViewChildFrame::GetNativeView() const { NOTREACHED(); return nullptr; } Calling it from its CTORS goes to crash. I extended browser_tests to validate this. Without this fix new test fails. https://codereview.chromium.org/2015373002/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/platform_apps/web_view/scrollable_embedder_and_guest/guest.html (right): https://codereview.chromium.org/2015373002/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/platform_apps/web_view/scrollable_embedder_and_guest/guest.html:7: <html id='root' style="overflow-y: visible;"> Several tests use: EXPECT_EQ(gfx::Vector2dF(), guest_host_view->GetLastScrollOffset()); to validate that guest content is not scrolled. However if we have body overflow-y style, expression above is always true even if guest content is actually scrolled. I updated this test in order to be able to use guest_host_view->GetLastScrollOffset() in new test and make other tests valid according this check. https://codereview.chromium.org/2015373002/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/platform_apps/web_view/scrollable_embedder_and_guest/main.html (left): https://codereview.chromium.org/2015373002/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/platform_apps/web_view/scrollable_embedder_and_guest/main.html:9: "width: 400px; height: 400px; margin: 0px; padding: 0px; overflow-y: scroll"> Similar to my previous comment, overflow-y in body is redundant.
https://codereview.chromium.org/2015373002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/2015373002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_widget_host_view_guest.cc:77: UpdateScreenInfo(GetNativeView()); On 2016/06/02 00:10:22, khmel wrote: > On 2016/05/30 14:00:51, wjmaclean wrote: > > On 2016/05/27 20:49:26, khmel wrote: > > > Direct reason of crash current_device_scale_factor_ = 0 (default value) from > > > RenderWidgetHostViewBase. This field is updated on UpdateScreenInfo. When we > > > handle gesture events, we temporary set scale device factor 1.0 and restore > > then > > > to current_device_scale_factor_ value, which is 0. That is why scroll does > not > > > work. > > > > > > Update scale factor is here: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... > > > > > > > > > > > > > This looks fine, though I recall noting recently that we also don't properly > set > > current_device_scale_factor_ for OOPIF WebView, i.e. when we use > > RenderWidgetHostViewChildFrame. Can this call be made instead in RWHVCF's > > constructor? If it can, that would prevent this problem from recurring when we > > transition to using just RWHVCF. > > > > I believe RWHVCF also has a unit test file somewhere, so we should add test > > coverage for this as well. > > It is hard to put it to RWHVCF CTOR because RWHVCF has method > gfx::NativeView RenderWidgetHostViewChildFrame::GetNativeView() const { > NOTREACHED(); > return nullptr; > } > > Calling it from its CTORS goes to crash. > > I extended browser_tests to validate this. Without this fix new test fails. We're going to be removing the NOTREACHED() in RWHVCF::GetNativeView() soon, so perhaps we'll move this when that happens.
On 2016/06/02 12:17:52, wjmaclean wrote: > https://codereview.chromium.org/2015373002/diff/1/content/browser/frame_host/... > File content/browser/frame_host/render_widget_host_view_guest.cc (right): > > https://codereview.chromium.org/2015373002/diff/1/content/browser/frame_host/... > content/browser/frame_host/render_widget_host_view_guest.cc:77: > UpdateScreenInfo(GetNativeView()); > On 2016/06/02 00:10:22, khmel wrote: > > On 2016/05/30 14:00:51, wjmaclean wrote: > > > On 2016/05/27 20:49:26, khmel wrote: > > > > Direct reason of crash current_device_scale_factor_ = 0 (default value) > from > > > > RenderWidgetHostViewBase. This field is updated on UpdateScreenInfo. When > we > > > > handle gesture events, we temporary set scale device factor 1.0 and > restore > > > then > > > > to current_device_scale_factor_ value, which is 0. That is why scroll does > > not > > > > work. > > > > > > > > Update scale factor is here: > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... > > > > > > > > > > > > > > > > > > This looks fine, though I recall noting recently that we also don't properly > > set > > > current_device_scale_factor_ for OOPIF WebView, i.e. when we use > > > RenderWidgetHostViewChildFrame. Can this call be made instead in RWHVCF's > > > constructor? If it can, that would prevent this problem from recurring when > we > > > transition to using just RWHVCF. > > > > > > I believe RWHVCF also has a unit test file somewhere, so we should add test > > > coverage for this as well. > > > > It is hard to put it to RWHVCF CTOR because RWHVCF has method > > gfx::NativeView RenderWidgetHostViewChildFrame::GetNativeView() const { > > NOTREACHED(); > > return nullptr; > > } > > > > Calling it from its CTORS goes to crash. > > > > I extended browser_tests to validate this. Without this fix new test fails. > > We're going to be removing the NOTREACHED() in RWHVCF::GetNativeView() soon, so > perhaps we'll move this when that happens. LGTM in the meantime.
khmel@chromium.org changed reviewers: + sky@chromium.org
Thank you James and Xiyuan for review. Hi Scott, PTAL
khmel@chromium.org changed reviewers: + jochen@chromium.org
Adding jochen@ as he has right to approve all left file, PTAL
On 2016/06/02 17:18:47, khmel wrote: > Adding jochen@ as he has right to approve all left file, > > PTAL Can you clarify if you still need me to review, and if so which files?
On 2016/06/02 18:03:50, sky wrote: > On 2016/06/02 17:18:47, khmel wrote: > > Adding jochen@ as he has right to approve all left file, > > > > PTAL > > Can you clarify if you still need me to review, and if so which files? Hi, I need approval for content/browser/frame_host/render_widget_host_view_guest.cc content/public/test/browser_test_utils.cc content/public/test/browser_test_utils.h You may approve content/public/test/browser_test_utils* I found that jochen@ may approve all pending files from the list. So, you may skip this review I would appreciate jochen@ review.
Description was changed from
==========
Fix webview crash on attempt to scroll.
This fixes crash of webview when its content is scrolled using
2-fingers gesture via touchpad or using touchscreen.
BUG=615512
TEST=Manually on device using Arc++ OptIn UI, ToS is scrolled
as expected using touchscreen or touchpad, no crashes.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========
to
==========
Fix webview crash on attempt to scroll.
This fixes crash of webview when its content is scrolled using
2-fingers gesture via touchpad or using touchscreen.
BUG=615512
TEST=Manually on device using Arc++ OptIn UI, ToS is scrolled
as expected using touchscreen or touchpad, no crashes.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========
sky@chromium.org changed reviewers: - sky@chromium.org
I'm removing myself as a reviewer then.
lgtm
The CQ bit was checked by khmel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2015373002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2015373002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by khmel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wjmaclean@chromium.org, jochen@chromium.org Link to the patchset: https://codereview.chromium.org/2015373002/#ps40001 (title: "fixed content_tests (handle case when view is not available), added expected event into Fling test …")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2015373002/40001
Message was sent while issue was closed.
Description was changed from
==========
Fix webview crash on attempt to scroll.
This fixes crash of webview when its content is scrolled using
2-fingers gesture via touchpad or using touchscreen.
BUG=615512
TEST=Manually on device using Arc++ OptIn UI, ToS is scrolled
as expected using touchscreen or touchpad, no crashes.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========
to
==========
Fix webview crash on attempt to scroll.
This fixes crash of webview when its content is scrolled using
2-fingers gesture via touchpad or using touchscreen.
BUG=615512
TEST=Manually on device using Arc++ OptIn UI, ToS is scrolled
as expected using touchscreen or touchpad, no crashes.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from
==========
Fix webview crash on attempt to scroll.
This fixes crash of webview when its content is scrolled using
2-fingers gesture via touchpad or using touchscreen.
BUG=615512
TEST=Manually on device using Arc++ OptIn UI, ToS is scrolled
as expected using touchscreen or touchpad, no crashes.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========
to
==========
Fix webview crash on attempt to scroll.
This fixes crash of webview when its content is scrolled using
2-fingers gesture via touchpad or using touchscreen.
BUG=615512
TEST=Manually on device using Arc++ OptIn UI, ToS is scrolled
as expected using touchscreen or touchpad, no crashes.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/d92694016e4cc9f7812894aa60eb4d36de6cd381
Cr-Commit-Position: refs/heads/master@{#397901}
==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/d92694016e4cc9f7812894aa60eb4d36de6cd381 Cr-Commit-Position: refs/heads/master@{#397901} |
