Preventing race conditions in Google Now extension

chrome/browser/resources/google_now/utility.js

+// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+'use strict';
+
+/**
+ * @fileoverview Utility objects and functions for Google Now extension.
+ */
+
+/**
+ * Checks for internal errors.
+ * @param {boolean} condition Condition that must be true.
+ * @param {string} message Diagnostic message for the case when the condition is
+ *     false.
+ */
+function verify(condition, message) {
+  // TODO(vadimt): Send UMAs instead of showing alert.
+  // TODO(vadimt): Make sure the execution doesn't continue after this call.

[skare_, 2013/03/14 00:48:50]

how? why?
don't mean that to sound short, just curious what you're trying to accomplish.

[rgustafson, 2013/03/14 19:05:20]

This was in relation to wanting to early return on certain things that were
asserted from what I understand. (?) That problem I don't think can wait, but I
don't know how you'll accomplish that here.
On 2013/03/14 00:48:50, Travis Skare wrote:

[vadimt, 2013/03/14 22:56:05]

Well, you asked to make sure that in startFirst(), we check that the queue is
not empty, and if empty, don't execute the rest of the method. I've added
'verify()' call into startFirst() and todo item into verify() to accomplish
this.
Likely, I'll throw an exception here.

[vadimt, 2013/03/14 22:56:05]

Correct. I'll accomplish this by throwing an exception. I don't do this right
now because this is required for this change (that I need to unblock the next
CL, which is retry-dismiss), and it requires some research (should I throw
string, which works in the context of HTML5 callbacks, or a structure containing
error message and a stack, which works in the case of Chrome API callbacks).

+  if (!condition) {
+    var errorText = 'ASSERT: ' + message;
+    console.error(errorText);
+    alert(errorText);
+  }
+}
+
+/**
+ * Builds the object to manage tasks (mutually exclusive chains of events).
+ * @param {function(string, string): boolean} areConflicting Function that
+ *     checks if a new task can't be added to a task queue that contains an
+ *     existing task.
+ * @return {Object} Set of methods to manage tasks.
+ */
+function TaskManager(areConflicting) {
+  /**
+   * Name of the alarm that triggers the error saying that the event page cannot
+   * unload.
+   */
+  var CANNOT_UNLOAD_ALARM_NAME = 'CANNOT-UNLOAD';
+
+  /**
+   * Maximal time we expect the event page to stay loaded after starting a task.
+   */
+  var MAXIMUM_LOADED_TIME_MINUTES = 5;
+
+  /**
+   * Queue of scheduled tasks. The first element, if present, corresponds to the
+   * currently running task.
+   * @type {Array.<Object.<TaskName, function()>>}

[skare_, 2013/03/14 00:48:50]

the TaskName typedef might belong in this file.

[vadimt, 2013/03/14 22:56:05]

Doesn't it have to list all task names, which doesn't belong to this layer?

[skare_, 2013/03/19 18:51:39]

ok... probably not an issue since we're not running JSCompiler to check and
minify, but IIRC it would complain about not knowing what a TaskName is here, so
at this level a string or some such prevents this from having dependencies on a
higher level.

On 2013/03/14 22:56:05, vadimt wrote:

[vadimt, 2013/03/19 20:01:55]

Already done.

+   */
+  var queue = [];
+
+  /**
+   * Name of the current step of the currently running task if present,
+   * otherwise, null. For diagnostics only.
+   * It's set when the task is started and before each asynchronous operation.
+   */
+  var stepName = null;
+
+  /**
+   * Starts the first queued task.
+   */
+  function startFirst() {
+    verify(queue.length >= 1, 'startFirst: queue is empty');
+
+    // Set alarm to verify that the event page will unload in a reasonable time.
+    chrome.alarms.create(CANNOT_UNLOAD_ALARM_NAME,
+                         {delayInMinutes: MAXIMUM_LOADED_TIME_MINUTES});
+
+    // Starts the oldest queued task, but doesn't remove it from the queue.

[skare_, 2013/03/14 00:48:50]

I forget the long and storied history of this comment, but I think inside a
function:

"Start the [...] task, but don't remove it [...]"

[vadimt, 2013/03/14 22:56:05]

Done.

+    verify(stepName == null, 'tasks.startFirst: stepName is not null');
+    var entry = queue[0];
+    stepName = entry.name + '-initial';
+    entry.task();
+  }
+
+  /**
+   * Checks if a new task can be added to the task queue.
+   * @param {TaskName} taskName Name of the new task.
+   * @return {boolean} Whether the new task can be added.
+   */
+  function canQueue(taskName) {
+    for (var i = 0; i < queue.length; ++i)
+      if (areConflicting(taskName, queue[i].name))
+        return false;
+
+    return true;
+  }
+
+  /**
+   * Adds a new task. If another task is not running, run the task immediately.

[skare_, 2013/03/14 00:48:50]

nit: s/run/runs. s/ignore/ignores and s/store/stores below

[vadimt, 2013/03/14 22:56:05]

Done.

+   * If any task in the queue is not compatible with the task, ignore the new
+   * task. Otherwise, store the task for future execution.
+   * @param {TaskName} taskName Name of the task.
+   * @param {function()} task Code of the task.

[skare_, 2013/03/14 00:48:50]

code of the task -> "function to run" or similar?

[vadimt, 2013/03/14 22:56:05]

Done.

+   */
+  function submit(taskName, task) {
+    if (!canQueue(taskName))
+      return;
+
+    queue.push({name: taskName, task: task});
+
+    if (queue.length == 1) {
+      startFirst();
+    }
+  }
+
+  /**
+   * Completes the current task and start the next queued task if available.

[skare_, 2013/03/14 00:48:50]

s/start/starts

[vadimt, 2013/03/14 22:56:05]

Done.

+   */
+  function finish() {
+    verify(queue.length >= 1, 'tasks.finish: The task queue is empty.');

[skare_, 2013/03/14 00:48:50]

referencing comment on verify() way up at the top of this file, here you might
want to just early-return rather than preventing execution, for example.

[vadimt, 2013/03/14 22:56:05]

I'll build a general solution (likely, throwing an exception) into verify(),
instead of having individual 'return's here and there.

+    queue.shift();
+    stepName = null;
+
+    if (queue.length >= 1)
+      startFirst();
+  }
+
+  /**
+   * Associates a name with the current step of the task. Used for diagnostics
+   * only. A task is a chain of asynchronous events; debugSetStepName should be
+   * called before starting any asynchronous operation.
+   * @param {string} step Name of new step.
+   */
+  function debugSetStepName(step) {

[skare_, 2013/03/14 00:48:50]

would this ever do anything more? If so, this class's code could benefit from
using debugSetStepName so it's set in one place and explicitly called out as
debug.

Overall though it still sort of complicates things on the client side. Could a
similar effect be accomplished using console.log messages, considering the tasks
will be serial (no webworkers or timeslicing/fiber-like behavior)?

[vadimt, 2013/03/14 22:56:05]

It may or may not do additional things in future. But imagine that I add a check
that the task queue is not empty here. Then, it won't be usable from inside if
the class. Generally, there is a style (I don't necessary support it in all
cases) saying "don't use public methods by a class' implementation". There is
also a general agreement that the class doesn't have to always use publicly
exposed setters/getters, for example, if they generate events or check
preconditions.

Speaking of console.log, we want step name appear on our verify() popups and be
reported by early adopters, which cannot be achieved using console.log().

+    // TODO(vadimt): Pass UMA counters instead of step names.
+    stepName = step;
+  }
+
+  chrome.alarms.onAlarm.addListener(function(alarm) {
+    if (alarm.name == CANNOT_UNLOAD_ALARM_NAME) {
+      // Error if the event page wasn't unloaded after a reasonable timeout
+      // since starting the last task.
+      // TODO(vadimt): Uncomment the verify once this bug is fixed:
+      // crbug.com/177563
+      // verify(false, 'Event page didn\'t unload, queue = ' +
+      // JSON.stringify(tasks) + ', step = ' + stepName + ' (ignore this verify
+      // if devtools is attached).');
+    }
+  });
+
+  chrome.runtime.onSuspend.addListener(function() {
+    chrome.alarms.clear(CANNOT_UNLOAD_ALARM_NAME);
+    verify(queue.length == 0 && stepName == null,
+      'Incomplete task when unloading event page, queue = ' +
+      JSON.stringify(queue) + ', step = ' + stepName);
+  });
+
+  return {
+    submit: submit,

[skare_, 2013/03/14 00:48:50]

submit -> pushTask, queueTask, addTask, schedule() or similar?

[vadimt, 2013/03/14 22:56:05]

Done.

+    debugSetStepName: debugSetStepName,
+    finish: finish
+  };
+}