[Extensions Bindings] Include request id in a custom callback response

extensions/renderer/api_request_handler.cc

 // Copyright 2016 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.
 
 #include "extensions/renderer/api_request_handler.h"
 
 #include "base/bind.h"
 #include "base/guid.h"
 #include "base/memory/ptr_util.h"
 #include "base/values.h"
 #include "content/public/child/v8_value_converter.h"
 #include "gin/converter.h"
+#include "gin/data_object_builder.h"
 #include "third_party/WebKit/public/web/WebScopedUserGesture.h"
 #include "third_party/WebKit/public/web/WebUserGestureIndicator.h"
 
 namespace extensions {
 
 APIRequestHandler::Request::Request() {}
 APIRequestHandler::Request::~Request() = default;
 
 APIRequestHandler::PendingRequest::PendingRequest(
     v8::Isolate* isolate,
@@ skipping 27 matching lines @@
 APIRequestHandler::~APIRequestHandler() {}
 
 int APIRequestHandler::StartRequest(v8::Local<v8::Context> context,
                                     const std::string& method,
                                     std::unique_ptr<base::ListValue> arguments,
                                     v8::Local<v8::Function> callback,
                                     v8::Local<v8::Function> custom_callback,
                                     binding::RequestThread thread) {
   auto request = base::MakeUnique<Request>();
 
+  // The request id is primarily used in the renderer to associate an API
+  // request with the associated callback, but it's also used in the browser as
+  // an identifier for the extension function (e.g. by the pageCapture API).
+  // TODO(devlin): We should probably fix this, since the request id is only
+  // unique per-isolate, rather than globally.
+  int request_id = next_request_id_++;
+
   if (!custom_callback.IsEmpty() || !callback.IsEmpty()) {
     v8::Isolate* isolate = context->GetIsolate();
     // In the JS bindings, custom callbacks are called with the arguments of
     // name, the full request object (see below), the original callback, and
     // the responses from the API. The responses from the API are handled by the
     // APIRequestHandler, but we need to curry in the other values.
     std::vector<v8::Local<v8::Value>> callback_args;
     if (!custom_callback.IsEmpty()) {
       // TODO(devlin): The |request| object in the JS bindings includes
       // properties for callback, callbackSchema, args, stack, id, and
       // customCallback. Of those, it appears that we only use stack, args, and
       // id (since callback is curried in separately). We may be able to update
-      // bindings to get away from some of those. For now, just pass in an empty
-      // object (most APIs don't rely on it).
-      v8::Local<v8::Object> request = v8::Object::New(isolate);
+      // bindings to get away from some of those. For now, just pass in an
+      // object with the request id.
+      v8::Local<v8::Object> request =
+          gin::DataObjectBuilder(isolate).Set("id", request_id).Build();
       v8::Local<v8::Value> callback_to_pass = callback;
       if (callback_to_pass.IsEmpty())
         callback_to_pass = v8::Undefined(isolate);
       callback_args = {gin::StringToSymbol(isolate, method), request,
                        callback_to_pass};
       callback = custom_callback;
     }
 
     // TODO(devlin): We could *probably* get away with just using an integer
     // here, but it's a little less foolproof. How slow is GenerateGUID? Should
     // we use that instead? It means updating the IPC
     // (ExtensionHostMsg_Request).
     // base::UnguessableToken is another good option.
-    request->request_id = next_request_id_++;
+    request->request_id = request_id;
     request->has_callback = true;
     pending_requests_.insert(std::make_pair(
         request->request_id,
         PendingRequest(isolate, callback, context, callback_args)));
   }
 
   request->has_user_gesture =
       blink::WebUserGestureIndicator::IsProcessingUserGestureThreadSafe();
   request->arguments = std::move(arguments);
   request->method_name = method;
   request->thread = thread;
 
   int id = request->request_id;

[jbroman, 2017/05/24 16:54:38]

To clarify my question, |request_id| isn't used here (or below) -- if it wasn't
populated above, it will be -1. Do we need to increment |next_request_id_| in
the case where |custom_callback.IsEmpty() && callback.IsEmpty()|?

[Devlin, 2017/05/24 17:31:40]

Oh, I see.  Good catch; this was a bug.  Fixed and added a test.

   send_request_.Run(std::move(request), context);
   return id;
 }
 
 void APIRequestHandler::CompleteRequest(int request_id,
                                         const base::ListValue& response_args,
                                         const std::string& error) {
   auto iter = pending_requests_.find(request_id);
   // The request may have been removed if the context was invalidated before a
   // response is ready.
@@ skipping 41 matching lines @@
 }
 
 std::set<int> APIRequestHandler::GetPendingRequestIdsForTesting() const {
   std::set<int> result;
   for (const auto& pair : pending_requests_)
     result.insert(pair.first);
   return result;
 }
 
 }  // namespace extensions