Feature detection-friendly restrictions for packaged apps.

Packaged apps do not have access to some features of the Web platform
that are deprecated, or that do not make sense in the context of an
app. Currently, accessing most of those features throws an error.

This change makes thse features behave more like missing featurs on the
Web platform. Whenever possible, accessing restricted features returns
undefined and logs an error to the browser console. Features accessible
via non-configurable ES5 properties (e.g. window.alert, document.write)
either become no-ops or throw errors, on a case-by-case basis.

Deprecated events have their "on*" properties return undefined (as
opposed to null, for events with no handlers) to facilitate feature
detection. Using addEventListener for these events used to throw an
error, and now logs an error to the console and turns into a no-op, to
mimic the behavior for undefined events as well as possible.

This change aims to make JavaScript libraries written for browsers work
in packaged apps as seamlessly as possible, while still providing useful
warnings for developers. This creates a couple of warts: (1) properties
are not removed, so the "in" operator will still see them, and (2)
dispatchEvent will never deliver deprecated events, but it normally
delives un-implemented events.

BUG=330487

Review URL: https://codereview.chromium.org/120733003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@243817 0039d316-1c4b-4281-b951-d872f2087c98

chrome/renderer/extensions/safe_builtins.cc

@@ -66,7 +66,8 @@ const char kScript[] =
     "// Save only what is needed to make tests that override builtins pass.\n"
     "saveBuiltin(Object,\n"
     "            ['hasOwnProperty'],\n"
-    "            ['getPrototypeOf', 'keys']);\n"
+    "            ['defineProperty', 'getOwnPropertyDescriptor',\n"
+    "             'getPrototypeOf', 'keys']);\n"
     "saveBuiltin(Function,\n"
     "            ['apply', 'bind', 'call']);\n"
     "saveBuiltin(Array,\n"

chrome/renderer/resources/extensions/platform_app.js

@@ -2,22 +2,59 @@
 // 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() {
