Drop the event when the target window has been deleted.
In the viz-hit-test case, when we cannot find the target window in
EventTargeter, we were using NOTREACHED before this CL. But there are
cases where the target window has simply been deleted by the time this
event is processed, e.g. clicking the settings button inside system
tray's network page, mouse move events after that but before the system
tray goes away would fail. In these cases, it makes sense to just drop
the event like we did in non-viz hit-test. Kept the TODO to investigate
if it can be a security fault when no target window is found.
Also fixes a memory exception in HTQ when using the transform directly.
Bug: 752380
Change-Id: Ifa099c88a98d623b8be363fe3a77b7bc8eea4a54
Reviewed-on: https://chromium-review.googlesource.com/651564
Commit-Queue: Ria Jiang <riajiang@chromium.org>
Reviewed-by: Robert Kroeger <rjkroege@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501636}diff --git a/components/viz/service/hit_test/hit_test_aggregator.cc b/components/viz/service/hit_test/hit_test_aggregator.cc
index 3039072..fce4996 100644
--- a/components/viz/service/hit_test/hit_test_aggregator.cc
+++ b/components/viz/service/hit_test/hit_test_aggregator.cc
@@ -6,6 +6,7 @@
#include "components/viz/common/hit_test/aggregated_hit_test_region.h"
#include "components/viz/service/hit_test/hit_test_aggregator_delegate.h"
+#include "third_party/skia/include/core/SkMatrix44.h"
namespace viz {
@@ -38,6 +39,17 @@
return true;
}
+void PrepareTransformForReadOnlySharedMemory(gfx::Transform* transform) {
+ // |transform| is going to be shared in read-only memory to HitTestQuery.
+ // However, if HitTestQuery tries to operate on it, then it is possible that
+ // it will attempt to perform write on the underlying SkMatrix44 [1], causing
+ // invalid memory write in read-only memory.
+ // [1]
+ // https://cs.chromium.org/chromium/src/third_party/skia/include/core/SkMatrix44.h?l=133
+ // Explicitly calling getType() to compute the type-mask in SkMatrix44.
+ transform->matrix().getType();
+}
+
} // namespace
HitTestAggregator::HitTestAggregator(HitTestAggregatorDelegate* delegate)
@@ -176,6 +188,7 @@
regions[0].flags = hit_test_region_list->flags;
regions[0].rect = hit_test_region_list->bounds;
regions[0].transform = hit_test_region_list->transform;
+ PrepareTransformForReadOnlySharedMemory(®ions[0].transform);
size_t region_index = 1;
for (const auto& region : hit_test_region_list->regions) {
@@ -230,6 +243,7 @@
break;
}
}
+ PrepareTransformForReadOnlySharedMemory(&element->transform);
DCHECK_GE(region_index - parent_index - 1, 0u);
element->child_count = region_index - parent_index - 1;
return region_index;
diff --git a/services/ui/ws/event_targeter.cc b/services/ui/ws/event_targeter.cc
index ddd1353..54bea53 100644
--- a/services/ui/ws/event_targeter.cc
+++ b/services/ui/ws/event_targeter.cc
@@ -6,6 +6,7 @@
#include "base/command_line.h"
#include "base/memory/ptr_util.h"
+#include "base/metrics/user_metrics.h"
#include "base/task_scheduler/post_task.h"
#include "base/threading/thread_task_runner_handle.h"
#include "components/viz/host/hit_test/hit_test_query.h"
@@ -65,9 +66,10 @@
event_targeter_delegate_->GetWindowFromFrameSinkId(
target.frame_sink_id);
if (!target_window) {
- // TODO(riajiang): There's no target window with this frame_sink_id,
- // maybe a security fault. http://crbug.com/746470
- NOTREACHED();
+ // TODO(riajiang): Investigate when this would be a security fault.
+ // http://crbug.com/746470
+ base::RecordAction(
+ base::UserMetricsAction("EventTargeting_DeletedTarget"));
}
deepest_window.window = target_window;
// TODO(riajiang): use |target.location_in_target|.
diff --git a/tools/metrics/actions/actions.xml b/tools/metrics/actions/actions.xml
index 63c89e4..1d377314 100644
--- a/tools/metrics/actions/actions.xml
+++ b/tools/metrics/actions/actions.xml
@@ -4600,6 +4600,15 @@
<description>Please enter the description of this user action.</description>
</action>
+<action name="EventTargeting_DeletedTarget">
+ <owner>riajiang@chromium.org</owner>
+ <owner>rjkroege@chromium.org</owner>
+ <description>
+ Events for a target window are processed after that target window has been
+ deleted.
+ </description>
+</action>
+
<action name="Exit">
<owner>Please list the metric's owners. Add more owner tags as needed.</owner>
<description>Please enter the description of this user action.</description>
diff --git a/ui/gfx/transform.h b/ui/gfx/transform.h
index 0964f74..0ee622e 100644
--- a/ui/gfx/transform.h
+++ b/ui/gfx/transform.h
@@ -122,6 +122,7 @@
void ConcatTransform(const Transform& transform);
// Returns true if this is the identity matrix.
+ // This function modifies a mutable variable in |matrix_|.
bool IsIdentity() const { return matrix_.isIdentity(); }
// Returns true if the matrix is either identity or pure translation.
