Skip to content

GitLab

T
tangled
Commit ee3f412f authored by Kevin Marshall's avatar Kevin Marshall Committed by Commit Bot
Browse files
Options

Fuchsia: Fix PostMessage message buffering, add & fix FrameImpl tests. 

* Fixes issue with an on-receive callback not being unbound, causing
  the incoming message buffer to be drained immediately.
* Add browser tests to verify buffering behavior.
* Add browser tests to verify proper origin enforcement for
  Frame::PostMessage.
* Make the AsyncValueReceiver interface a bit nicer to use.
* Minor cleanups for other FrameImpl browser tests.

Bug: 893236
Change-Id: I521e9316ccf9195133a940be670556a882cb1044
Reviewed-on: https://chromium-review.googlesource.com/c/1351777
Commit-Queue: Kevin Marshall <kmarshall@chromium.org>
Reviewed-by: default avatarWez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612812}
parent d260cfb5
Hide whitespace changes
Inline Side-by-side
Showing with 160 additions and 96 deletions
webrunner/browser/frame_impl_browsertest.cc
View file @ ee3f412f
......@@ -101,8 +101,6 @@ IN_PROC_BROWSER_TEST_F(FrameImplTest, NavigateFrame) {
frame->GetNavigationController(controller.NewRequest());
CheckLoadUrl(url::kAboutBlankURL, url::kAboutBlankURL, controller.get());
frame.Unbind();
}
IN_PROC_BROWSER_TEST_F(FrameImplTest, NavigateDataFrame) {
......@@ -112,8 +110,6 @@ IN_PROC_BROWSER_TEST_F(FrameImplTest, NavigateDataFrame) {
frame->GetNavigationController(controller.NewRequest());
CheckLoadUrl(kDataUrl, kDataUrl, controller.get());
frame.Unbind();
}
IN_PROC_BROWSER_TEST_F(FrameImplTest, FrameDeletedBeforeContext) {
......@@ -416,8 +412,6 @@ IN_PROC_BROWSER_TEST_F(FrameImplTest, ExecuteJavaScriptImmediate) {
Field(&NavigationDetails::url, IsSet()))))
.WillOnce(InvokeWithoutArgs([&run_loop]() { run_loop.Quit(); }));
CheckRunWithTimeout(&run_loop);
frame.Unbind();
}
IN_PROC_BROWSER_TEST_F(FrameImplTest, ExecuteJavaScriptOnLoad) {
......@@ -437,8 +431,6 @@ IN_PROC_BROWSER_TEST_F(FrameImplTest, ExecuteJavaScriptOnLoad) {
chromium::web::NavigationControllerPtr controller;
frame->GetNavigationController(controller.NewRequest());
CheckLoadUrl(url.spec(), "hello", controller.get());
frame.Unbind();
}
IN_PROC_BROWSER_TEST_F(FrameImplTest, ExecuteJavascriptOnLoadWrongOrigin) {
......@@ -462,8 +454,6 @@ IN_PROC_BROWSER_TEST_F(FrameImplTest, ExecuteJavascriptOnLoadWrongOrigin) {
// script with a replacement title.
CheckLoadUrl(url.spec(), "Welcome to Stan the Offline Dino's Homepage",
controller.get());
frame.Unbind();
}
IN_PROC_BROWSER_TEST_F(FrameImplTest, ExecuteJavaScriptOnLoadWildcardOrigin) {
......@@ -491,8 +481,6 @@ IN_PROC_BROWSER_TEST_F(FrameImplTest, ExecuteJavaScriptOnLoadWildcardOrigin) {
// still be picked up by the wildcard.
GURL alt_url = embedded_test_server()->GetURL("localhost", kDynamicTitlePath);
CheckLoadUrl(alt_url.spec(), "hello", controller.get());
frame.Unbind();
}
// Test that consecutive scripts are executed in order by computing a cumulative
......@@ -517,8 +505,6 @@ IN_PROC_BROWSER_TEST_F(FrameImplTest, ExecuteMultipleJavaScriptsOnLoad) {
chromium::web::NavigationControllerPtr controller;
frame->GetNavigationController(controller.NewRequest());
CheckLoadUrl(url.spec(), "hello there", controller.get());
frame.Unbind();
}
// Test that we can inject scripts before and after RenderFrame creation.
......@@ -550,8 +536,6 @@ IN_PROC_BROWSER_TEST_F(FrameImplTest, ExecuteOnLoadEarlyAndLateRegistrations) {
// Navigate back and see if both scripts are working.
CheckLoadUrl(url.spec(), "hello there", controller.get());
frame.Unbind();
}
IN_PROC_BROWSER_TEST_F(FrameImplTest, ExecuteJavaScriptBadEncoding) {
......@@ -577,8 +561,6 @@ IN_PROC_BROWSER_TEST_F(FrameImplTest, ExecuteJavaScriptBadEncoding) {
run_loop.Quit();
});
CheckRunWithTimeout(&run_loop);
frame.Unbind();
}
// Verifies that a Frame will handle navigation observer disconnection events
......@@ -756,31 +738,6 @@ std::string ReadFromBufferOrDie(const fuchsia::mem::Buffer& buffer) {
return output;
}
// Stores an asynchronously generated value for later retrieval.
template <typename T>
class AsyncValueReceiver {
public:
explicit AsyncValueReceiver(
base::RepeatingClosure on_capture = base::DoNothing())
: on_capture_(std::move(on_capture)) {}
~AsyncValueReceiver() = default;
fit::function<void(T)> GetReceiveClosure() {
return [this](T value) {
captured_ = std::move(value);
on_capture_.Run();
};
}
T& captured() { return captured_; }
private:
T captured_;
base::RepeatingClosure on_capture_;
DISALLOW_COPY_AND_ASSIGN(AsyncValueReceiver<T>);
};
IN_PROC_BROWSER_TEST_F(FrameImplTest, PostMessage) {
chromium::web::FramePtr frame = CreateFrame();
......@@ -794,9 +751,9 @@ IN_PROC_BROWSER_TEST_F(FrameImplTest, PostMessage) {
chromium::web::WebMessage message;
message.data = MemBufferFromString(kPage1Path);
AsyncValueReceiver<bool> post_result;
Promise<bool> post_result;
frame->PostMessage(std::move(message), post_message_url.GetOrigin().spec(),
post_result.GetReceiveClosure());
post_result.GetReceiveCallback());
base::RunLoop run_loop;
EXPECT_CALL(navigation_observer_,
MockableOnNavigationStateChanged(
......@@ -804,9 +761,7 @@ IN_PROC_BROWSER_TEST_F(FrameImplTest, PostMessage) {
Field(&NavigationDetails::url, IsSet()))))
.WillOnce(InvokeWithoutArgs([&run_loop]() { run_loop.Quit(); }));
CheckRunWithTimeout(&run_loop);
EXPECT_TRUE(post_result.captured());
frame.Unbind();
EXPECT_TRUE(*post_result);
}
// Send a MessagePort to the content, then perform bidirectional messaging
......@@ -828,32 +783,28 @@ IN_PROC_BROWSER_TEST_F(FrameImplTest, PostMessagePassMessagePort) {
std::make_unique<chromium::web::OutgoingTransferable>();
msg.outgoing_transfer->set_message_port(message_port.NewRequest());
msg.data = MemBufferFromString("hi");
AsyncValueReceiver<bool> post_result;
Promise<bool> post_result;
frame->PostMessage(std::move(msg), post_message_url.GetOrigin().spec(),
post_result.GetReceiveClosure());
post_result.GetReceiveCallback());
base::RunLoop run_loop;
AsyncValueReceiver<chromium::web::WebMessage> receiver(
run_loop.QuitClosure());
message_port->ReceiveMessage(receiver.GetReceiveClosure());
Promise<chromium::web::WebMessage> receiver(run_loop.QuitClosure());
message_port->ReceiveMessage(receiver.GetReceiveCallback());
CheckRunWithTimeout(&run_loop);
EXPECT_EQ("got_port", ReadFromBufferOrDie(receiver.captured().data));
EXPECT_EQ("got_port", ReadFromBufferOrDie(receiver->data));
}
{
msg.data = MemBufferFromString("ping");
AsyncValueReceiver<bool> post_result;
message_port->PostMessage(std::move(msg), post_result.GetReceiveClosure());
Promise<bool> post_result;
message_port->PostMessage(std::move(msg), post_result.GetReceiveCallback());
base::RunLoop run_loop;
AsyncValueReceiver<chromium::web::WebMessage> receiver(
run_loop.QuitClosure());
message_port->ReceiveMessage(receiver.GetReceiveClosure());
Promise<chromium::web::WebMessage> receiver(run_loop.QuitClosure());
message_port->ReceiveMessage(receiver.GetReceiveCallback());
CheckRunWithTimeout(&run_loop);
EXPECT_EQ("ack ping", ReadFromBufferOrDie(receiver.captured().data));
EXPECT_TRUE(post_result.captured());
EXPECT_EQ("ack ping", ReadFromBufferOrDie(receiver->data));
EXPECT_TRUE(*post_result);
}
frame.Unbind();
}
// Send a MessagePort to the content, then perform bidirectional messaging
......@@ -875,17 +826,16 @@ IN_PROC_BROWSER_TEST_F(FrameImplTest, PostMessageMessagePortDisconnected) {
std::make_unique<chromium::web::OutgoingTransferable>();
msg.outgoing_transfer->set_message_port(message_port.NewRequest());
msg.data = MemBufferFromString("hi");
AsyncValueReceiver<bool> post_result;
Promise<bool> post_result;
frame->PostMessage(std::move(msg), post_message_url.GetOrigin().spec(),
post_result.GetReceiveClosure());
post_result.GetReceiveCallback());
base::RunLoop run_loop;
AsyncValueReceiver<chromium::web::WebMessage> receiver(
run_loop.QuitClosure());
message_port->ReceiveMessage(receiver.GetReceiveClosure());
Promise<chromium::web::WebMessage> receiver(run_loop.QuitClosure());
message_port->ReceiveMessage(receiver.GetReceiveCallback());
CheckRunWithTimeout(&run_loop);
EXPECT_EQ("got_port", ReadFromBufferOrDie(receiver.captured().data));
EXPECT_TRUE(post_result.captured());
EXPECT_EQ("got_port", ReadFromBufferOrDie(receiver->data));
EXPECT_TRUE(*post_result);
}
// Navigating off-page should tear down the Mojo channel, thereby causing the
......@@ -893,12 +843,10 @@ IN_PROC_BROWSER_TEST_F(FrameImplTest, PostMessageMessagePortDisconnected) {
{
base::RunLoop run_loop;
message_port.set_error_handler(
[&run_loop](zx_status_t status) { run_loop.Quit(); });
[&run_loop](zx_status_t) { run_loop.Quit(); });
controller->LoadUrl(url::kAboutBlankURL, nullptr);
CheckRunWithTimeout(&run_loop);
}
frame.Unbind();
}
// Send a MessagePort to the content, and through that channel, receive a
......@@ -922,39 +870,112 @@ IN_PROC_BROWSER_TEST_F(FrameImplTest, PostMessageUseContentProvidedPort) {
std::make_unique<chromium::web::OutgoingTransferable>();
msg.outgoing_transfer->set_message_port(message_port.NewRequest());
msg.data = MemBufferFromString("hi");
AsyncValueReceiver<bool> post_result;
frame->PostMessage(std::move(msg), "*", post_result.GetReceiveClosure());
Promise<bool> post_result;
frame->PostMessage(std::move(msg), "*", post_result.GetReceiveCallback());
base::RunLoop run_loop;
AsyncValueReceiver<chromium::web::WebMessage> receiver(
run_loop.QuitClosure());
message_port->ReceiveMessage(receiver.GetReceiveClosure());
Promise<chromium::web::WebMessage> receiver(run_loop.QuitClosure());
message_port->ReceiveMessage(receiver.GetReceiveCallback());
CheckRunWithTimeout(&run_loop);
EXPECT_EQ("got_port", ReadFromBufferOrDie(receiver.captured().data));
incoming_message_port =
receiver.captured().incoming_transfer->message_port().Bind();
EXPECT_TRUE(post_result.captured());
EXPECT_EQ("got_port", ReadFromBufferOrDie(receiver->data));
incoming_message_port = receiver->incoming_transfer->message_port().Bind();
EXPECT_TRUE(*post_result);
}
{
AsyncValueReceiver<bool> post_result;
// Get the content to send three 'ack ping' messages, which will accumulate in
// the MessagePortImpl buffer.
for (int i = 0; i < 3; ++i) {
base::RunLoop run_loop;
Promise<bool> post_result(run_loop.QuitClosure());
msg.data = MemBufferFromString("ping");
incoming_message_port->PostMessage(std::move(msg),
post_result.GetReceiveClosure());
post_result.GetReceiveCallback());
run_loop.Run();
EXPECT_TRUE(*post_result);
}
// Receive another acknowledgement from content on a side channel to ensure
// that all the "ack pings" are ready to be consumed.
{
chromium::web::MessagePortPtr ack_message_port;
chromium::web::WebMessage msg;
msg.outgoing_transfer =
std::make_unique<chromium::web::OutgoingTransferable>();
msg.outgoing_transfer->set_message_port(ack_message_port.NewRequest());
msg.data = MemBufferFromString("hi");
// Quit the runloop only after we've received a WebMessage AND a PostMessage
// result.
Promise<bool> post_result;
frame->PostMessage(std::move(msg), "*", post_result.GetReceiveCallback());
base::RunLoop run_loop;
AsyncValueReceiver<chromium::web::WebMessage> receiver(
run_loop.QuitClosure());
incoming_message_port->ReceiveMessage(receiver.GetReceiveClosure());
Promise<chromium::web::WebMessage> receiver(run_loop.QuitClosure());
ack_message_port->ReceiveMessage(receiver.GetReceiveCallback());
CheckRunWithTimeout(&run_loop);
EXPECT_EQ("ack ping", ReadFromBufferOrDie(receiver.captured().data));
EXPECT_TRUE(post_result.captured());
EXPECT_EQ("got_port", ReadFromBufferOrDie(receiver->data));
EXPECT_TRUE(*post_result);
}
frame.Unbind();
// Pull the three 'ack ping's from the buffer.
for (int i = 0; i < 3; ++i) {
base::RunLoop run_loop;
Promise<chromium::web::WebMessage> receiver(run_loop.QuitClosure());
incoming_message_port->ReceiveMessage(receiver.GetReceiveCallback());
CheckRunWithTimeout(&run_loop);
EXPECT_EQ("ack ping", ReadFromBufferOrDie(receiver->data));
}
}
// TODO BEFORE SUBMITTING(kmarshall): bad origin tests, multiple buffered
// messages, perhaps a proof-of-concept test with injected bindings *and*
// message i/o.
IN_PROC_BROWSER_TEST_F(FrameImplTest, PostMessageBadOriginDropped) {
chromium::web::FramePtr frame = CreateFrame();
ASSERT_TRUE(embedded_test_server()->Start());
GURL post_message_url(embedded_test_server()->GetURL("/message_port.html"));
chromium::web::NavigationControllerPtr controller;
frame->GetNavigationController(controller.NewRequest());
CheckLoadUrl(post_message_url.spec(), "messageport", controller.get());
chromium::web::MessagePortPtr bad_origin_incoming_message_port;
chromium::web::WebMessage msg;
// PostMessage() to invalid origins should be ignored. We pass in a
// MessagePort but nothing should happen to it.
chromium::web::MessagePortPtr unused_message_port;
msg.outgoing_transfer =
std::make_unique<chromium::web::OutgoingTransferable>();
msg.outgoing_transfer->set_message_port(unused_message_port.NewRequest());
msg.data = MemBufferFromString("bad origin, bad!");
Promise<bool> unused_post_result;
frame->PostMessage(std::move(msg), "https://example.com",
unused_post_result.GetReceiveCallback());
Promise<chromium::web::WebMessage> unused_message_read;
bad_origin_incoming_message_port->ReceiveMessage(
unused_message_read.GetReceiveCallback());
// PostMessage() with a valid origin should succeed.
// Verify it by looking for an ack message on the MessagePort we passed in.
// Since message events are handled in order, observing the result of this
// operation will verify whether the previous PostMessage() was received but
// discarded.
chromium::web::MessagePortPtr incoming_message_port;
chromium::web::MessagePortPtr message_port;
msg.outgoing_transfer =
std::make_unique<chromium::web::OutgoingTransferable>();
msg.outgoing_transfer->set_message_port(message_port.NewRequest());
msg.data = MemBufferFromString("good origin");
Promise<bool> post_result;
frame->PostMessage(std::move(msg), "*", post_result.GetReceiveCallback());
base::RunLoop run_loop;
Promise<chromium::web::WebMessage> receiver(run_loop.QuitClosure());
message_port->ReceiveMessage(receiver.GetReceiveCallback());
CheckRunWithTimeout(&run_loop);
EXPECT_EQ("got_port", ReadFromBufferOrDie(receiver->data));
incoming_message_port = receiver->incoming_transfer->message_port().Bind();
EXPECT_TRUE(*post_result);
// Verify that the first PostMessage() call wasn't handled.
EXPECT_FALSE(unused_message_read.has_value());
}
} // namespace webrunner
webrunner/browser/message_port_impl.cc
View file @ ee3f412f
......@@ -112,7 +112,7 @@ MessagePortImpl::MessagePortImpl(mojo::ScopedMessagePipeHandle mojo_port)
connector_->set_connection_error_handler(
base::BindOnce(&MessagePortImpl::OnDisconnected, base::Unretained(this)));
binding_.set_error_handler([this](zx_status_t status) {
if (status != ZX_OK)
if (status != ZX_OK && status != ZX_ERR_PEER_CLOSED)
ZX_DLOG(INFO, status) << "Disconnected";
OnDisconnected();
......@@ -135,6 +135,7 @@ void MessagePortImpl::PostMessage(chromium::web::WebMessage message,
return;
}
CHECK(connector_->Accept(&mojo_message));
callback(true);
}
void MessagePortImpl::ReceiveMessage(ReceiveMessageCallback callback) {
......@@ -148,7 +149,8 @@ void MessagePortImpl::MaybeDeliverToClient() {
if (!pending_client_read_cb_ || message_queue_.empty())
return;
std::move(pending_client_read_cb_)(std::move(message_queue_.front()));
base::ResetAndReturn(&pending_client_read_cb_)(
std::move(message_queue_.front()));
message_queue_.pop_front();
}
......
webrunner/browser/test_common.h
View file @ ee3f412f
......@@ -5,6 +5,7 @@
#ifndef WEBRUNNER_BROWSER_TEST_COMMON_H_
#define WEBRUNNER_BROWSER_TEST_COMMON_H_
#include "base/optional.h"
#include "content/public/browser/web_contents_observer.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "webrunner/fidl/chromium/web/cpp/fidl.h"
......@@ -45,6 +46,46 @@ class MockNavigationObserver : public chromium::web::NavigationEventObserver,
DISALLOW_COPY_AND_ASSIGN(MockNavigationObserver);
};
// Stores an asynchronously generated value for later retrieval, optionally
// invoking a callback on value receipt for controlling test flow.
//
// The value can be read by using the dereference (*) or arrow (->) operators.
// Values must first be received before they can be accessed. Dereferencing a
// value before it is set will produce a CHECK violation.
template <typename T>
class Promise {
public:
explicit Promise(base::RepeatingClosure on_capture = base::DoNothing())
: on_capture_(std::move(on_capture)) {}
~Promise() = default;
// Returns a fit::function<> which will receive and store a value T.
fit::function<void(T)> GetReceiveCallback() {
return [this](T value) {
captured_ = std::move(value);
on_capture_.Run();
};
}
bool has_value() const { return captured_.has_value(); };
T& operator*() {
CHECK(captured_.has_value());
return *captured_;
}
T* operator->() {
CHECK(captured_.has_value());
return &*captured_;
}
private:
base::Optional<T> captured_;
base::RepeatingClosure on_capture_;
DISALLOW_COPY_AND_ASSIGN(Promise<T>);
};
} // namespace webrunner
#endif // WEBRUNNER_BROWSER_TEST_COMMON_H_
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment