Skip to content

GitLab

T
tangled
Commit 2093bb8f authored by Colin Blundell's avatar Colin Blundell Committed by Commit Bot
Browse files
Options

Revert "Print with dead subframes for pdf composition" 

This reverts commit d20c0123.

Reason for revert: The SubframeUnavailableBeforePrint test added in this CL is flaky (see crbug.com/817068).

Original change's description:
> Print with dead subframes for pdf composition
>
> Handle two cases with dead subframes for pdf composition:
> -- When a web page has a dead subframe prior to printing, we need to
> detect the liveness of the subframe, and avoid requesting printing for
> such frame;
> -- If after we request printing a subframe, the subframe dies, we add
> monitoring for render frame's closed event to check whether it is one
> of our pending ones. If so, notify pdf compositor service about that.
>
> We add an interface in pdf compositor service to be notified about
> the frame's unavailability for either of the above cases.
>
> In this CL, we also add a map to record the subframes that are already
> printed and use it to avoid printing the same ones repeatedly.
>
> BUG=814086
>
> Change-Id: Ibd69dd21a6498a5c2784dfe892bc5803e84fa6f0
> Reviewed-on: https://chromium-review.googlesource.com/932018
> Commit-Queue: Wei Li <weili@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Lei Zhang <thestig@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#539291}

TBR=dcheng@chromium.org,thestig@chromium.org,weili@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

TBR=dcheng@chromium.org

