Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1545683002:
Use WeakPtr to RenderWidgetHostViewBase in InputEventRouter. (Closed)
DescriptionUse WeakPtr to RenderWidgetHostViewBase in InputEventRouter.
Since it is difficult to ensure when nested WebContents (a top-level
WebContents and one or more guests) may be destructed, it is also
difficult to guarantee that a RenderWidgetHostViewBase-derivative will
be always able to properly remove itself from any RenderWidgetHost-
InputEventRouter it may be registered with. This may lead to the latter
having an owner_map_ with stale pointers in it, leading to crashes.
This CL converts the owner_map_ to store WeakPtrs to RenderWidgetHost-
ViewBase, which can be tested before use. It's worth noting that
RenderWidgetHostViewBase already provides support for WeakPtrs.
BUG=570646
Committed: https://crrev.com/5712088e9b663455cf67b60825ee36cdbdd024dc
Cr-Commit-Position: refs/heads/master@{#366652}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Address suggestions. #
Depends on Patchset: Messages
Total messages: 19 (9 generated)
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1545683002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1545683002/1
Description was changed from ========== Use WeakPtr to RenderWidgetHostViewBase in InputEventRouter. Since it is difficult to ensure when nested WebContents (a top-level WebContents and one or more guests) may be destructed, it is also difficult to guarantee that a RenderWidgetHostViewBase-derivative will be always able to properly remove itself from any RenderWidgetHost- InputEventRouter it may be registered with. This may lead to the latter having an owner_map_ with stale pointers in it, leading to crashes. This CL converts the owner_map_ to store WeakPtrs to RenderWidgetHost- ViewBase, which can be tested before use. It's worth noting that RenderWidgetHostViewBase already provides support for WeakPtrs. BUG=570646 ========== to ========== Use WeakPtr to RenderWidgetHostViewBase in InputEventRouter. Since it is difficult to ensure when nested WebContents (a top-level WebContents and one or more guests) may be destructed, it is also difficult to guarantee that a RenderWidgetHostViewBase-derivative will be always able to properly remove itself from any RenderWidgetHost- InputEventRouter it may be registered with. This may lead to the latter having an owner_map_ with stale pointers in it, leading to crashes. This CL converts the owner_map_ to store WeakPtrs to RenderWidgetHost- ViewBase, which can be tested before use. It's worth noting that RenderWidgetHostViewBase already provides support for WeakPtrs. BUG=570646 ==========
wjmaclean@chromium.org changed reviewers: + kenrb@chromium.org, nasko@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL This replaces https://codereview.chromium.org/1541913002/
wjmaclean@chromium.org changed reviewers: + fsamuel@chromium.org - kenrb@chromium.org
Seems reasonable. lgtm. Alternatively a RenderWidgetHostViewObserver could be implemented and have the InputEventRouter observe all RenderWidgetHostViews added. That would avoid the need to use weak pointers I think. Here we could also end up stuck with these pointers forever in the map (I think?), which doesn't seem ideal. https://codereview.chromium.org/1545683002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/1545683002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_input_event_router.cc:89: has_current_touch_target_ = true; Is this flag necessary? It seems you only use it for DCHECKing?
Some simple comments. https://codereview.chromium.org/1545683002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/1545683002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_input_event_router.cc:20: RenderWidgetHostInputEventRouter::WeakTarget Why does this method need to return the WeakTarget type? Can't it return a the pointer type, since it needs to be checked by the callers anyway. https://codereview.chromium.org/1545683002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_input_event_router.cc:57: if (target) Let'd use early returns - if (!target) return, right after the actual call to FindEventTarget. https://codereview.chromium.org/1545683002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_input_event_router.cc:70: if (target) Same as above, early return. https://codereview.chromium.org/1545683002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_input_event_router.cc:88: FindEventTarget(root_view, original_point, &transformed_point); You could just call ->GetWeakPtr() here, since it is the only place you really need a weak pointer.
The observer idea is a good one, though this is a speculative fix to make sure we've correctly identified the underlying problem. If this works, we can re-write it using an observer. In the short term, if you're concerned about stale pointers piling up, we could always evict any we find from the map ... Revised as per comments, PTAL. https://codereview.chromium.org/1545683002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_input_event_router.cc (right): https://codereview.chromium.org/1545683002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_input_event_router.cc:20: RenderWidgetHostInputEventRouter::WeakTarget On 2015/12/22 18:16:33, nasko wrote: > Why does this method need to return the WeakTarget type? Can't it return a the > pointer type, since it needs to be checked by the callers anyway. It doesn't have to return the WeakPtr ... I'll change this. https://codereview.chromium.org/1545683002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_input_event_router.cc:57: if (target) On 2015/12/22 18:16:33, nasko wrote: > Let'd use early returns - if (!target) return, right after the actual call to > FindEventTarget. Done. https://codereview.chromium.org/1545683002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_input_event_router.cc:70: if (target) On 2015/12/22 18:16:33, nasko wrote: > Same as above, early return. Done. https://codereview.chromium.org/1545683002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_input_event_router.cc:88: FindEventTarget(root_view, original_point, &transformed_point); On 2015/12/22 18:16:33, nasko wrote: > You could just call ->GetWeakPtr() here, since it is the only place you really > need a weak pointer. I don't understand? You mean call it on current_touch_target? or root_view? The former could be stale (we'll only know if we store the WeakPtr in the map). The latter is always a value provided by the caller, so we trust it not to be stale.
LGTM
The CQ bit was checked by wjmaclean@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fsamuel@chromium.org Link to the patchset: https://codereview.chromium.org/1545683002/#ps20001 (title: "Address suggestions.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1545683002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1545683002/20001
Message was sent while issue was closed.
Description was changed from ========== Use WeakPtr to RenderWidgetHostViewBase in InputEventRouter. Since it is difficult to ensure when nested WebContents (a top-level WebContents and one or more guests) may be destructed, it is also difficult to guarantee that a RenderWidgetHostViewBase-derivative will be always able to properly remove itself from any RenderWidgetHost- InputEventRouter it may be registered with. This may lead to the latter having an owner_map_ with stale pointers in it, leading to crashes. This CL converts the owner_map_ to store WeakPtrs to RenderWidgetHost- ViewBase, which can be tested before use. It's worth noting that RenderWidgetHostViewBase already provides support for WeakPtrs. BUG=570646 ========== to ========== Use WeakPtr to RenderWidgetHostViewBase in InputEventRouter. Since it is difficult to ensure when nested WebContents (a top-level WebContents and one or more guests) may be destructed, it is also difficult to guarantee that a RenderWidgetHostViewBase-derivative will be always able to properly remove itself from any RenderWidgetHost- InputEventRouter it may be registered with. This may lead to the latter having an owner_map_ with stale pointers in it, leading to crashes. This CL converts the owner_map_ to store WeakPtrs to RenderWidgetHost- ViewBase, which can be tested before use. It's worth noting that RenderWidgetHostViewBase already provides support for WeakPtrs. BUG=570646 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Use WeakPtr to RenderWidgetHostViewBase in InputEventRouter. Since it is difficult to ensure when nested WebContents (a top-level WebContents and one or more guests) may be destructed, it is also difficult to guarantee that a RenderWidgetHostViewBase-derivative will be always able to properly remove itself from any RenderWidgetHost- InputEventRouter it may be registered with. This may lead to the latter having an owner_map_ with stale pointers in it, leading to crashes. This CL converts the owner_map_ to store WeakPtrs to RenderWidgetHost- ViewBase, which can be tested before use. It's worth noting that RenderWidgetHostViewBase already provides support for WeakPtrs. BUG=570646 ========== to ========== Use WeakPtr to RenderWidgetHostViewBase in InputEventRouter. Since it is difficult to ensure when nested WebContents (a top-level WebContents and one or more guests) may be destructed, it is also difficult to guarantee that a RenderWidgetHostViewBase-derivative will be always able to properly remove itself from any RenderWidgetHost- InputEventRouter it may be registered with. This may lead to the latter having an owner_map_ with stale pointers in it, leading to crashes. This CL converts the owner_map_ to store WeakPtrs to RenderWidgetHost- ViewBase, which can be tested before use. It's worth noting that RenderWidgetHostViewBase already provides support for WeakPtrs. BUG=570646 Committed: https://crrev.com/5712088e9b663455cf67b60825ee36cdbdd024dc Cr-Commit-Position: refs/heads/master@{#366652} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/5712088e9b663455cf67b60825ee36cdbdd024dc Cr-Commit-Position: refs/heads/master@{#366652} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
