Mark create/update/clear callbacks of notification API as optional

chrome/renderer/resources/extensions/notifications_custom_bindings.js

Index: chrome/renderer/resources/extensions/notifications_custom_bindings.js
diff --git a/chrome/renderer/resources/extensions/notifications_custom_bindings.js b/chrome/renderer/resources/extensions/notifications_custom_bindings.js
index eebf2900920287f0b539b04e32d48e6219a2c444..5c278ec0e78f65ca6363b412031d0ae1b95ee36f 100644
--- a/chrome/renderer/resources/extensions/notifications_custom_bindings.js
+++ b/chrome/renderer/resources/extensions/notifications_custom_bindings.js
@@ -7,6 +7,7 @@
 var binding = require('binding').Binding.create('notifications');
 
 var sendRequest = require('sendRequest').sendRequest;
+var exceptionHandler = require('uncaught_exception_handler');
 var imageUtil = require('imageUtil');
 var lastError = require('lastError');
 var notificationsPrivate = requireNative('notifications_private');
@@ -111,20 +112,23 @@ function genHandle(name, failure_function) {
   return function(id, input_notification_details, callback) {
     // TODO(dewittj): Remove this hack. This is used as a way to deep

[not at google - send to devlin, 2015/01/20 17:41:35]

The best way to deep copy would be to expose a native function which uses
WebSerializedScriptValue.serialize(obj).deserialize(). I think the TODO should
be changed to reflect that. The main advantage over JSON is that recursive
objects wouldn't crash; it may or may not be any quicker.

[robwu, 2015/01/20 22:34:01]

Recursive objects are invalid for this API, so that is not something that we
have to worry about.

I *think* that the purpose of deep cloning the object is to make sure that
changing the object that is being passed in to the function after the function
call does not change the behavior of the first function call.

var buttons = [{ ... }, { ... }];
chrome.notifications.create(..., { buttons: buttons, ... });
buttons.length = 0; // Should not cause buttons to not appear in the
notification.

[dewittj, 2015/01/20 22:46:51]

The JSON copy has 2 important properties:
1) Custom bindings can change the copy without affecting client data structures
2) Client program can run while the API is asynchronously downloading images,
and change the input object without affecting the data passed to Chrome in the
custom bindings.

At the time, extension bindings did not work with prototype properties; i.e.
given var b = Object.create(a), passing b to the bindings would cause a crash
since the schema validator would work but the c++ code would not grab the
properties from a.  Even if it did work, it would be susceptible to #2 above.

I believe Blink contains a function that does deep cloning but it is not part of
the JS runtime.  I'm open to fixing this if there are better answers nowadays.

[robwu, 2015/01/20 22:58:44]

There are many other extension APIs that suffer from 1 or 2, see
https://crbug.com/436588. I believe that we should not develop custom
serialization solutions for each potentially affected extension API, but make
deep cloning a part of the extension parameter validator. If you want to discuss
this further, just follow up on the related bug instead of here to avoid
fragmentation of the discussion.

[not at google - send to devlin, 2015/01/21 22:21:19]

I'm sure that copying is necessary. I was addressing the fact that I presume the
TODO is to avoid copying via JSON and instead copying via <some other method>,
and a good <some other method> is using SerializedScriptValue (the same
serialization scheme that postMessage and IndexedDB use).

[robwu, 2015/01/21 23:21:09]

Yep, I understood that, and I pointed out that copying is just a quick hack to
get immutability of the parameters. A way to achieve this goal is to make
immutable properties a part of the validator, i.e. let the validator return a
(partial) copy of the sanitized parameters.

     // copy a complex JSON object.
-    var notification_details = JSON.parse(
-        JSON.stringify(input_notification_details));
+    var notification_details = $JSON.parse(
+        $JSON.stringify(input_notification_details));
     var that = this;
+    var stack = exceptionHandler.getExtensionStackTrace();
     replaceNotificationOptionURLs(notification_details, function(success) {
       if (success) {
         sendRequest(that.name,
             [id, notification_details, callback],
-            that.definition.parameters);
+            that.definition.parameters, {stack: stack});
         return;
       }
-      lastError.run(name,
-                    'Unable to download all specified images.',
-                    null,
-                    failure_function, [callback, id])
+      var error = 'Unable to download all specified images.';
+      if (callback) {
+        lastError.run(name, error, stack, failure_function, [callback, id]);
+      } else {
+        lastError.reportUncheckedLastError(name, error, stack);
+      }

[not at google - send to devlin, 2015/01/21 22:21:19]

You could avoid needing to expose reportUncheckedLastError publicly with the
equivalent of

lastError.run(name, error, stack, failure_function, [callback || function(){},
id])

[robwu, 2015/01/21 23:21:09]

Done. Though reportUncheckedLastError has to be public anyway, because of the
use in send_request.js and within last_error.js itself.

     });
   };
 }