Feature detection-friendly restrictions for packaged apps.

chrome/renderer/resources/extensions/platform_app.js

Index: chrome/renderer/resources/extensions/platform_app.js
diff --git a/chrome/renderer/resources/extensions/platform_app.js b/chrome/renderer/resources/extensions/platform_app.js
index 78a13ffe01e51ea4049d8abccc37a02f59f24695..1bd468dec72130d85a935854584f30fd25338d8f 100644
--- a/chrome/renderer/resources/extensions/platform_app.js
+++ b/chrome/renderer/resources/extensions/platform_app.js
@@ -2,22 +2,43 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+var $console = window.console;
+
 /**
- * Returns a function that throws a 'not available' exception when called.
+ * Returns a function that logs a 'not available' error to the console and
+ * returns undefined.
  *
  * @param {string} messagePrefix text to prepend to the exception message.
  */
 function generateDisabledMethodStub(messagePrefix, opt_messageSuffix) {
+  var message = messagePrefix + ' is not available in packaged apps.';
+  if (opt_messageSuffix) message = message + ' ' + opt_messageSuffix;
+  return function() {
+    $console.error(message);
+    return;
+  };
+}
+
+/**
+ * Returns a function that throws a 'not available' error.
+ *
+ * @param {string} messagePrefix text to prepend to the exception message.
+ */
+function generateThrowingMethodStub(messagePrefix, opt_messageSuffix) {
+  var message = messagePrefix + ' is not available in packaged apps.';
+  if (opt_messageSuffix) message = message + ' ' + opt_messageSuffix;
   return function() {
-    var message = messagePrefix + ' is not available in packaged apps.';
-    if (opt_messageSuffix) message = message + ' ' + opt_messageSuffix;
-    throw message;
+    throw new Error(message);
   };
 }
 
 /**
- * Replaces the given methods of the passed in object with stubs that throw
- * 'not available' exceptions when called.
+ * Replaces the given methods of the passed in object with stubs that log
+ * 'not available' errors to the console and return undefined.
+ *
+ * This should be used on methods attached via non-configurable properties,
+ * such as window.alert. disableGetters should be used when possible, because
+ * it is friendlier towards feature detection.
  *
  * @param {Object} object The object with methods to disable. The prototype is
  *     preferred.
@@ -25,18 +46,23 @@ function generateDisabledMethodStub(messagePrefix, opt_messageSuffix) {
  *     thrown by the stub (this is the name that the object is commonly referred
  *     to by web developers, e.g. "document" instead of "HTMLDocument").
  * @param {Array.<string>} methodNames names of methods to disable.
+ * @param {Boolean} useThrowingStubs if true, the replaced methods will throw
+ *     an error instead of silently returning undefined
  */
