Add a new app API to enable watchdog behavior restarts in kiosk apps

extensions/browser/api/runtime/runtime_api.cc

Index: extensions/browser/api/runtime/runtime_api.cc
diff --git a/extensions/browser/api/runtime/runtime_api.cc b/extensions/browser/api/runtime/runtime_api.cc
index e62805e674289632f33a1786b7a89eac8b06d478..34ff90f21ebfc41b4b476b8a14b893729886b3a5 100644
--- a/extensions/browser/api/runtime/runtime_api.cc
+++ b/extensions/browser/api/runtime/runtime_api.cc
@@ -10,6 +10,7 @@
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/values.h"
 #include "base/version.h"
 #include "content/public/browser/browser_context.h"
@@ -28,6 +29,7 @@
 #include "extensions/browser/lazy_background_task_queue.h"
 #include "extensions/browser/notification_types.h"
 #include "extensions/browser/process_manager_factory.h"
+#include "extensions/browser/state_store.h"
 #include "extensions/common/api/runtime.h"
 #include "extensions/common/error_utils.h"
 #include "extensions/common/extension.h"
@@ -44,37 +46,59 @@ namespace runtime = api::runtime;
 
 namespace {
 
-const char kNoBackgroundPageError[] = "You do not have a background page.";
-const char kPageLoadError[] = "Background page failed to load.";
-const char kFailedToCreateOptionsPage[] = "Could not create an options page.";
-const char kInstallId[] = "id";
-const char kInstallReason[] = "reason";
-const char kInstallReasonChromeUpdate[] = "chrome_update";
-const char kInstallReasonUpdate[] = "update";
-const char kInstallReasonInstall[] = "install";
-const char kInstallReasonSharedModuleUpdate[] = "shared_module_update";
-const char kInstallPreviousVersion[] = "previousVersion";
-const char kInvalidUrlError[] = "Invalid URL: \"*\".";
-const char kPlatformInfoUnavailable[] = "Platform information unavailable.";
-
-const char kUpdatesDisabledError[] = "Autoupdate is not enabled.";
+constexpr char kNoBackgroundPageError[] = "You do not have a background page.";

[Devlin, 2016/05/20 17:26:32]

Is constexpr preferred by Chromium style now?

[afakhry, 2016/05/21 01:13:00]

Yes. Please, see: http://chromium-cpp.appspot.com/#core-whitelist

[Devlin, 2016/05/23 16:49:39]

From there "Don't go out of the way to convert existing code."  So I'd say make
the new ones constexpr, but leave these.

[afakhry, 2016/05/25 01:55:46]

Done.

+constexpr char kPageLoadError[] = "Background page failed to load.";
+constexpr char kFailedToCreateOptionsPage[] =
+    "Could not create an options page.";
+constexpr char kInstallId[] = "id";
+constexpr char kInstallReason[] = "reason";
+constexpr char kInstallReasonChromeUpdate[] = "chrome_update";
+constexpr char kInstallReasonUpdate[] = "update";
+constexpr char kInstallReasonInstall[] = "install";
+constexpr char kInstallReasonSharedModuleUpdate[] = "shared_module_update";
+constexpr char kInstallPreviousVersion[] = "previousVersion";
+constexpr char kInvalidUrlError[] = "Invalid URL: \"*\".";
+constexpr char kPlatformInfoUnavailable[] = "Platform information unavailable.";
+
+constexpr char kUpdatesDisabledError[] = "Autoupdate is not enabled.";
 
 // A preference key storing the url loaded when an extension is uninstalled.
-const char kUninstallUrl[] = "uninstall_url";
+constexpr char kUninstallUrl[] = "uninstall_url";
 
 // A preference key storing the information about an extension that was
 // installed but not loaded. We keep the pending info here so that we can send
 // chrome.runtime.onInstalled event during the extension load.
-const char kPrefPendingOnInstalledEventDispatchInfo[] =
+constexpr char kPrefPendingOnInstalledEventDispatchInfo[] =
     "pending_on_installed_event_dispatch_info";
 
 // Previously installed version number.
-const char kPrefPreviousVersion[] = "previous_version";
+constexpr char kPrefPreviousVersion[] = "previous_version";
 
 // The name of the directory to be returned by getPackageDirectoryEntry. This
 // particular value does not matter to user code, but is chosen for consistency
 // with the equivalent Pepper API.
-const char kPackageDirectoryPath[] = "crxfs";
+constexpr char kPackageDirectoryPath[] = "crxfs";
+
+// Preference key for storing the last successful restart on watchdog requests.
+constexpr char kPrefRestartOnWatchdogTime[] = "last_restart_on_watchdog_time";
+
+// Error and status messages strings for the restartOnWatchdog() API.
+constexpr char kErrorInvalidArgument[] = "Invalid argument: *.";
+constexpr char kErrorOnlyKioskModeAllowed[] =
+    "API available only for ChromeOS kiosk mode.";
+constexpr char kErrorOnlyFirstExtensionAllowed[] =
+    "Not the first extension to call this API.";
+constexpr char kErrorInvalidStatus[] = "Invalid restart request status.";
+constexpr char kMessageRestartRequestCanceled[] =
+    "Restart request was cancelled.";
+constexpr char kMessageNoMoreRestartsScheduled[] =
+    "No more restarts are scheduled.";
+
+constexpr int kMinDurationBetweenSuccessiveRestartsHours = 3;
+
+// This is used for browsertests, so that we can test the restartOnWatchdog
+// API without a kiost app.
+bool allow_non_kiost_apps_restart_api_for_test = false;
 
 void DispatchOnStartupEventImpl(BrowserContext* browser_context,
                                 const std::string& extension_id,
@@ -156,7 +180,10 @@ RuntimeAPI::RuntimeAPI(content::BrowserContext* context)
     : browser_context_(context),
       dispatch_chrome_updated_event_(false),
       extension_registry_observer_(this),
-      process_manager_observer_(this) {
+      process_manager_observer_(this),
+      minimum_duration_between_restarts_(base::TimeDelta::FromHours(
+          kMinDurationBetweenSuccessiveRestartsHours)),
+      weak_ptr_factory_(this) {
   // RuntimeAPI is redirected in incognito, so |browser_context_| is never
   // incognito.
   DCHECK(!browser_context_->IsOffTheRecord());
@@ -321,10 +348,130 @@ bool RuntimeAPI::RestartDevice(std::string* error_message) {
   return delegate_->RestartDevice(error_message);
 }
 
+RuntimeAPI::RestartOnWatchdogStatus RuntimeAPI::RestartDeviceOnWatchdogTimeout(
+    const std::string& extension_id,
+    int seconds_from_now,
+    const OnWatchdogTimeoutCallback& callback) {
+  // To achieve as much accuracy as possible, record the time of the call as

[Devlin, 2016/05/20 17:26:32]

If we have a min time of 3 hours, does "as much accuracy as possible" really
matter?

[afakhry, 2016/05/21 01:13:00]

The minimum time is for any two successive restarts resulting from this API;
i.e. when an app schedules a restart after 2 seconds from now, and succeeds, the
following restart that this app can schedule after restart is 3 hours later.
Nothing prevents the app from calling this API once every 1 second for example.

The accuracy here really matters in our browsertests, since we're talking about
a couple of seconds. :D

+  // |now| here.
+  const base::Time now = base::Time::NowFromSystemTime();
+
+  if (schedule_restart_first_extension_id_.empty()) {
+    schedule_restart_first_extension_id_ = extension_id;
+  } else if (extension_id != schedule_restart_first_extension_id_) {
+    // We only allow the first extension to call this API to call it repeatedly.
+    // Any other extension will fail.
+    return RestartOnWatchdogStatus::FAILED_NOT_FIRST_EXTENSION;
+  }
+
+  if (seconds_from_now == -1) {
+    MaybeCancelRunningWatchdogTimer();
+    return RestartOnWatchdogStatus::SUCCESS_RESTART_CANCELLED;
+  }
+
+  // Try to read any previously recorded restart request time.
+  StateStore* storage = ExtensionSystem::Get(browser_context_)->state_store();
+  if (storage) {
+    storage->GetExtensionValue(

[Devlin, 2016/05/20 17:26:32]

The fact that we need to store this value implies to me that this is supposed to
carry across device power ons/offs - is that correct?  If so, don't we need to
read it at startup somewhere?  Also, should we make sure that we don't restart
right after turning on?  If it's not supposed to persist, could we just have
this as a property of the runtime api?

[afakhry, 2016/05/21 01:13:00]

Yes, it is supposed to persist accross device power ons/off.

I don't think restarting after first turning on matters. From what I understand
about the purpose of this API is that it's made for kiosk apps so that they can
call it repeatedly as a way to tell Chrome OS that the app is functioning and
behaving properly. 

Let's say we have a device that was just powered on. We have on it a kiosk app
that starts running. The app developer decided to call this API every 10 seconds
(for instance). It calls it the first time. few seconds later, it hangs forever.
It will miss the next API call. Our API will recognize this as an app failure
and will restart the device, hopefully so that the app can be updated
automatically. After restart however, this app will not be able to schedule any
restarts sooner than 3 hours, even if it hangs again after say 15 seconds from
the beginning.

Little bit of history: I was told that we're adding this API to avoid a problem
that took place with a kiosk app that caused the whole system to enter a bad
state to point, someone had to be sent to every single device running that app
to restart it manually. (I'm sorry, I don't have the exact details).

We can raise this issue on the bug thread as well, and ask for clarification
regarding the expected behavior.

[Devlin, 2016/05/23 16:49:39]

I think I understand the point of it, my question was why we're storing it. 
From my understood use cases (which align with what you described), there's no
reason to store it persistently rather than just as a property on the API.  Am I
missing something?

[xiyuan, 2016/05/24 16:47:05]

The minimum time 3 hours between API reboots means if last reboot time is T, the
next reboot time must be >= T + 3 hours. I think we would have to persist T
since the device would be rebooted and lose all in memory states between the two
runs. Right?

[Devlin, 2016/05/24 16:51:46]

Right, but I think we should store that when/if we restart the device, not when
we schedule the restart, since the latter could be common, while the former
should be quite rare.

[xiyuan, 2016/05/24 16:56:04]

The code here is to read the persist value to calculate the time for scheduling
the reboot. The SetExtensionValue call is around line 455 just before we reboot
the device.

[Devlin, 2016/05/24 16:57:31]

Ah, whoops!  Misread this.  You're right.  Though I think we should still store
this time as a member on the API to avoid re-reading it each time we schedule a
restart.

[afakhry, 2016/05/25 01:55:46]

We now get it from the pref service.

+        extension_id, kPrefRestartOnWatchdogTime,
+        base::Bind(&RuntimeAPI::ScheduleDelayedRestart,
+                   weak_ptr_factory_.GetWeakPtr(), extension_id, now,
+                   seconds_from_now, callback));
+  } else {
+    std::unique_ptr<base::Value> stored_last_restart(
+        new base::FundamentalValue(0.0));
+    ScheduleDelayedRestart(extension_id, now, seconds_from_now, callback,
+                           std::move(stored_last_restart));
+  }
+
+  return RestartOnWatchdogStatus::SUCCESS_RESTART_SCHEDULED;
+}
+
 bool RuntimeAPI::OpenOptionsPage(const Extension* extension) {
   return delegate_->OpenOptionsPage(extension);
 }
 
+void RuntimeAPI::MaybeCancelRunningWatchdogTimer() {
+  if (!watchdog_timer_.IsRunning())
+    return;
+
+  if (!current_watchdog_request_callback_.is_null()) {
+    current_watchdog_request_callback_.Run(false,
+                                           kMessageRestartRequestCanceled);
+    current_watchdog_request_callback_.Reset();
+  }
+}
+
+void RuntimeAPI::ScheduleDelayedRestart(
+    const std::string& extension_id,
+    const base::Time& now,
+    int seconds_from_now,
+    const OnWatchdogTimeoutCallback& callback,
+    std::unique_ptr<base::Value> stored_last_restart) {
+  base::TimeDelta delay_till_restart =
+      base::TimeDelta::FromSeconds(seconds_from_now);
+
+  // Read the last restart  time, and throttle restart requests that are
+  // received too soon successively.
+  double last_restart_time_double = 0.0;
+  if (stored_last_restart &&
+      stored_last_restart->GetAsDouble(&last_restart_time_double)) {
+    base::Time last_restart_time =
+        base::Time::FromDoubleT(last_restart_time_double);
+
+    base::Time future_restart_time = now + delay_till_restart;
+    base::TimeDelta future_time_since_last_restart =
+        future_restart_time > last_restart_time
+            ? future_restart_time - last_restart_time
+            : base::TimeDelta();
+    if (future_time_since_last_restart < minimum_duration_between_restarts_) {
+      // Schedule the restart after |minimum_duration_between_restarts_| has
+      // passed.
+      delay_till_restart =
+          minimum_duration_between_restarts_ - (now - last_restart_time);
+    }
+  }
+
+  MaybeCancelRunningWatchdogTimer();
+  current_watchdog_request_callback_ = callback;
+  watchdog_timer_.Start(
+      FROM_HERE, delay_till_restart,
+      base::Bind(&RuntimeAPI::OnRestartWatchdogTimeout,
+                 weak_ptr_factory_.GetWeakPtr(), extension_id));
+}
+
+void RuntimeAPI::OnRestartWatchdogTimeout(const std::string& extension_id) {
+  // We can persist "now" as the last successful restart time, assuming that the
+  // following restart request will succeed, since it can only fail if requested
+  // by non kiosk apps, and we prevent that from the beginning (unless in
+  // browsertests).
+  // This assumption is important, since once restart is requested, we might not
+  // have enough time to persist the data to disk.
+  StateStore* storage = ExtensionSystem::Get(browser_context_)->state_store();
+  if (storage) {
+    base::Time now = base::Time::NowFromSystemTime();
+    std::unique_ptr<base::Value> restart_time(
+        new base::FundamentalValue(now.ToDoubleT()));
+    storage->SetExtensionValue(extension_id, kPrefRestartOnWatchdogTime,
+                               std::move(restart_time));
+  }
+
+  std::string error_message;
+  const bool success = delegate_->RestartDevice(&error_message);
+
+  current_watchdog_request_callback_.Run(success, error_message);
+  current_watchdog_request_callback_.Reset();
+
+  if (!success && !allow_non_kiost_apps_restart_api_for_test) {
+    // This is breaking our above assumption and should never be reached.
+    NOTREACHED();
+  }
+}
+
+void RuntimeAPI::AllowNonKiostAppsInRestartOnWatchdogForTesting() {
+  allow_non_kiost_apps_restart_api_for_test = true;
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 
 // static
@@ -551,6 +698,55 @@ ExtensionFunction::ResponseAction RuntimeRestartFunction::Run() {
   return RespondNow(NoArguments());
 }
 
+ExtensionFunction::ResponseAction RuntimeRestartOnWatchdogFunction::Run() {
+  if (!allow_non_kiost_apps_restart_api_for_test &&
+      !ExtensionsBrowserClient::Get()->IsRunningInForcedAppMode()) {
+    return RespondNow(Error(kErrorOnlyKioskModeAllowed));
+  }
+
+  std::unique_ptr<api::runtime::RestartOnWatchdog::Params> params(
+      api::runtime::RestartOnWatchdog::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
+  int seconds = params->seconds;
+
+  if (seconds < -1)
+    return RespondNow(Error(kErrorInvalidArgument, base::IntToString(seconds)));
+
+  RuntimeAPI::RestartOnWatchdogStatus request_status =
+      RuntimeAPI::GetFactoryInstance()
+          ->Get(browser_context())
+          ->RestartDeviceOnWatchdogTimeout(
+              extension()->id(), seconds,
+              base::Bind(&RuntimeRestartOnWatchdogFunction::OnWatchdogTimeout,
+                         this));
+
+  switch (request_status) {
+    case RuntimeAPI::RestartOnWatchdogStatus::FAILED_NOT_FIRST_EXTENSION:
+      return RespondNow(Error(kErrorOnlyFirstExtensionAllowed));
+
+    case RuntimeAPI::RestartOnWatchdogStatus::SUCCESS_RESTART_CANCELLED:
+      return RespondNow(
+          ArgumentList(api::runtime::RestartOnWatchdog::Results::Create(
+              true, kMessageNoMoreRestartsScheduled)));
+
+    case RuntimeAPI::RestartOnWatchdogStatus::SUCCESS_RESTART_SCHEDULED:
+      return RespondLater();
+  }
+
+  NOTREACHED();
+  return RespondNow(Error(kErrorInvalidStatus));
+}
+
+void RuntimeRestartOnWatchdogFunction::OnWatchdogTimeout(

[Devlin, 2016/05/20 17:26:32]

This strikes me as a little scary, since we'll almost always leak this. Is there
a reason we can't just let the extension know if the restart was properly
scheduled, rather than executing a callback later?

[afakhry, 2016/05/21 01:13:00]

Could you please clarify how we leak it? I tried to make sure, we don't schedule
any new restart until we're completely done with the previous one. Also we don't
schedule anyone (and hence call RespondLater()) until we're quite sure no errors
or invalid arguments or state where encountered.

This also helps a lot with the browsertests.

[Devlin, 2016/05/23 16:49:39]

For leaking, I was expecting the restart function to be synchronous.  So either
we restarted (in which case this never returns, and leaks) or didn't restart (in
which case we wait indefinitely).  I suppose depending on how often we replace
the restart request, it may not be all the time.  Even so, this pattern seems
really weird.  I'd much rather invoke the callback immediately and just have a
state of success vs failure in scheduling the restart.

[afakhry, 2016/05/25 01:55:46]

Done. No more RespondLater(), and the response callback is invoked when the
restart is successfully scheduled. Tests have been modified accordingly.

+    bool success,
+    const std::string& message) {
+  if (!success)
+    WriteToConsole(content::CONSOLE_MESSAGE_LEVEL_ERROR, message);
+
+  Respond(ArgumentList(
+      api::runtime::RestartOnWatchdog::Results::Create(success, message)));
+}
+
 ExtensionFunction::ResponseAction RuntimeGetPlatformInfoFunction::Run() {
   runtime::PlatformInfo info;
   if (!RuntimeAPI::GetFactoryInstance()