Bug: 814086
Change-Id: I2062517c3f955523618be23a52ad507c4da3de90
Reviewed-on: https://chromium-review.googlesource.com/941201
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: default avatarColin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539818}
parent d103f115
Hide whitespace changes
Inline Side-by-side
Showing with 26 additions and 233 deletions
chrome/browser/printing/print_browsertest.cc
View file @ 2093bb8f
......@@ -102,33 +102,6 @@ class TestPrintFrameContentMsgFilter : public content::BrowserMessageFilter {
base::RepeatingClosure msg_callback_;
};
class KillPrintFrameContentMsgFilter : public content::BrowserMessageFilter {
public:
explicit KillPrintFrameContentMsgFilter(content::RenderProcessHost* rph)
: content::BrowserMessageFilter(PrintMsgStart), rph_(rph) {}
bool OnMessageReceived(const IPC::Message& message) override {
// Only handle PrintHostMsg_DidPrintFrameContent message.
bool handled = true;
IPC_BEGIN_MESSAGE_MAP(KillPrintFrameContentMsgFilter, message)
IPC_MESSAGE_HANDLER(PrintHostMsg_DidPrintFrameContent, KillRenderProcess)
IPC_MESSAGE_UNHANDLED(handled = false)
IPC_END_MESSAGE_MAP()
return handled;
}
private:
~KillPrintFrameContentMsgFilter() override {}
void KillRenderProcess(int document_cookie,
const PrintHostMsg_DidPrintContent_Params& param) {
base::ScopedAllowBaseSyncPrimitivesForTesting allow_sync_primitives;
rph_->Shutdown(0, true);
}
content::RenderProcessHost* rph_;
};
} // namespace
class PrintBrowserTest : public InProcessBrowserTest {
......@@ -198,17 +171,6 @@ class PrintBrowserTest : public InProcessBrowserTest {
std::unique_ptr<base::RunLoop> run_loop_;
};
class SitePerProcessPrintBrowserTest : public PrintBrowserTest {
public:
SitePerProcessPrintBrowserTest() {}
~SitePerProcessPrintBrowserTest() override {}
// content::BrowserTestBase
void SetUpCommandLine(base::CommandLine* command_line) override {
content::IsolateAllSitesForTesting(command_line);
}
};
class IsolateOriginsPrintBrowserTest : public PrintBrowserTest {
public:
static constexpr char kIsolatedSite[] = "b.com";
......@@ -380,58 +342,6 @@ IN_PROC_BROWSER_TEST_F(PrintBrowserTest, PrintSubframeABA) {
WaitUntilMessagesReceived();
}
// Printing a web page with a dead subframe for site per process should succeed.
// This test passes whenever the print preview is rendered. This should not be
// a timed out test which indicates the print preview hung.
IN_PROC_BROWSER_TEST_F(SitePerProcessPrintBrowserTest,
SubframeUnavailableBeforePrint) {
ASSERT_TRUE(embedded_test_server()->Started());
GURL url(
embedded_test_server()->GetURL("/printing/content_with_iframe.html"));
ui_test_utils::NavigateToURL(browser(), url);
content::WebContents* original_contents =
browser()->tab_strip_model()->GetActiveWebContents();
ASSERT_EQ(2u, original_contents->GetAllFrames().size());
content::RenderFrameHost* test_frame = original_contents->GetAllFrames()[1];
ASSERT_TRUE(test_frame);
ASSERT_TRUE(test_frame->IsRenderFrameLive());
// Wait for the renderer to be down.
content::RenderProcessHostWatcher render_process_watcher(
test_frame->GetProcess(),
content::RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);
// Shutdown the subframe.
ASSERT_TRUE(test_frame->GetProcess()->Shutdown(0, false));
render_process_watcher.Wait();
ASSERT_FALSE(test_frame->IsRenderFrameLive());
PrintAndWaitUntilPreviewIsReady(/*print_only_selection=*/false);
}
// If a subframe dies during printing, the page printing should still succeed.
// This test passes whenever the print preview is rendered. This should not be
// a timed out test which indicates the print preview hung.
IN_PROC_BROWSER_TEST_F(SitePerProcessPrintBrowserTest,
SubframeUnavailableDuringPrint) {
ASSERT_TRUE(embedded_test_server()->Started());
GURL url(
embedded_test_server()->GetURL("/printing/content_with_iframe.html"));
ui_test_utils::NavigateToURL(browser(), url);
content::WebContents* original_contents =
browser()->tab_strip_model()->GetActiveWebContents();
ASSERT_EQ(2u, original_contents->GetAllFrames().size());
content::RenderFrameHost* subframe = original_contents->GetAllFrames()[1];
ASSERT_TRUE(subframe);
auto* subframe_rph = subframe->GetProcess();
auto filter =
base::MakeRefCounted<KillPrintFrameContentMsgFilter>(subframe_rph);
subframe_rph->AddFilter(filter.get());
PrintAndWaitUntilPreviewIsReady(/*print_only_selection=*/false);
}
// Printing preview a web page with an iframe from an isolated origin.
// This test passes whenever the print preview is rendered. This should not be
// a timed out test which indicates the print preview hung or crash.
......
components/printing/browser/print_composite_client.cc
View file @ 2093bb8f
......@@ -8,7 +8,6 @@
#include "base/bind.h"
#include "base/memory/shared_memory_handle.h"
#include "base/stl_util.h"
#include "components/printing/common/print_messages.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/render_frame_host.h"
......@@ -48,22 +47,6 @@ bool PrintCompositeClient::OnMessageReceived(
return handled;
}
void PrintCompositeClient::RenderFrameDeleted(
content::RenderFrameHost* render_frame_host) {
auto frame_guid = GenerateFrameGuid(render_frame_host->GetProcess()->GetID(),
render_frame_host->GetRoutingID());
auto iter = pending_subframe_cookies_.find(frame_guid);
if (iter != pending_subframe_cookies_.end()) {
// When a subframe we are expecting is deleted, we should notify pdf
// compositor service.
for (auto doc_cookie : iter->second) {
auto& compositor = GetCompositeRequest(doc_cookie);
compositor->NotifyUnavailableSubframe(frame_guid);
}
pending_subframe_cookies_.erase(iter);
}
}
void PrintCompositeClient::OnDidPrintFrameContent(
content::RenderFrameHost* render_frame_host,
int document_cookie,
......@@ -72,6 +55,7 @@ void PrintCompositeClient::OnDidPrintFrameContent(
// is done here. Most of it will be directly forwarded to pdf compositor
// service.
auto& compositor = GetCompositeRequest(document_cookie);
DCHECK(compositor.is_bound());
mojo::ScopedSharedBufferHandle buffer_handle = mojo::WrapSharedMemoryHandle(
params.metafile_data_handle, params.data_size,
......@@ -82,12 +66,6 @@ void PrintCompositeClient::OnDidPrintFrameContent(
frame_guid, std::move(buffer_handle),
ConvertContentInfoMap(web_contents(), render_frame_host,
params.subframe_content_info));
// Update our internal states about this frame.
pending_subframe_cookies_[frame_guid].erase(document_cookie);
if (pending_subframe_cookies_[frame_guid].empty())
pending_subframe_cookies_.erase(frame_guid);
printed_subframes_[document_cookie].insert(frame_guid);
}
void PrintCompositeClient::PrintCrossProcessSubframe(
......@@ -97,33 +75,9 @@ void PrintCompositeClient::PrintCrossProcessSubframe(
PrintMsg_PrintFrame_Params params;
params.printable_area = rect;
params.document_cookie = document_cookie;
uint64_t frame_guid = GenerateFrameGuid(subframe_host->GetProcess()->GetID(),
subframe_host->GetRoutingID());
if (subframe_host->IsRenderFrameLive()) {
auto subframe_iter = printed_subframes_.find(document_cookie);
if (subframe_iter != printed_subframes_.end() &&
base::ContainsKey(subframe_iter->second, frame_guid)) {
// If this frame is already printed, no need to print again.
return;
}
auto cookie_iter = pending_subframe_cookies_.find(frame_guid);
if (cookie_iter != pending_subframe_cookies_.end() &&
base::ContainsKey(cookie_iter->second, document_cookie)) {
// If this frame is being printed, no need to print again.
return;
}
// Send the request to the destination frame.
subframe_host->Send(
new PrintMsg_PrintFrameContent(subframe_host->GetRoutingID(), params));
pending_subframe_cookies_[frame_guid].insert(document_cookie);
} else {
// When the subframe is dead, no need to send message,
// just notify the service.
auto& compositor = GetCompositeRequest(document_cookie);
compositor->NotifyUnavailableSubframe(frame_guid);
}
// Send the request to the destination frame.
subframe_host->Send(
new PrintMsg_PrintFrameContent(subframe_host->GetRoutingID(), params));
}
void PrintCompositeClient::DoCompositePageToPdf(
......@@ -195,8 +149,6 @@ void PrintCompositeClient::OnDidCompositeDocumentToPdf(
printing::mojom::PdfCompositor::Status status,
mojo::ScopedSharedBufferHandle handle) {
RemoveCompositeRequest(document_cookie);
// Clear all stored printed subframes.
printed_subframes_.erase(document_cookie);
std::move(callback).Run(status, std::move(handle));
}
......@@ -227,10 +179,8 @@ ContentToFrameMap PrintCompositeClient::ConvertContentInfoMap(
mojom::PdfCompositorPtr& PrintCompositeClient::GetCompositeRequest(int cookie) {
auto iter = compositor_map_.find(cookie);
if (iter != compositor_map_.end()) {
DCHECK(iter->second.is_bound());
if (iter != compositor_map_.end())
return iter->second;
}
auto iterator =
compositor_map_.emplace(cookie, CreateCompositeRequest()).first;
......
components/printing/browser/print_composite_client.h
View file @ 2093bb8f
......@@ -8,7 +8,6 @@
#include <map>
#include <memory>
#include "base/containers/flat_set.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"
......@@ -36,7 +35,6 @@ class PrintCompositeClient
// content::WebContentsObserver
bool OnMessageReceived(const IPC::Message& message,
content::RenderFrameHost* render_frame_host) override;
void RenderFrameDeleted(content::RenderFrameHost* render_frame_host) override;
// IPC message handler.
void OnDidPrintFrameContent(
......@@ -125,14 +123,6 @@ class PrintCompositeClient
// requests.
std::map<int, mojom::PdfCompositorPtr> compositor_map_;
// Stores the mapping between render frame's global unique id and document
// cookies that requested such frame.
std::map<uint64_t, base::flat_set<int>> pending_subframe_cookies_;
// Stores the mapping between document cookie and all the printed subframes
// for that document.
std::map<int, base::flat_set<uint64_t>> printed_subframes_;
DISALLOW_COPY_AND_ASSIGN(PrintCompositeClient);
};
......
components/printing/service/pdf_compositor_impl.cc
View file @ 2093bb8f
......@@ -8,7 +8,6 @@
#include "base/logging.h"
#include "base/memory/shared_memory_handle.h"
#include "base/stl_util.h"
#include "components/printing/service/public/cpp/pdf_service_mojo_types.h"
#include "components/printing/service/public/cpp/pdf_service_mojo_utils.h"
#include "mojo/public/cpp/system/platform_handle.h"
......@@ -27,21 +26,6 @@ PdfCompositorImpl::PdfCompositorImpl(
PdfCompositorImpl::~PdfCompositorImpl() = default;
void PdfCompositorImpl::NotifyUnavailableSubframe(uint64_t frame_guid) {
// Add this frame into the map.
DCHECK(!base::ContainsKey(frame_info_map_, frame_guid));
auto& frame_info =
frame_info_map_.emplace(frame_guid, std::make_unique<FrameInfo>())
.first->second;
frame_info->composited = true;
// Set content to be nullptr so it will be replaced by an empty picture during
// deserialization of its parent.
frame_info->content = nullptr;
// Update the requests in case any of them might be waiting for this frame.
UpdateRequestsWithSubframeInfo(frame_guid, std::vector<uint64_t>());
}
void PdfCompositorImpl::AddSubframeContent(
uint64_t frame_guid,
mojo::ScopedSharedBufferHandle serialized_content,
......@@ -72,33 +56,6 @@ void PdfCompositorImpl::AddSubframeContent(
pending_subframes.push_back(subframe_guid);
}
// Update the requests in case any of them is waiting for this frame.
UpdateRequestsWithSubframeInfo(frame_guid, pending_subframes);
}
void PdfCompositorImpl::CompositePageToPdf(
uint64_t frame_guid,
uint32_t page_num,
mojo::ScopedSharedBufferHandle serialized_content,
const ContentToFrameMap& subframe_content_map,
mojom::PdfCompositor::CompositePageToPdfCallback callback) {
HandleCompositionRequest(frame_guid, page_num, std::move(serialized_content),
subframe_content_map, std::move(callback));
}
void PdfCompositorImpl::CompositeDocumentToPdf(
uint64_t frame_guid,
mojo::ScopedSharedBufferHandle serialized_content,
const ContentToFrameMap& subframe_content_map,
mojom::PdfCompositor::CompositeDocumentToPdfCallback callback) {
HandleCompositionRequest(frame_guid, base::nullopt,
std::move(serialized_content), subframe_content_map,
std::move(callback));
}
void PdfCompositorImpl::UpdateRequestsWithSubframeInfo(
uint64_t frame_guid,
const std::vector<uint64_t>& pending_subframes) {
// Check for each request's pending list.
for (auto it = requests_.begin(); it != requests_.end();) {
auto& request = *it;
......@@ -124,6 +81,26 @@ void PdfCompositorImpl::UpdateRequestsWithSubframeInfo(
}
}
void PdfCompositorImpl::CompositePageToPdf(
uint64_t frame_guid,
uint32_t page_num,
mojo::ScopedSharedBufferHandle serialized_content,
const ContentToFrameMap& subframe_content_map,
mojom::PdfCompositor::CompositePageToPdfCallback callback) {
HandleCompositionRequest(frame_guid, page_num, std::move(serialized_content),
subframe_content_map, std::move(callback));
}
void PdfCompositorImpl::CompositeDocumentToPdf(
uint64_t frame_guid,
mojo::ScopedSharedBufferHandle serialized_content,
const ContentToFrameMap& subframe_content_map,
mojom::PdfCompositor::CompositeDocumentToPdfCallback callback) {
HandleCompositionRequest(frame_guid, base::nullopt,
std::move(serialized_content), subframe_content_map,
std::move(callback));
}
bool PdfCompositorImpl::IsReadyToComposite(
uint64_t frame_guid,
const ContentToFrameMap& subframe_content_map,
......
components/printing/service/pdf_compositor_impl.h
View file @ 2093bb8f
......@@ -33,7 +33,6 @@ class PdfCompositorImpl : public mojom::PdfCompositor {
~PdfCompositorImpl() override;
// mojom::PdfCompositor
void NotifyUnavailableSubframe(uint64_t frame_guid) override;
void AddSubframeContent(
uint64_t frame_guid,
mojo::ScopedSharedBufferHandle serialized_content,
......@@ -128,12 +127,6 @@ class PdfCompositorImpl : public mojom::PdfCompositor {
CompositeToPdfCallback callback;
};
// Check whether any request is waiting for the specific subframe, if so,
// update its dependecy with the subframe's pending child frames.
void UpdateRequestsWithSubframeInfo(
uint64_t frame_guid,
const std::vector<uint64_t>& pending_subframes);
// Check whether the frame with a list of subframe content is ready to
// composite. If not, return all unavailable frames' ids in
// |pending_subframes|.
......
components/printing/service/pdf_compositor_impl_unittest.cc
View file @ 2093bb8f
......@@ -324,25 +324,4 @@ TEST_F(PdfCompositorImplTest, MultiRequestsDepOrder) {
std::move(subframe_content_map));
}
TEST_F(PdfCompositorImplTest, NotifyUnavailableSubframe) {
MockPdfCompositorImpl impl;
// Page 0 with frame 3 has content 1, which refers to frame 8.
// When the content is not available, the request is not fulfilled.
ContentToFrameMap subframe_content_map;
subframe_content_map[1u] = 8u;
EXPECT_CALL(impl, OnFulfillRequest(testing::_, testing::_)).Times(0);
impl.CompositePageToPdf(
3u, 0, mojo::SharedBufferHandle::Create(10),
std::move(subframe_content_map),
base::BindOnce(&PdfCompositorImplTest::OnCompositeToPdfCallback,
base::Unretained(this)));
testing::Mock::VerifyAndClearExpectations(&impl);
// Notifies that frame 8's unavailable, the previous request should be
// fulfilled.
EXPECT_CALL(impl, OnFulfillRequest(3u, 0)).Times(1);
impl.NotifyUnavailableSubframe(8u);
testing::Mock::VerifyAndClearExpectations(&impl);
}
} // namespace printing
components/printing/service/public/interfaces/pdf_compositor.mojom
View file @ 2093bb8f
......@@ -15,12 +15,6 @@ interface PdfCompositor {
COMPOSTING_FAILURE,
};
// Notifies that a subframe is unavailable, such as the render frame process
// hosting it crashed or terminated. The subframe will be composited with no
// content in the composited result.
// |frame_guid| is this subframe's global unique id.
NotifyUnavailableSubframe(uint64 frame_guid);
// Add the content of a subframe for composition.
// |frame_guid| is this subframe's global unique id.
// |serialized_content| points to a buffer of a serialized Skia picture which
......
printing/common/pdf_metafile_utils.cc
View file @ 2093bb8f
......@@ -77,7 +77,7 @@ sk_sp<SkPicture> DeserializeOopPicture(const void* data,
auto* context = reinterpret_cast<DeserializationContext*>(ctx);
auto iter = context->find(pic_id);
if (iter == context->end() || !iter->second) {
if (iter == context->end()) {
// When we don't have the out-of-process picture available, we return
// an empty picture. Returning a nullptr will cause the deserialization
// crash.
......
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