chromium/src
0
Fork 0
Code Activity

IPC: Do not read message attachments more than once.

Browse Source 
This CL eliminates attachment water mark resetting and all code that
uses it. This is done in order to allow passing objects owning
descriptors through the IPC.

This breaks support of reading messages with attachments from the
IPC::TestSink that were already read before. This was used only in
PrintRenderFrameHelperPreviewTests which were fixed.

Note to sheriffs: This CL may break some untested code in NaCl; please
revert in that case.

Change-Id: Ib6f02f067a96c6c34a8f5404ebc867459e9d79cc
Reviewed-on: https://chromium-review.googlesource.com/998102
Commit-Queue: Alexandr Ilin <alexilin@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Erik Chen <erikchen@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550542}
This commit is contained in:
Alexandr Ilin
2018-04-13 08:10:20 +00:00
committed by Commit Bot
parent 080a06cefb
commit d4c1d1d638
5 changed files with 21 additions and 52 deletions

7
components/printing/test/print_mock_render_thread.cc

@ -108,6 +108,8 @@ void PrintMockRenderThread::OnDidPreviewPage(
const PrintHostMsg_DidPreviewPage_Params& params) {
DCHECK_GE(params.page_number, printing::FIRST_PAGE_INDEX);
print_preview_pages_remaining_--;
print_preview_pages_.emplace_back(params.page_number,
params.content.data_size);
}
void PrintMockRenderThread::OnCheckForCancel(int32_t preview_ui_id,
@ -210,4 +212,9 @@ void PrintMockRenderThread::set_print_preview_cancel_page_number(int page) {
int PrintMockRenderThread::print_preview_pages_remaining() const {
return print_preview_pages_remaining_;
}
const std::vector<std::pair<int, uint32_t>>&
PrintMockRenderThread::print_preview_pages() const {
return print_preview_pages_;
}
#endif // BUILDFLAG(ENABLE_PRINTING)

6
components/printing/test/print_mock_render_thread.h

@ -57,6 +57,9 @@ class PrintMockRenderThread : public content::MockRenderThread {
// Get the number of pages to generate for print preview.
int print_preview_pages_remaining() const;
// Get a vector of print preview pages.
const std::vector<std::pair<int, uint32_t>>& print_preview_pages() const;
#endif
private:
@ -100,6 +103,9 @@ class PrintMockRenderThread : public content::MockRenderThread {
// Number of pages to generate for print preview.
int print_preview_pages_remaining_;
// Vector of <page_number, content_data_size> that were previewed.
std::vector<std::pair<int, uint32_t>> print_preview_pages_;
#endif
scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_;

21
components/printing/test/print_render_frame_helper_browsertest.cc

@ -221,11 +221,6 @@ class PrintRenderFrameHelperTestBase : public content::RenderViewTest {
PrintHostMsg_DidPrintDocument::ID);
bool did_print = !!print_msg;
ASSERT_EQ(expect_printed, did_print);
if (did_print) {
PrintHostMsg_DidPrintDocument::Param post_did_print_page_param;
PrintHostMsg_DidPrintDocument::Read(print_msg,
&post_did_print_page_param);
}
}
#if BUILDFLAG(ENABLE_BASIC_PRINTING)
@ -652,17 +647,11 @@ class MAYBE_PrintRenderFrameHelperPreviewTest
void VerifyDidPreviewPage(bool expect_generated, int page_number) {
bool msg_found = false;
uint32_t data_size = 0;
size_t msg_count = render_thread_->sink().message_count();
for (size_t i = 0; i < msg_count; ++i) {
const IPC::Message* msg = render_thread_->sink().GetMessageAt(i);
if (msg->type() == PrintHostMsg_DidPreviewPage::ID) {
PrintHostMsg_DidPreviewPage::Param page_param;
PrintHostMsg_DidPreviewPage::Read(msg, &page_param);
if (std::get<0>(page_param).page_number == page_number) {
msg_found = true;
data_size = std::get<0>(page_param).content.data_size;
break;
}
for (const auto& preview : print_render_thread_->print_preview_pages()) {
if (preview.first == page_number) {
msg_found = true;
data_size = preview.second;
break;
}
}
EXPECT_EQ(expect_generated, msg_found) << "For page " << page_number;

9
ipc/ipc_message_attachment_set.cc

@ -111,14 +111,9 @@ scoped_refptr<MessageAttachment> MessageAttachmentSet::GetAttachmentAt(
//
// So we can either track of the use of each descriptor in a bitset, or we
// can enforce that we walk the indexes strictly in order.
//
// There's one more wrinkle: When logging messages, we may reparse them. So
// we have an exception: When the consumed_descriptor_highwater_ is at the
// end of the array and index 0 is requested, we reset the highwater value.
// TODO(morrita): This is absurd. This "wringle" disallow to introduce clearer
// ownership model. Only client is NaclIPCAdapter. See crbug.com/415294
if (index == 0 && consumed_descriptor_highwater_ == size()) {
consumed_descriptor_highwater_ = 0;
DLOG(WARNING) << "Attempted to double-read a message attachment, "
"returning a nullptr";
}
if (index != consumed_descriptor_highwater_)

30
ipc/ipc_message_attachment_set_posix_unittest.cc

@ -91,8 +91,6 @@ TEST(MessageAttachmentSet, MaxSize) {
TEST(MessageAttachmentSet, WalkInOrder) {
scoped_refptr<MessageAttachmentSet> set(new MessageAttachmentSet);
// TODO(morrita): This test is wrong. TakeDescriptorAt() shouldn't be
// used to retrieve borrowed descriptors. That never happens in production.
ASSERT_TRUE(
set->AddAttachment(new internal::PlatformFileAttachment(kFDBase)));
ASSERT_TRUE(
@ -103,6 +101,7 @@ TEST(MessageAttachmentSet, WalkInOrder) {
ASSERT_EQ(GetFdAt(set.get(), 0), kFDBase);
ASSERT_EQ(GetFdAt(set.get(), 1), kFDBase + 1);
ASSERT_EQ(GetFdAt(set.get(), 2), kFDBase + 2);
ASSERT_FALSE(set->GetAttachmentAt(0));
set->CommitAllDescriptors();
}
@ -110,8 +109,6 @@ TEST(MessageAttachmentSet, WalkInOrder) {
TEST(MessageAttachmentSet, WalkWrongOrder) {
scoped_refptr<MessageAttachmentSet> set(new MessageAttachmentSet);
// TODO(morrita): This test is wrong. TakeDescriptorAt() shouldn't be
// used to retrieve borrowed descriptors. That never happens in production.
ASSERT_TRUE(
set->AddAttachment(new internal::PlatformFileAttachment(kFDBase)));
ASSERT_TRUE(
@ -125,31 +122,6 @@ TEST(MessageAttachmentSet, WalkWrongOrder) {
set->CommitAllDescriptors();
}
TEST(MessageAttachmentSet, WalkCycle) {
scoped_refptr<MessageAttachmentSet> set(new MessageAttachmentSet);
// TODO(morrita): This test is wrong. TakeDescriptorAt() shouldn't be
// used to retrieve borrowed descriptors. That never happens in production.
ASSERT_TRUE(
set->AddAttachment(new internal::PlatformFileAttachment(kFDBase)));
ASSERT_TRUE(
set->AddAttachment(new internal::PlatformFileAttachment(kFDBase + 1)));
ASSERT_TRUE(
set->AddAttachment(new internal::PlatformFileAttachment(kFDBase + 2)));
ASSERT_EQ(GetFdAt(set.get(), 0), kFDBase);
ASSERT_EQ(GetFdAt(set.get(), 1), kFDBase + 1);
ASSERT_EQ(GetFdAt(set.get(), 2), kFDBase + 2);
ASSERT_EQ(GetFdAt(set.get(), 0), kFDBase);
ASSERT_EQ(GetFdAt(set.get(), 1), kFDBase + 1);
ASSERT_EQ(GetFdAt(set.get(), 2), kFDBase + 2);
ASSERT_EQ(GetFdAt(set.get(), 0), kFDBase);
ASSERT_EQ(GetFdAt(set.get(), 1), kFDBase + 1);
ASSERT_EQ(GetFdAt(set.get(), 2), kFDBase + 2);
set->CommitAllDescriptors();
}
#if defined(OS_ANDROID)
#define MAYBE_DontClose DISABLED_DontClose
#else