-function disableMethods(object, objectName, methodNames) {
+function disableMethods(object, objectName, methodNames, useThrowingStubs) {

[Jeffrey Yasskin, 2014/01/08 22:49:17]

It seems like it would make sense for a *call* to a disabled method to always
throw something, since if the method were absent (undefined), the call would
throw a TypeError. It's only getters that it makes sense to use a console.error
for, right?

[pwnall-personal, 2014/01/08 23:20:46]

Completely agreed, if getting the method from the object would return
"undefined". Since we can't do that for non-configurable properties, I don't
think it's OK to throw an exception on call.

I think the following code follows responsible feature detection practices, and
shouldn't break.

if (window.confirm) {
    var response = window.confirm("Are you sure you want to delete this?");
    ...
}

[Jeffrey Yasskin, 2014/01/09 00:44:19]

Oh, right. Yes, your change makes sense. Maybe comment that, or use your example
in the test, so that a future maintainer doesn't "fix" it?

[pwnall-personal, 2014/01/09 05:47:26]

Done.
Thank you very much for the suggestion!

I added some comments to the method description.

There are some tests for the current behavior in one of the tests, but they
don't document the reasoning behind them.
https://codereview.chromium.org/120733003/patch/440001/450004

I think it makes more sense to have the reasoning in the source code, because I
expect people wanting to change the behavior to look at the source before they
look into updating tests.

   $Array.forEach(methodNames, function(methodName) {
-    object[methodName] =
-        generateDisabledMethodStub(objectName + '.' + methodName + '()');
+    var messagePrefix = objectName + '.' + methodName + '()';
+    object[methodName] = useThrowingStubs ?
+        generateThrowingMethodStub(messagePrefix) :
+        generateDisabledMethodStub(messagePrefix);
   });
 }
 
 /**
- * Replaces the given properties of the passed in object with stubs that throw
- * 'not available' exceptions when gotten.  If a property's setter is later
- * invoked, the getter and setter are restored to default behaviors.
+ * Replaces the given properties of the passed in object with stubs that log
+ * 'not available' warnings to the console and return undefined when gotten. If
+ * a property's setter is later invoked, the getter and setter are restored to
+ * default behaviors.
  *
  * @param {Object} object The object with properties to disable. The prototype
  *     is preferred.
@@ -51,36 +77,66 @@ function disableGetters(object, objectName, propertyNames, opt_messageSuffix) {
     var stub = generateDisabledMethodStub(objectName + '.' + propertyName,
                                           opt_messageSuffix);
     stub._is_platform_app_disabled_getter = true;
-    object.__defineGetter__(propertyName, stub);
-
-    object.__defineSetter__(propertyName, function(value) {
-      var getter = this.__lookupGetter__(propertyName);
-      if (!getter || getter._is_platform_app_disabled_getter) {
-        // The stub getter is still defined.  Blow-away the property to restore
-        // default getter/setter behaviors and re-create it with the given
-        // value.
-        delete this[propertyName];
-        this[propertyName] = value;
-      } else {
-        // Do nothing.  If some custom getter (not ours) has been defined, there
-        // would be no way to read back the value stored by a default setter.
-        // Also, the only way to clear a custom getter is to first delete the
-        // property.  Therefore, the value we have here should just go into a
-        // black hole.
+    $Object.defineProperty(object, propertyName, {
+      configurable: true,
+      enumerable: false,
+      get: stub,
+      set: function(value) {
+        var descriptor = $Object.getOwnPropertyDescriptor(this, propertyName);
+        if (!descriptor || !descriptor.get ||
+            descriptor.get._is_platform_app_disabled_getter) {
+          // The stub getter is still defined.  Blow-away the property to
+          // restore default getter/setter behaviors and re-create it with the
+          // given value.
+          delete this[propertyName];
+          this[propertyName] = value;
+        } else {
+          // Do nothing.  If some custom getter (not ours) has been defined,
+          // there would be no way to read back the value stored by a default
+          // setter. Also, the only way to clear a custom getter is to first
+          // delete the property.  Therefore, the value we have here should
+          // just go into a black hole.
+        }
       }
     });
   });
 }
 
+/**
+ * Replaces the given properties of the passed in object with stubs that log
+ * 'not available' warnings to the console when set.
+ *
+ * @param {Object} object The object with properties to disable.
+ *     is preferred.

[Jeffrey Yasskin, 2014/01/08 22:49:17]

What "is preferred."?

[pwnall-personal, 2014/01/08 23:20:46]

More care when copy-pasting is definitely preferred :)

Thank you very much for catching this!

+ * @param {string} objectName The display name to use in the error message
+ *     thrown by the setter stub (this is the name that the object is commonly
+ *     referred to by web developers, e.g. "document" instead of
+ *     "HTMLDocument").
+ * @param {Array.<string>} propertyNames names of properties to disable.
+ */
+function disableSetters(object, objectName, propertyNames, opt_messageSuffix) {
+  $Array.forEach(propertyNames, function(propertyName) {
+    var stub = generateDisabledMethodStub(objectName + '.' + propertyName,
+                                          opt_messageSuffix);
+    $Object.defineProperty(object, propertyName, {
+      configurable: true,
+      enumerable: false,
+      get: function() {
+        return;
+      },
+      set: stub
+    });
+  });
+}
+
 // Disable document.open|close|write|etc.
 disableMethods(HTMLDocument.prototype, 'document',
-    ['open', 'clear', 'close', 'write', 'writeln']);
+    ['open', 'clear', 'close', 'write', 'writeln'],
+    true);

[Jeffrey Yasskin, 2014/01/08 22:49:17]

Why does document get throwing method stubs, but window doesn't?

[pwnall-personal, 2014/01/08 23:20:46]

I treated the document methods differently because the HTML5 specification says
document.write is evil and may throw exceptions in some cases, so I figured it's
OK to keep the current behavior of throwing exceptions.
http://www.w3.org/TR/html5/dom.html#dom-document-write

Most importantly, code that relies on feature detection to call these methods
(especially on document.write, the most popular one) will break in a standard
browser environment, so it's OK if it breaks in a Chrome App.

I'm not too hung up on the idea of throwing exceptions here. Some browsers just
ignore the calls, so I'd be happy with that solution as well. I care more about
"sane" methods not throwing in Chrome Apps.

What do you think?

[Jeffrey Yasskin, 2014/01/09 00:44:19]

I don't have a strong opinion here at all (i.e. do what you want after thinking
about it), but it sounds like write() and writeln() are evil, so we shouldn't
mind throwing on calls to them, but maybe open(), clear(), and close() are less
evil, so we should be more careful for them?

[pwnall-personal, 2014/01/09 05:47:26]

That makes perfect sense, thank you!

I updated the CL to do exactly that. Unfortunately, now we don't have a good way
to test that document.close() and document.clear() are stubbed, but I think the
tests cover the behavior that we actually care about.

In the future, if we want more precise testing, we can check that
document.close.toString() does not contain "[native method]", but that seems
hacky and not worth it for now.

 
 // Disable history.
 window.history = {};
-disableMethods(window.history, 'history',
-    ['back', 'forward', 'go']);
-disableGetters(window.history, 'history', ['length']);
+disableGetters(window.history, 'history', ['back', 'forward', 'go', 'length']);
 
 // Disable find.
 disableMethods(Window.prototype, 'window', ['find']);
@@ -125,10 +181,7 @@ window.addEventListener('readystatechange', function(event) {
 }, true);
 
 // Disable onunload, onbeforeunload.
-Window.prototype.__defineSetter__(
-    'onbeforeunload', generateDisabledMethodStub('onbeforeunload'));
-Window.prototype.__defineSetter__(
-    'onunload', generateDisabledMethodStub('onunload'));
+disableSetters(Window.prototype, 'window', ['onbeforeunload', 'onunload']);
 var windowAddEventListener = Window.prototype.addEventListener;
 Window.prototype.addEventListener = function(type) {
   if (type === 'unload' || type === 'beforeunload')