restricts when context could be retrieved.

chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc

Index: chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc
diff --git a/chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc b/chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc
index b9122323dee0974db5e0cd8f45d50cf2675c14df..8d4fb752d7d2cd21eef60ddc8afc5858023ccc2f 100644
--- a/chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc
+++ b/chrome/browser/chromeos/arc/voice_interaction/arc_voice_interaction_framework_service.cc
@@ -11,10 +11,12 @@
 #include "ash/public/cpp/shell_window_ids.h"
 #include "ash/shell.h"
 #include "base/bind.h"
+#include "base/callback.h"
 #include "base/command_line.h"
 #include "base/containers/flat_set.h"
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/metrics/user_metrics.h"
 #include "base/metrics/user_metrics_action.h"
 #include "base/task_scheduler/post_task.h"
@@ -29,6 +31,7 @@
 #include "ui/compositor/layer.h"
 #include "ui/compositor/layer_owner.h"
 #include "ui/compositor/layer_tree_owner.h"
+#include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/image/image.h"
 #include "ui/gfx/image/image_util.h"
 #include "ui/gfx/native_widget_types.h"
@@ -41,6 +44,16 @@ namespace arc {
 
 namespace {
 
+// 2s time out for a context query from container since user initiated
+// interaction. This must be strictly less than
+// kMaxTimeMsSinceUserInteractionForHistogram.

[Ilya Sherman, 2017/05/25 22:21:16]

Why is it important for this value to be strictly less than the max value for
the histogram?  Values above the max will be recorded to an overflow bucket.

[Muyuan, 2017/05/25 22:32:58]

It's the allowed time for interaction realistically, meaning any time above this
one will be considered to be blocked, which in theory should be a lot smaller
than the upper bound for histogram.

[Ilya Sherman, 2017/05/25 22:37:17]

Please explain this inline within the comments.  (It's actually still not 100%
clear to me why you need this less-than relationship.  Is the idea that you want
to validate the choice by looking at the real-world data recorded in the
histogram?)

+constexpr base::TimeDelta kAllowedTimeMsSinceUserInteraction =

[Luis Héctor Chávez, 2017/05/25 22:32:50]

nit: kAllowedTimeSinceUserInteraction

same below, drop the "Ms"

[Muyuan, 2017/05/25 23:01:26]

Done.

+    base::TimeDelta::FromSeconds(2);
+constexpr base::TimeDelta kMaxTimeMsSinceUserInteractionForHistogram =
+    base::TimeDelta::FromSeconds(5);
+
+const int32_t kContextRequestMaxRemainingCount = 2;

[Luis Héctor Chávez, 2017/05/25 22:32:50]

nit: constexpr

[Muyuan, 2017/05/25 23:01:26]

Done.

+
 void ScreenshotCallback(
     const mojom::VoiceInteractionFrameworkHost::CaptureFocusedWindowCallback&
         callback,
@@ -204,12 +217,7 @@ bool ArcVoiceInteractionFrameworkService::AcceleratorPressed(
     DCHECK(framework_instance);
     framework_instance->SetMetalayerVisibility(true);
   } else {
-    mojom::VoiceInteractionFrameworkInstance* framework_instance =
-        ARC_GET_INSTANCE_FOR_METHOD(
-            arc_bridge_service()->voice_interaction_framework(),
-            StartVoiceInteractionSession);
-    DCHECK(framework_instance);
-    framework_instance->StartVoiceInteractionSession();
+    StartSessionFromUserInteraction(gfx::Rect());
   }
 
   return true;
@@ -223,6 +231,12 @@ bool ArcVoiceInteractionFrameworkService::CanHandleAccelerators() const {
 void ArcVoiceInteractionFrameworkService::CaptureFocusedWindow(
     const CaptureFocusedWindowCallback& callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  if (!ValidateTimeSinceUserInteraction()) {
+    callback.Run(std::vector<uint8_t>{});
+    return;
+  }
+
   aura::Window* window =
       ash::Shell::Get()->activation_client()->GetActiveWindow();
 
@@ -240,6 +254,12 @@ void ArcVoiceInteractionFrameworkService::CaptureFocusedWindow(
 void ArcVoiceInteractionFrameworkService::CaptureFullscreen(
     const CaptureFullscreenCallback& callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  if (!ValidateTimeSinceUserInteraction()) {
+    callback.Run(std::vector<uint8_t>{});
+    return;
+  }
+
   // Since ARC currently only runs in primary display, we restrict
   // the screenshot to it.
   aura::Window* window = ash::Shell::GetPrimaryRootWindow();
@@ -277,7 +297,15 @@ void ArcVoiceInteractionFrameworkService::ShowMetalayer(
     LOG(ERROR) << "Metalayer is already enabled";
     return;
   }
-  metalayer_closed_callback_ = closed;
+  metalayer_closed_callback_ = base::Bind(
+      [](const base::Callback<bool()>& init, const base::Closure& closed) {
+        // Initiate user interaction at the point meta layer disappears.

[Luis Héctor Chávez, 2017/05/25 22:32:50]

nit: Initiate user interaction when the metalayer disappears.

[Muyuan, 2017/05/25 23:01:26]

Done.

+        if (init.Run())
+          closed.Run();
+      },
+      base::Bind(&ArcVoiceInteractionFrameworkService::InitiateUserInteraction,
+                 base::Unretained(this)),

[Luis Héctor Chávez, 2017/05/25 22:32:50]

Please comment why base::Unretained is safe to use.

[Muyuan, 2017/05/25 23:01:26]

Done.

+      closed);
   SetMetalayerVisibility(true);
 }
 
@@ -305,4 +333,72 @@ void ArcVoiceInteractionFrameworkService::SetMetalayerVisibility(bool visible) {
   framework_instance->SetMetalayerVisibility(visible);
 }
 
+void ArcVoiceInteractionFrameworkService::StartSessionFromUserInteraction(
+    const gfx::Rect& rect) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  if (!InitiateUserInteraction())
+    return;
+
+  if (rect.IsEmpty()) {
+    mojom::VoiceInteractionFrameworkInstance* framework_instance =
+        ARC_GET_INSTANCE_FOR_METHOD(
+            arc_bridge_service()->voice_interaction_framework(),
+            StartVoiceInteractionSession);
+    DCHECK(framework_instance);
+    framework_instance->StartVoiceInteractionSession();
+  } else {
+    mojom::VoiceInteractionFrameworkInstance* framework_instance =
+        ARC_GET_INSTANCE_FOR_METHOD(
+            arc_bridge_service()->voice_interaction_framework(),
+            StartVoiceInteractionSessionForRegion);
+    DCHECK(framework_instance);
+    framework_instance->StartVoiceInteractionSessionForRegion(rect);
+  }
+}
+
+bool ArcVoiceInteractionFrameworkService::ValidateTimeSinceUserInteraction() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  if (!context_request_remaining_count_) {
+    // Allowed number of requests used up. But we still have additional
+    // requests. It's likely that there is something malicious going on.
+    LOG(ERROR) << "Illegal context request from container.";
+    UMA_HISTOGRAM_BOOLEAN("VoiceInteraction.IllegalContextRequest", true);
+    return false;
+  }
+  auto elapsed = base::TimeTicks::Now() - user_interaction_start_time_;
+  elapsed = elapsed > kMaxTimeMsSinceUserInteractionForHistogram
+                ? kMaxTimeMsSinceUserInteractionForHistogram
+                : elapsed;
+
+  UMA_HISTOGRAM_CUSTOM_COUNTS(
+      "VoiceInteraction.RequestArrivalSinceUserInteraction",
+      elapsed.InMilliseconds(), 0,

[Ilya Sherman, 2017/05/25 22:21:16]

nit: 0 is not a valid minimum bucket, and you should see a DCHECK firing for
this.  If you do not build with DCHECKs enabled, I'd strongly encourage you to
do so.  Please specify 1 instead.  (Data can still go into the "0" bucket; it's
just that that bucket is used for underflow, and the API expects it not to be
specified explicitly.)

[Muyuan, 2017/05/25 22:32:58]

Done.

+      kMaxTimeMsSinceUserInteractionForHistogram.InMilliseconds(), 20);
+
+  if (elapsed > kAllowedTimeMsSinceUserInteraction) {
+    LOG(ERROR) << "Timed out since last user interaction.";
+    context_request_remaining_count_ = 0;
+    return false;
+  }
+
+  context_request_remaining_count_--;
+  return true;
+}
+
+bool ArcVoiceInteractionFrameworkService::InitiateUserInteraction() {
+  auto start_time = base::TimeTicks::Now();
+  if ((start_time - user_interaction_start_time_) <
+          kAllowedTimeMsSinceUserInteraction &&
+      context_request_remaining_count_) {
+    // If next request starts too soon and there is a session in action, we

[Luis Héctor Chávez, 2017/05/25 22:32:50]

nit: s/a session in action/an active session/

[Muyuan, 2017/05/25 23:01:26]

Done.

+    // should drop it.
+    return false;
+  }
+  user_interaction_start_time_ = start_time;
+  context_request_remaining_count_ = kContextRequestMaxRemainingCount;
+  return true;
+}
+
 }  // namespace arc