[Extensions + IPC] Allow passing a single base::Value result over IPC
Add IPC::ParamTraits<base::Value> to allow passing a single base::Value
(instead of a base::ListValue) over IPC. Update
ExtensionHostMsg_ExecuteCodeFinished to incorporate this.
Bug: None
Change-Id: Iff39cc07868caab668b2d663effe8e29f1e0ff67
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2653989
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#849389}
diff --git a/extensions/browser/script_executor.cc b/extensions/browser/script_executor.cc
index 3ad3f8e..783f3d4 100644
--- a/extensions/browser/script_executor.cc
+++ b/extensions/browser/script_executor.cc
@@ -191,12 +191,11 @@
}
// Handles the ExecuteCodeFinished message.
- // TODO(devlin): Change this to a base::Value, rather than a ListValue.
void OnExecuteCodeFinished(content::RenderFrameHost* render_frame_host,
int request_id,
const std::string& error,
const GURL& on_url,
- const base::ListValue& result_list) {
+ const base::Optional<base::Value>& result) {
DCHECK_EQ(request_id_, request_id);
DCHECK(!pending_render_frames_.empty());
size_t erased = base::Erase(pending_render_frames_, render_frame_host);
@@ -210,8 +209,8 @@
// TODO(devlin): Do we need to trust the renderer for the URL here? Is there
// a risk of the frame having navigated since the injection happened?
frame_result.url = on_url;
- if (!result_list.GetList().empty())
- frame_result.value = result_list.GetList()[0].Clone();
+ if (result.has_value())
+ frame_result.value = result->Clone();
results_.push_back(std::move(frame_result));
diff --git a/extensions/common/extension_messages.h b/extensions/common/extension_messages.h
index bb327d2..55b75ff5 100644
--- a/extensions/common/extension_messages.h
+++ b/extensions/common/extension_messages.h
@@ -44,6 +44,7 @@
#include "extensions/common/user_script.h"
#include "extensions/common/view_type.h"
#include "ipc/ipc_message_macros.h"
+#include "ipc/ipc_message_utils.h"
#include "ui/accessibility/ax_param_traits.h"
#include "url/gurl.h"
#include "url/origin.h"
@@ -849,7 +850,10 @@
int /* request id */,
std::string /* error; empty implies success */,
GURL /* URL of the code executed on. May be empty if unsuccessful. */,
- base::ListValue /* result of the script */)
+ base::Optional<base::Value> /* result of the script, if any. We use a
+ base::Optional<> here to differentiate
+ between no result (such as in the case of an
+ error) and a null result. */)
// Sent from the renderer to the browser to notify that content scripts are
// running in the renderer that the IPC originated from.
diff --git a/extensions/renderer/programmatic_script_injector.cc b/extensions/renderer/programmatic_script_injector.cc
index 19b8026..6d13b30 100644
--- a/extensions/renderer/programmatic_script_injector.cc
+++ b/extensions/renderer/programmatic_script_injector.cc
@@ -145,9 +145,10 @@
std::unique_ptr<base::Value> execution_result,
UserScript::RunLocation run_location,
content::RenderFrame* render_frame) {
- DCHECK(results_.empty());
- if (execution_result)
- results_.Append(std::move(execution_result));
+ DCHECK(!result_.has_value());
+ if (execution_result) {
+ result_ = base::Value::FromUniquePtrValue(std::move(execution_result));
+ }
Finish(std::string(), render_frame);
}
@@ -195,10 +196,9 @@
// injecting scripts. Don't respond if it was (the browser side watches for
// frame deletions so nothing is left hanging).
if (render_frame) {
- render_frame->Send(
- new ExtensionHostMsg_ExecuteCodeFinished(
- render_frame->GetRoutingID(), params_->request_id,
- error, url_, results_));
+ render_frame->Send(new ExtensionHostMsg_ExecuteCodeFinished(
+ render_frame->GetRoutingID(), params_->request_id, error, url_,
+ result_));
}
}
diff --git a/extensions/renderer/programmatic_script_injector.h b/extensions/renderer/programmatic_script_injector.h
index 5e2e2913..10109c7c 100644
--- a/extensions/renderer/programmatic_script_injector.h
+++ b/extensions/renderer/programmatic_script_injector.h
@@ -8,6 +8,7 @@
#include <memory>
#include "base/macros.h"
+#include "base/optional.h"
#include "base/values.h"
#include "extensions/renderer/script_injection.h"
#include "url/gurl.h"
@@ -77,8 +78,8 @@
// is used to provide user-friendly messages.
std::string origin_for_about_error_;
- // The results of the script execution.
- base::ListValue results_;
+ // The result of the script execution.
+ base::Optional<base::Value> result_;
// Whether or not this script injection has finished.
bool finished_;
diff --git a/ipc/ipc_message_unittest.cc b/ipc/ipc_message_unittest.cc
index 5d9da31..63503a3c 100644
--- a/ipc/ipc_message_unittest.cc
+++ b/ipc/ipc_message_unittest.cc
@@ -65,6 +65,44 @@
EXPECT_FALSE(iter.ReadString16(&vs16));
}
+TEST(IPCMessageTest, Value) {
+ auto expect_value_equals = [](const base::Value& input) {
+ IPC::Message msg(1, 2, IPC::Message::PRIORITY_NORMAL);
+ IPC::WriteParam(&msg, input);
+
+ base::Value output;
+ base::PickleIterator iter(msg);
+ EXPECT_TRUE(IPC::ReadParam(&msg, &iter, &output)) << input;
+ EXPECT_EQ(input, output);
+ };
+
+ expect_value_equals(base::Value("foo"));
+ expect_value_equals(base::Value(42));
+ expect_value_equals(base::Value(0.07));
+ expect_value_equals(base::Value(true));
+ expect_value_equals(base::Value(base::Value::BlobStorage({'a', 'b', 'c'})));
+
+ {
+ base::Value dict(base::Value::Type::DICTIONARY);
+ dict.SetIntKey("key1", 42);
+ dict.SetStringKey("key2", "hi");
+ expect_value_equals(dict);
+ }
+ {
+ base::Value list(base::Value::Type::LIST);
+ list.Append(42);
+ list.Append("hello");
+ expect_value_equals(list);
+ }
+
+ // Also test the corrupt case.
+ IPC::Message bad_msg(1, 2, IPC::Message::PRIORITY_NORMAL);
+ bad_msg.WriteInt(99);
+ base::PickleIterator iter(bad_msg);
+ base::Value output;
+ EXPECT_FALSE(IPC::ReadParam(&bad_msg, &iter, &output));
+}
+
TEST(IPCMessageTest, ListValue) {
base::ListValue input;
input.AppendDouble(42.42);
diff --git a/ipc/ipc_message_utils.cc b/ipc/ipc_message_utils.cc
index ec2d60e..8d6931b 100644
--- a/ipc/ipc_message_utils.cc
+++ b/ipc/ipc_message_utils.cc
@@ -214,6 +214,11 @@
if (!ReadParam(m, iter, &type))
return false;
+ constexpr int kMinValueType = static_cast<int>(base::Value::Type::NONE);
+ constexpr int kMaxValueType = static_cast<int>(base::Value::Type::LIST);
+ if (type > kMaxValueType || type < kMinValueType)
+ return false;
+
switch (static_cast<base::Value::Type>(type)) {
case base::Value::Type::NONE:
*value = base::Value();
@@ -1184,6 +1189,22 @@
l->append(json);
}
+void ParamTraits<base::Value>::Write(base::Pickle* m, const param_type& p) {
+ WriteValue(m, &p, 0);
+}
+
+bool ParamTraits<base::Value>::Read(const base::Pickle* m,
+ base::PickleIterator* iter,
+ param_type* r) {
+ return ReadValue(m, iter, r, 0);
+}
+
+void ParamTraits<base::Value>::Log(const param_type& p, std::string* l) {
+ std::string json;
+ base::JSONWriter::Write(p, &json);
+ l->append(json);
+}
+
void ParamTraits<base::NullableString16>::Write(base::Pickle* m,
const param_type& p) {
WriteParam(m, p.string());
diff --git a/ipc/ipc_message_utils.h b/ipc/ipc_message_utils.h
index a94bf3d..b86eac44 100644
--- a/ipc/ipc_message_utils.h
+++ b/ipc/ipc_message_utils.h
@@ -753,6 +753,16 @@
};
template <>
+struct COMPONENT_EXPORT(IPC) ParamTraits<base::Value> {
+ typedef base::Value param_type;
+ static void Write(base::Pickle* m, const param_type& p);
+ static bool Read(const base::Pickle* m,
+ base::PickleIterator* iter,
+ param_type* r);
+ static void Log(const param_type& p, std::string* l);
+};
+
+template <>
struct COMPONENT_EXPORT(IPC) ParamTraits<base::NullableString16> {
typedef base::NullableString16 param_type;
static void Write(base::Pickle* m, const param_type& p);