Commit 45696485 authored by Lei Zhang's avatar Lei Zhang Committed by Commit Bot

Implement base::RefCountedSharedMemoryMapping.

This base::RefCountedMemory subclass owns a
base::ReadOnlySharedMemoryMapping instance.

To demonstrate the usefulness of this new class, convert PdfCompositor
from mojo::ScopedSharedBufferHandle to base::ReadOnlySharedMemoryRegion.
Use base::RefCountedSharedMemoryMapping to wrap memory regions and avoid
data copies.

While changing PdfCompositor, turn some DCHECKs into actual checks,
since there is no guarantee shared memory options will always succeed.

BUG=826213

Change-Id: I3135235ea350f58c83606e530b33ed6aa7be3cb5
Reviewed-on: https://chromium-review.googlesource.com/989429Reviewed-by: default avatarTom Sepez <tsepez@chromium.org>
Reviewed-by: default avatarKen Rockot <rockot@chromium.org>
Reviewed-by: default avatarWei Li <weili@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553255}
parent 4725b4fb
......@@ -7,6 +7,7 @@
#include <utility>
#include "base/logging.h"
#include "base/memory/read_only_shared_memory_region.h"
namespace base {
......@@ -95,11 +96,37 @@ RefCountedSharedMemory::RefCountedSharedMemory(
RefCountedSharedMemory::~RefCountedSharedMemory() = default;
const unsigned char* RefCountedSharedMemory::front() const {
return reinterpret_cast<const unsigned char*>(shm_->memory());
return static_cast<const unsigned char*>(shm_->memory());
}
size_t RefCountedSharedMemory::size() const {
return size_;
}
RefCountedSharedMemoryMapping::RefCountedSharedMemoryMapping(
ReadOnlySharedMemoryMapping mapping)
: mapping_(std::move(mapping)), size_(mapping_.size()) {
DCHECK_GT(size_, 0U);
}
RefCountedSharedMemoryMapping::~RefCountedSharedMemoryMapping() = default;
const unsigned char* RefCountedSharedMemoryMapping::front() const {
return static_cast<const unsigned char*>(mapping_.memory());
}
size_t RefCountedSharedMemoryMapping::size() const {
return size_;
}
// static
scoped_refptr<RefCountedSharedMemoryMapping>
RefCountedSharedMemoryMapping::CreateFromWholeRegion(
const ReadOnlySharedMemoryRegion& region) {
ReadOnlySharedMemoryMapping mapping = region.Map();
if (!mapping.IsValid())
return nullptr;
return MakeRefCounted<RefCountedSharedMemoryMapping>(std::move(mapping));
}
} // namespace base
......@@ -15,9 +15,12 @@
#include "base/macros.h"
#include "base/memory/ref_counted.h"
#include "base/memory/shared_memory.h"
#include "base/memory/shared_memory_mapping.h"
namespace base {
class ReadOnlySharedMemoryRegion;
// A generic interface to memory. This object is reference counted because most
// of its subclasses own the data they carry, and this interface needs to
// support heterogeneous containers of these different types of memory.
......@@ -138,8 +141,8 @@ class BASE_EXPORT RefCountedString : public RefCountedMemory {
DISALLOW_COPY_AND_ASSIGN(RefCountedString);
};
// An implementation of RefCountedMemory, where the bytes are stored in shared
// memory.
// An implementation of RefCountedMemory, where the bytes are stored in
// SharedMemory.
class BASE_EXPORT RefCountedSharedMemory : public RefCountedMemory {
public:
// Constructs a RefCountedMemory object by taking ownership of an already
......@@ -159,6 +162,32 @@ class BASE_EXPORT RefCountedSharedMemory : public RefCountedMemory {
DISALLOW_COPY_AND_ASSIGN(RefCountedSharedMemory);
};
// An implementation of RefCountedMemory, where the bytes are stored in
// ReadOnlySharedMemoryMapping.
class BASE_EXPORT RefCountedSharedMemoryMapping : public RefCountedMemory {
public:
// Constructs a RefCountedMemory object by taking ownership of an already
// mapped ReadOnlySharedMemoryMapping object.
explicit RefCountedSharedMemoryMapping(ReadOnlySharedMemoryMapping mapping);
// Convenience method to map all of |region| and take ownership of the
// mapping. Returns an empty scoped_refptr if the map operation fails.
static scoped_refptr<RefCountedSharedMemoryMapping> CreateFromWholeRegion(
const ReadOnlySharedMemoryRegion& region);
// RefCountedMemory:
const unsigned char* front() const override;
size_t size() const override;
private:
~RefCountedSharedMemoryMapping() override;
const ReadOnlySharedMemoryMapping mapping_;
const size_t size_;
DISALLOW_COPY_AND_ASSIGN(RefCountedSharedMemoryMapping);
};
} // namespace base
#endif // BASE_MEMORY_REF_COUNTED_MEMORY_H_
......@@ -8,6 +8,7 @@
#include <utility>
#include "base/memory/read_only_shared_memory_region.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
......@@ -86,6 +87,39 @@ TEST(RefCountedMemoryUnitTest, RefCountedSharedMemory) {
EXPECT_EQ('_', mem->front()[9]);
}
TEST(RefCountedMemoryUnitTest, RefCountedSharedMemoryMapping) {
static const char kData[] = "mem_region_dummy_data";
scoped_refptr<RefCountedSharedMemoryMapping> mem;
{
MappedReadOnlyRegion region =
ReadOnlySharedMemoryRegion::Create(sizeof(kData));
ReadOnlySharedMemoryMapping ro_mapping = region.region.Map();
WritableSharedMemoryMapping rw_mapping = std::move(region.mapping);
ASSERT_TRUE(rw_mapping.IsValid());
memcpy(rw_mapping.memory(), kData, sizeof(kData));
mem = MakeRefCounted<RefCountedSharedMemoryMapping>(std::move(ro_mapping));
}
ASSERT_LE(sizeof(kData), mem->size());
EXPECT_EQ('e', mem->front()[1]);
EXPECT_EQ('m', mem->front()[2]);
EXPECT_EQ('o', mem->front()[8]);
{
MappedReadOnlyRegion region =
ReadOnlySharedMemoryRegion::Create(sizeof(kData));
WritableSharedMemoryMapping rw_mapping = std::move(region.mapping);
ASSERT_TRUE(rw_mapping.IsValid());
memcpy(rw_mapping.memory(), kData, sizeof(kData));
mem = RefCountedSharedMemoryMapping::CreateFromWholeRegion(region.region);
}
ASSERT_LE(sizeof(kData), mem->size());
EXPECT_EQ('_', mem->front()[3]);
EXPECT_EQ('r', mem->front()[4]);
EXPECT_EQ('i', mem->front()[7]);
}
TEST(RefCountedMemoryUnitTest, Equals) {
std::string s1("same");
scoped_refptr<RefCountedMemory> mem1 = RefCountedString::TakeString(&s1);
......
......@@ -268,28 +268,30 @@ void PrintPreviewMessageHandler::OnCompositePdfPageDone(
int page_number,
int request_id,
mojom::PdfCompositor::Status status,
mojo::ScopedSharedBufferHandle handle) {
base::ReadOnlySharedMemoryRegion region) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
if (status != mojom::PdfCompositor::Status::SUCCESS) {
DLOG(ERROR) << "Compositing pdf failed with error " << status;
return;
}
NotifyUIPreviewPageReady(page_number, request_id,
GetDataFromMojoHandle(std::move(handle)));
NotifyUIPreviewPageReady(
page_number, request_id,
base::RefCountedSharedMemoryMapping::CreateFromWholeRegion(region));
}
void PrintPreviewMessageHandler::OnCompositePdfDocumentDone(
int page_count,
int request_id,
mojom::PdfCompositor::Status status,
mojo::ScopedSharedBufferHandle handle) {
base::ReadOnlySharedMemoryRegion region) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
if (status != mojom::PdfCompositor::Status::SUCCESS) {
DLOG(ERROR) << "Compositing pdf failed with error " << status;
return;
}
NotifyUIPreviewDocumentReady(page_count, request_id,
GetDataFromMojoHandle(std::move(handle)));
NotifyUIPreviewDocumentReady(
page_count, request_id,
base::RefCountedSharedMemoryMapping::CreateFromWholeRegion(region));
}
bool PrintPreviewMessageHandler::OnMessageReceived(
......
......@@ -7,6 +7,7 @@
#include "base/compiler_specific.h"
#include "base/macros.h"
#include "base/memory/read_only_shared_memory_region.h"
#include "base/memory/ref_counted_memory.h"
#include "base/memory/weak_ptr.h"
#include "components/printing/service/public/interfaces/pdf_compositor.mojom.h"
......@@ -88,11 +89,11 @@ class PrintPreviewMessageHandler
void OnCompositePdfPageDone(int page_number,
int request_id,
mojom::PdfCompositor::Status status,
mojo::ScopedSharedBufferHandle handle);
base::ReadOnlySharedMemoryRegion region);
void OnCompositePdfDocumentDone(int page_count,
int request_id,
mojom::PdfCompositor::Status status,
mojo::ScopedSharedBufferHandle handle);
base::ReadOnlySharedMemoryRegion region);
base::WeakPtrFactory<PrintPreviewMessageHandler> weak_ptr_factory_;
......
......@@ -284,7 +284,7 @@ bool PrintViewManagerBase::PrintJobHasDocument(int cookie) {
void PrintViewManagerBase::OnComposePdfDone(
const PrintHostMsg_DidPrintDocument_Params& params,
mojom::PdfCompositor::Status status,
mojo::ScopedSharedBufferHandle handle) {
base::ReadOnlySharedMemoryRegion region) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
if (status != mojom::PdfCompositor::Status::SUCCESS) {
DLOG(ERROR) << "Compositing pdf failed with error " << status;
......@@ -294,14 +294,11 @@ void PrintViewManagerBase::OnComposePdfDone(
if (!print_job_->document())
return;
std::unique_ptr<base::SharedMemory> shared_buf =
GetShmFromMojoHandle(std::move(handle));
if (!shared_buf)
scoped_refptr<base::RefCountedSharedMemoryMapping> data =
base::RefCountedSharedMemoryMapping::CreateFromWholeRegion(region);
if (!data)
return;
size_t size = shared_buf->mapped_size();
auto data = base::MakeRefCounted<base::RefCountedSharedMemory>(
std::move(shared_buf), size);
PrintDocument(data, params.page_size, params.content_area,
params.physical_offsets);
}
......
......@@ -8,6 +8,7 @@
#include <memory>
#include "base/macros.h"
#include "base/memory/read_only_shared_memory_region.h"
#include "base/memory/scoped_refptr.h"
#include "base/memory/weak_ptr.h"
#include "base/strings/string16.h"
......@@ -18,7 +19,6 @@
#include "components/printing/service/public/interfaces/pdf_compositor.mojom.h"
#include "content/public/browser/notification_observer.h"
#include "content/public/browser/notification_registrar.h"
#include "mojo/public/cpp/system/platform_handle.h"
#include "printing/buildflags/buildflags.h"
struct PrintHostMsg_DidPrintDocument_Params;
......@@ -117,7 +117,7 @@ class PrintViewManagerBase : public content::NotificationObserver,
// IPC message handlers for service.
void OnComposePdfDone(const PrintHostMsg_DidPrintDocument_Params& params,
mojom::PdfCompositor::Status status,
mojo::ScopedSharedBufferHandle handle);
base::ReadOnlySharedMemoryRegion region);
// Helpers for PrintForPrintPreview();
#if BUILDFLAG(ENABLE_PRINT_PREVIEW)
......
......@@ -201,7 +201,7 @@ void PrintCompositeClient::DoCompositeDocumentToPdf(
void PrintCompositeClient::OnDidCompositePageToPdf(
printing::mojom::PdfCompositor::CompositePageToPdfCallback callback,
printing::mojom::PdfCompositor::Status status,
mojo::ScopedSharedBufferHandle handle) {
base::ReadOnlySharedMemoryRegion region) {
// Due to https://crbug.com/742517, we can not add and use COUNT for enums in
// mojo.
UMA_HISTOGRAM_ENUMERATION(
......@@ -209,14 +209,14 @@ void PrintCompositeClient::OnDidCompositePageToPdf(
static_cast<int32_t>(
printing::mojom::PdfCompositor::Status::COMPOSTING_FAILURE) +
1);
std::move(callback).Run(status, std::move(handle));
std::move(callback).Run(status, std::move(region));
}
void PrintCompositeClient::OnDidCompositeDocumentToPdf(
int document_cookie,
printing::mojom::PdfCompositor::CompositeDocumentToPdfCallback callback,
printing::mojom::PdfCompositor::Status status,
mojo::ScopedSharedBufferHandle handle) {
base::ReadOnlySharedMemoryRegion region) {
RemoveCompositeRequest(document_cookie);
// Clear all stored printed subframes.
printed_subframes_.erase(document_cookie);
......@@ -228,7 +228,7 @@ void PrintCompositeClient::OnDidCompositeDocumentToPdf(
static_cast<int32_t>(
printing::mojom::PdfCompositor::Status::COMPOSTING_FAILURE) +
1);
std::move(callback).Run(status, std::move(handle));
std::move(callback).Run(status, std::move(region));
}
ContentToFrameMap PrintCompositeClient::ConvertContentInfoMap(
......
......@@ -95,13 +95,13 @@ class PrintCompositeClient
void OnDidCompositePageToPdf(
printing::mojom::PdfCompositor::CompositePageToPdfCallback callback,
printing::mojom::PdfCompositor::Status status,
mojo::ScopedSharedBufferHandle handle);
base::ReadOnlySharedMemoryRegion region);
void OnDidCompositeDocumentToPdf(
int document_cookie,
printing::mojom::PdfCompositor::CompositeDocumentToPdfCallback callback,
printing::mojom::PdfCompositor::Status status,
mojo::ScopedSharedBufferHandle handle);
base::ReadOnlySharedMemoryRegion region);
// Get the request or create a new one if none exists.
// Since printed pages always share content with it document, they share the
......
......@@ -4,7 +4,9 @@
#include "components/printing/service/pdf_compositor_impl.h"
#include <algorithm>
#include <tuple>
#include <utility>
#include "base/logging.h"
#include "base/memory/shared_memory_handle.h"
......@@ -197,7 +199,7 @@ mojom::PdfCompositor::Status PdfCompositorImpl::CompositeToPdf(
base::Optional<uint32_t> page_num,
std::unique_ptr<base::SharedMemory> shared_mem,
const ContentToFrameMap& subframe_content_map,
mojo::ScopedSharedBufferHandle* handle) {
base::ReadOnlySharedMemoryRegion* region) {
DeserializationContext subframes =
GetDeserializationContext(subframe_content_map);
......@@ -226,14 +228,24 @@ mojom::PdfCompositor::Status PdfCompositorImpl::CompositeToPdf(
}
doc->close();
*handle = mojo::SharedBufferHandle::Create(wstream.bytesWritten());
DCHECK((*handle).is_valid());
mojo::ScopedSharedBufferMapping mapping =
(*handle)->Map(wstream.bytesWritten());
DCHECK(mapping);
wstream.copyToAndReset(mapping.get());
const size_t size = wstream.bytesWritten();
mojo::ScopedSharedBufferHandle handle =
mojo::SharedBufferHandle::Create(size);
mojo::ScopedSharedBufferHandle readonly_handle;
if (handle.is_valid()) {
mojo::ScopedSharedBufferMapping mapping = handle->Map(size);
if (mapping) {
wstream.copyToAndReset(mapping.get());
readonly_handle =
handle->Clone(mojo::SharedBufferHandle::AccessMode::READ_ONLY);
}
}
if (!readonly_handle.is_valid()) {
DLOG(ERROR) << "CompositeToPdf: Cannot create new shared memory region.";
return mojom::PdfCompositor::Status::HANDLE_MAP_ERROR;
}
*region = mojo::UnwrapReadOnlySharedMemoryRegion(std::move(readonly_handle));
return mojom::PdfCompositor::Status::SUCCESS;
}
......@@ -282,11 +294,11 @@ void PdfCompositorImpl::FulfillRequest(
std::unique_ptr<base::SharedMemory> serialized_content,
const ContentToFrameMap& subframe_content_map,
CompositeToPdfCallback callback) {
mojo::ScopedSharedBufferHandle handle;
base::ReadOnlySharedMemoryRegion region;
auto status =
CompositeToPdf(frame_guid, page_num, std::move(serialized_content),
subframe_content_map, &handle);
std::move(callback).Run(status, std::move(handle));
subframe_content_map, &region);
std::move(callback).Run(status, std::move(region));
}
PdfCompositorImpl::FrameContentInfo::FrameContentInfo(
......
......@@ -13,12 +13,12 @@
#include "base/containers/flat_map.h"
#include "base/containers/flat_set.h"
#include "base/macros.h"
#include "base/memory/read_only_shared_memory_region.h"
#include "base/memory/ref_counted_memory.h"
#include "base/memory/shared_memory.h"
#include "base/optional.h"
#include "components/printing/service/public/cpp/pdf_service_mojo_types.h"
#include "components/printing/service/public/interfaces/pdf_compositor.mojom.h"
#include "mojo/public/cpp/system/buffer.h"
#include "services/service_manager/public/cpp/service_context_ref.h"
#include "third_party/skia/include/core/SkPicture.h"
#include "third_party/skia/include/core/SkRefCnt.h"
......@@ -57,7 +57,7 @@ class PdfCompositorImpl : public mojom::PdfCompositor {
// mojom::PdfCompositor::CompositeDocumentToPdfCallback.
using CompositeToPdfCallback =
base::OnceCallback<void(PdfCompositor::Status,
mojo::ScopedSharedBufferHandle)>;
base::ReadOnlySharedMemoryRegion)>;
// Make this function virtual so tests can override it.
virtual void FulfillRequest(
......@@ -164,7 +164,7 @@ class PdfCompositorImpl : public mojom::PdfCompositor {
base::Optional<uint32_t> page_num,
std::unique_ptr<base::SharedMemory> shared_mem,
const ContentToFrameMap& subframe_content_map,
mojo::ScopedSharedBufferHandle* handle);
base::ReadOnlySharedMemoryRegion* region);
// Composite the content of a subframe.
sk_sp<SkPicture> CompositeSubframe(uint64_t frame_guid);
......
......@@ -3,6 +3,7 @@
// found in the LICENSE file.
#include <memory>
#include <utility>
#include "base/callback.h"
#include "base/run_loop.h"
......@@ -52,7 +53,7 @@ class PdfCompositorImplTest : public testing::Test {
}
void OnCompositeToPdfCallback(mojom::PdfCompositor::Status status,
mojo::ScopedSharedBufferHandle handle) {
base::ReadOnlySharedMemoryRegion region) {
// A stub for testing, no implementation.
}
......
......@@ -3,6 +3,7 @@
// found in the LICENSE file.
#include <memory>
#include <utility>
#include "base/callback.h"
#include "base/files/file_path.h"
......@@ -88,12 +89,13 @@ class PdfCompositorServiceTest : public service_manager::test::ServiceTest {
PdfCompositorServiceTest() : ServiceTest("pdf_compositor_service_unittest") {}
~PdfCompositorServiceTest() override {}
MOCK_METHOD1(CallbackOnCompositeSuccess, void(mojo::SharedBufferHandle));
MOCK_METHOD1(CallbackOnCompositeSuccess,
void(const base::ReadOnlySharedMemoryRegion&));
MOCK_METHOD1(CallbackOnCompositeStatus, void(mojom::PdfCompositor::Status));
void OnCompositeToPdfCallback(mojom::PdfCompositor::Status status,
mojo::ScopedSharedBufferHandle handle) {
base::ReadOnlySharedMemoryRegion region) {
if (status == mojom::PdfCompositor::Status::SUCCESS)
CallbackOnCompositeSuccess(handle.get());
CallbackOnCompositeSuccess(region);
else
CallbackOnCompositeStatus(status);
run_loop_->Quit();
......
......@@ -6,7 +6,6 @@
#include <utility>
#include "base/memory/ref_counted_memory.h"
#include "base/memory/shared_memory.h"
#include "mojo/public/cpp/system/platform_handle.h"
......@@ -35,16 +34,4 @@ std::unique_ptr<base::SharedMemory> GetShmFromMojoHandle(
return shm;
}
scoped_refptr<base::RefCountedMemory> GetDataFromMojoHandle(
mojo::ScopedSharedBufferHandle handle) {
std::unique_ptr<base::SharedMemory> shm =
GetShmFromMojoHandle(std::move(handle));
if (!shm)
return nullptr;
size_t size = shm->mapped_size();
return base::MakeRefCounted<base::RefCountedSharedMemory>(std::move(shm),
size);
}
} // namespace printing
......@@ -7,11 +7,9 @@
#include <memory>
#include "base/memory/scoped_refptr.h"
#include "mojo/public/cpp/system/buffer.h"
namespace base {
class RefCountedMemory;
class SharedMemory;
} // namespace base
......@@ -20,9 +18,6 @@ namespace printing {
std::unique_ptr<base::SharedMemory> GetShmFromMojoHandle(
mojo::ScopedSharedBufferHandle handle);
scoped_refptr<base::RefCountedMemory> GetDataFromMojoHandle(
mojo::ScopedSharedBufferHandle handle);
} // namespace printing
#endif // COMPONENTS_PRINTING_SERVICE_PUBLIC_CPP_PDF_SERVICE_MOJO_UTILS_H_
......@@ -9,6 +9,7 @@ mojom("interfaces") {
"pdf_compositor.mojom",
]
public_deps = [
"//mojo/public/mojom/base",
"//url/mojom:url_mojom_gurl",
]
}
......@@ -4,6 +4,7 @@
module printing.mojom;
import "mojo/public/mojom/base/shared_memory.mojom";
import "url/mojom/url.mojom";
const string kServiceName = "pdf_compositor";
......@@ -44,14 +45,16 @@ interface PdfCompositor {
CompositePageToPdf(uint64 frame_guid, uint32 page_num,
handle<shared_buffer> sk_handle,
map<uint32, uint64> subframe_content_info)
=> (Status status, handle<shared_buffer>? pdf_handle);
=> (Status status,
mojo_base.mojom.ReadOnlySharedMemoryRegion? pdf_region);
// Requests to composite the entire document and convert it into a PDF file.
// All the arguments carry the same meaning as CompositePageToPdf() above,
// except this call doesn't have |page_num|.
CompositeDocumentToPdf(uint64 frame_guid, handle<shared_buffer> sk_handle,
map<uint32, uint64> subframe_content_info)
=> (Status status, handle<shared_buffer>? pdf_handle);
=> (Status status,
mojo_base.mojom.ReadOnlySharedMemoryRegion? pdf_region);
// Sets the URL which is committed in the main frame of the WebContents,
// for use in crash diagnosis.
......
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