-    var message = messagePrefix + ' is not available in packaged apps.';
-    if (opt_messageSuffix) message = message + ' ' + opt_messageSuffix;
-    throw message;
+    $console.error(message);
+    return;
   };
 }
 
 /**
- * Replaces the given methods of the passed in object with stubs that throw
- * 'not available' exceptions when called.
+ * 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() {
+    throw new Error(message);
+  };
+}
+
+/**
+ * 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.
+ *
+ * In most cases, the useThrowingStubs should be false, so the stubs used to
+ * replace the methods log an error to the console, but allow the calling code
+ * to continue. We shouldn't break library code that uses feature detection
+ * responsibly, such as:
+ *     if(window.confirm) {
+ *       var result = window.confirm('Are you sure you want to delete ...?');
+ *       ...
+ *     }
+ *
+ * useThrowingStubs should only be true for methods that are deprecated in the
+ * Web platform, and should not be used by a responsible library, even in
+ * conjunction with feature detection. A great example is document.write(), as
+ * the HTML5 specification recommends against using it, and says that its
+ * behavior is unreliable. No reasonable library code should ever use it.
+ * HTML5 spec: http://www.w3.org/TR/html5/dom.html#dom-document-write
  *
  * @param {Object} object The object with methods to disable. The prototype is
  *     preferred.
@@ -25,18 +62,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) {
   $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 +93,67 @@ 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.
+        }
       }
     });
   });
 }
 
-// Disable document.open|close|write|etc.
-disableMethods(HTMLDocument.prototype, 'document',
-    ['open', 'clear', 'close', 'write', 'writeln']);
+/**
+ * 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. The prototype
+ *     is preferred.
+ * @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 benign Document methods.
+disableMethods(HTMLDocument.prototype, 'document', ['open', 'clear', 'close']);
+
+// Replace evil Document methods with exception-throwing stubs.
+disableMethods(HTMLDocument.prototype, 'document', ['write', 'writeln'], true);
 
 // 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']);
@@ -120,15 +193,11 @@ window.addEventListener('readystatechange', function(event) {
   // it first to 'undefined' to avoid this.
   document.all = undefined;
   disableGetters(document, 'document',
-      ['alinkColor', 'all', 'bgColor', 'fgColor', 'linkColor',
-       'vlinkColor']);
+      ['alinkColor', 'all', 'bgColor', 'fgColor', 'linkColor', 'vlinkColor']);
 }, 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')

chrome/test/data/extensions/platform_apps/disabled_window_properties/test.js

@@ -4,19 +4,7 @@
 
 // NOTE: Some of the test code was put in the global scope on purpose!
 
-function useToolbarGetter() {
-  var result;
-  try {
-    // The following will invoke the window.toolbar getter, and this may or may
-    // not throw an exception.
-    result = {object: window.toolbar};
-  } catch (e) {
-    result = {exception: e};
-  }
-  return result;
-}
-
-var resultFromToolbarGetterAtStart = useToolbarGetter();
+var resultFromToolbarGetterAtStart = window.toolbar;
 
 // The following statement implicitly invokes the window.toolbar setter.  This
 // should delete the "disabler" getter and setter that were set up in
@@ -24,17 +12,15 @@ var resultFromToolbarGetterAtStart = useToolbarGetter();
 // getter/setter behaviors from here on.
 var toolbar = {blah: 'glarf'};
 
-var resultFromToolbarGetterAfterRedefinition = useToolbarGetter();
+var resultFromToolbarGetterAfterRedefinition = window.toolbar;
 var toolbarIsWindowToolbarAfterRedefinition = (toolbar === window.toolbar);
 
 toolbar.blah = 'baz';
 
 chrome.app.runtime.onLaunched.addListener(function() {
-  chrome.test.assertTrue(
-      resultFromToolbarGetterAtStart.hasOwnProperty('exception'));
-
-  chrome.test.assertTrue(
-      resultFromToolbarGetterAfterRedefinition.hasOwnProperty('object'));
+  chrome.test.assertEq('undefined', typeof(resultFromToolbarGetterAtStart));
+  chrome.test.assertEq('object',
+      typeof(resultFromToolbarGetterAfterRedefinition));
   chrome.test.assertTrue(toolbarIsWindowToolbarAfterRedefinition);
 
   chrome.test.assertEq('baz', toolbar.blah);

chrome/test/data/extensions/platform_apps/restrictions/main.js

@@ -26,30 +26,48 @@ function assertThrowsError(method, opt_expectedError) {
 }
 
 chrome.test.runTests([
-  function testDocument() {
-    assertThrowsError(document.open);
-    assertThrowsError(document.clear);
-    assertThrowsError(document.close);
+  function testDocumentBenignMethods() {
+    // The real document.open returns a document.
+    assertEq('undefined', typeof(document.open()));
+
+    // document.clear() has been deprecated on the Web as well, so there is no
+    // good method of testing that the method has been stubbed. We have to
+    // settle for testing that calling the method doesn't throw.
+    assertEq('undefined', typeof(document.clear()));
+
+    // document.close() doesn't do anything on its own, so there is good method
+    // of testing that it has been stubbed. Settle for making sure it doesn't
+    // throw.
+    assertEq('undefined', typeof(document.close()));
+
+    succeed();
+  },
+
+  function testDocumentEvilMethods() {
     assertThrowsError(document.write);
     assertThrowsError(document.writeln);
 
-    assertThrowsError(function() {document.all;});
-    assertThrowsError(function() {document.bgColor;});
-    assertThrowsError(function() {document.fgColor;});
-    assertThrowsError(function() {document.alinkColor;});
-    assertThrowsError(function() {document.linkColor;});
-    assertThrowsError(function() {document.vlinkColor;});
+    succeed();
+  },
+
+  function testDocumentGetters() {
+    assertEq('undefined', typeof(document.all));
+    assertEq('undefined', typeof(document.bgColor));
+    assertEq('undefined', typeof(document.fgColor));
+    assertEq('undefined', typeof(document.alinkColor));
+    assertEq('undefined', typeof(document.linkColor));
+    assertEq('undefined', typeof(document.vlinkColor));
 
     succeed();
   },
 
   function testHistory() {
-    // These are replaced by wrappers that throws exceptions.
-    assertThrowsError(history.back);
-    assertThrowsError(history.forward);
-    assertThrowsError(function() {history.length;});
+    // Accessing these logs warnings to the console.
+    assertEq('undefined', typeof(history.back));
+    assertEq('undefined', typeof(history.forward));
+    assertEq('undefined', typeof(history.length));
 
-    // These are part of the HTML5 History API that is feature detected, so we
+    // These are part of the HTML5 History API that are feature detected, so we
     // remove them altogether, allowing apps to have fallback behavior.
     chrome.test.assertFalse('pushState' in history);
     chrome.test.assertFalse('replaceState' in history);
@@ -59,30 +77,30 @@ chrome.test.runTests([
   },
 
   function testWindowFind() {
-    assertThrowsError(Window.prototype.find);
-    assertThrowsError(window.find);
-    assertThrowsError(find);
+    assertEq('undefined', typeof(Window.prototype.find('needle')));
+    assertEq('undefined', typeof(window.find('needle')));
+    assertEq('undefined', typeof(find('needle')));
     succeed();
   },
 
   function testWindowAlert() {
-    assertThrowsError(Window.prototype.alert);
-    assertThrowsError(window.alert);
-    assertThrowsError(alert);
+    assertEq('undefined', typeof(Window.prototype.alert()));
+    assertEq('undefined', typeof(window.alert()));
+    assertEq('undefined', typeof(alert()));
     succeed();
   },
 
   function testWindowConfirm() {
-    assertThrowsError(Window.prototype.confirm);
-    assertThrowsError(window.confirm);
-    assertThrowsError(confirm);
+    assertEq('undefined', typeof(Window.prototype.confirm('Failed')));
+    assertEq('undefined', typeof(window.confirm('Failed')));
+    assertEq('undefined', typeof(confirm('Failed')));
     succeed();
   },
 
   function testWindowPrompt() {
-    assertThrowsError(Window.prototype.prompt);
-    assertThrowsError(window.prompt);
-    assertThrowsError(prompt);
+    assertEq('undefined', typeof(Window.prototype.prompt('Failed')));
+    assertEq('undefined', typeof(window.prompt('Failed')));
+    assertEq('undefined', typeof(prompt('Failed')));
     succeed();
   },
 
@@ -90,29 +108,31 @@ chrome.test.runTests([
     var bars = ['locationbar', 'menubar', 'personalbar',
                 'scrollbars', 'statusbar', 'toolbar'];
     for (var x = 0; x < bars.length; x++) {
-      assertThrowsError(function() {
-        var visible = this[bars[x]].visible;
-        visible = window[bars[x]].visible;
-      });
+      assertEq('undefined', typeof(this[bars[x]]));
+      assertEq('undefined', typeof(window[bars[x]]));
     }
     succeed();
   },
 
   function testBlockedEvents() {
-    var eventHandler = function() { fail('event handled'); };
+    // Fails the test if called by dispatchEvent().
+    var eventHandler = function() { fail('blocked event handled'); };
+
     var blockedEvents = ['unload', 'beforeunload'];
 
     for (var i = 0; i < blockedEvents.length; ++i) {
-      assertThrowsError(function() {
-        window['on' + blockedEvents[i]] = eventHandler;
-      });
-      assertThrowsError(function() {
-        window.addEventListener(blockedEvents[i], eventHandler);
-      });
-      assertThrowsError(function() {
-        Window.prototype.addEventListener.apply(window,
-            [blockedEvents[i], eventHandler]);
-      });
+      window['on' + blockedEvents[i]] = eventHandler;
+      assertEq(undefined, window['on' + blockedEvents[i]]);
+
+      var event = new Event(blockedEvents[i]);
+      window.addEventListener(blockedEvents[i], eventHandler);
+      // Ensures that addEventListener did not actually register the handler.
+      // If eventHandler is registered as a listener, it will be called by
+      // dispatchEvent() and the test will fail.
+      window.dispatchEvent(event);
+      Window.prototype.addEventListener.apply(window,
+          [blockedEvents[i], eventHandler]);
+      window.dispatchEvent(event);
     }
 
     succeed();
@@ -132,7 +152,7 @@ chrome.test.runTests([
   function testIframe() {
     var iframe = document.createElement('iframe');
     iframe.onload = function() {
-      assertThrowsError(iframe.contentWindow.alert);
+      assertThrowsError(iframe.contentWindow.document.write);
       succeed();
     };
     iframe.src = 'iframe.html';

chrome/test/data/extensions/platform_apps/restrictions/sandboxed_iframe.js

@@ -3,17 +3,16 @@
 // found in the LICENSE file.
 
 window.onload = function() {
-  try {
-    window.alert('should throw');
+  window.onunload = function() {
     window.parent.postMessage({'success': false,
-                               'reason' : 'should have thrown'},
+                               'reason' : 'unload handler works'},
                               '*');
-  } catch(e) {
-    var message = e.message || e;
-    var succ = message.indexOf('is not available in packaged apps') != -1;
-    window.parent.postMessage({'success': succ,
-                               'reason' : 'got wrong error: ' + message},
+  };
+  if (typeof(window.unload) !== 'undefined') {
+    window.parent.postMessage({'success': false,
+                               'reason' : 'unload is not undefined'},
                               '*');
-
   }
+  window.dispatchEvent(new Event('unload'));
+  window.parent.postMessage({'success': true}, '*');
 };

chrome/test/data/extensions/platform_apps/storage/test.js

@@ -18,15 +18,7 @@ chrome.test.runTests([
   },
 
   function testLocalStorage() {
-    try {
-      window.localStorage;
-      chrome.test.fail('error not thrown');
-    } catch (e) {
-      var message = e.message || e;
-      var expected = 'is not available in packaged apps. ' +
-                     'Use chrome.storage.local instead.';
-      assertContains(message, expected, 'Unexpected message ' + message);
-      chrome.test.succeed();
-    }
+    chrome.test.assertTrue(!window.localStorage);
+    chrome.test.succeed();
   }
 ]);