New issue
Advanced search Search tips

Issue 814086 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug



Sign in to add a comment

Monitor frames' health to not wait indefinitely for pdf compositor service

Project Member Reported by weili@chromium.org, Feb 21 2018

Issue description

pdf compositor service waits on all frames content. Adds a monitoring to stop waiting on dead frames to avoid waiting indefinitely.

 

Comment 1 by creis@chromium.org, Feb 21 2018

Cc: creis@chromium.org
Components: Internals>Printing Internals>Sandbox>SiteIsolation
Labels: -Pri-3 M-66 OS-Chrome OS-Linux OS-Mac OS-Windows Pri-1
Thanks!  The effect here is that pages with sad iframes (due to crashed or killed out-of-process iframes) cannot be printed, since the dialog never gets past the "Loading preview..." stage.

Can we target M66?  Hopefully it's just a matter for checking RenderFrameHost::IsRenderFrameLive() before asking a frame to print.  Not sure if we want to try to print the sad iframe image or just leave it blank.  (Guessing blank is preferable?)

Comment 2 by weili@chromium.org, Feb 21 2018

Status: Started (was: Assigned)
sure, working on it
Project Member

Comment 3 by bugdroid1@chromium.org, Feb 26 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d20c01231eb8acf9d38bc3aa37a108f2cee5f8d2

commit d20c01231eb8acf9d38bc3aa37a108f2cee5f8d2
Author: Wei Li <weili@chromium.org>
Date: Mon Feb 26 22:30:13 2018

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}
[modify] https://crrev.com/d20c01231eb8acf9d38bc3aa37a108f2cee5f8d2/chrome/browser/printing/print_browsertest.cc
[modify] https://crrev.com/d20c01231eb8acf9d38bc3aa37a108f2cee5f8d2/components/printing/browser/print_composite_client.cc
[modify] https://crrev.com/d20c01231eb8acf9d38bc3aa37a108f2cee5f8d2/components/printing/browser/print_composite_client.h
[modify] https://crrev.com/d20c01231eb8acf9d38bc3aa37a108f2cee5f8d2/components/printing/service/pdf_compositor_impl.cc
[modify] https://crrev.com/d20c01231eb8acf9d38bc3aa37a108f2cee5f8d2/components/printing/service/pdf_compositor_impl.h
[modify] https://crrev.com/d20c01231eb8acf9d38bc3aa37a108f2cee5f8d2/components/printing/service/pdf_compositor_impl_unittest.cc
[modify] https://crrev.com/d20c01231eb8acf9d38bc3aa37a108f2cee5f8d2/components/printing/service/public/interfaces/pdf_compositor.mojom
[modify] https://crrev.com/d20c01231eb8acf9d38bc3aa37a108f2cee5f8d2/printing/common/pdf_metafile_utils.cc

Comment 4 by weili@chromium.org, Feb 27 2018

Status: Fixed (was: Started)
Project Member

Comment 5 by bugdroid1@chromium.org, Feb 28 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2093bb8f1a80cdafbf24832d8662dbabaa92311a

commit 2093bb8f1a80cdafbf24832d8662dbabaa92311a
Author: Colin Blundell <blundell@chromium.org>
Date: Wed Feb 28 14:21:02 2018

Revert "Print with dead subframes for pdf composition"

This reverts commit d20c01231eb8acf9d38bc3aa37a108f2cee5f8d2.

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: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539818}
[modify] https://crrev.com/2093bb8f1a80cdafbf24832d8662dbabaa92311a/chrome/browser/printing/print_browsertest.cc
[modify] https://crrev.com/2093bb8f1a80cdafbf24832d8662dbabaa92311a/components/printing/browser/print_composite_client.cc
[modify] https://crrev.com/2093bb8f1a80cdafbf24832d8662dbabaa92311a/components/printing/browser/print_composite_client.h
[modify] https://crrev.com/2093bb8f1a80cdafbf24832d8662dbabaa92311a/components/printing/service/pdf_compositor_impl.cc
[modify] https://crrev.com/2093bb8f1a80cdafbf24832d8662dbabaa92311a/components/printing/service/pdf_compositor_impl.h
[modify] https://crrev.com/2093bb8f1a80cdafbf24832d8662dbabaa92311a/components/printing/service/pdf_compositor_impl_unittest.cc
[modify] https://crrev.com/2093bb8f1a80cdafbf24832d8662dbabaa92311a/components/printing/service/public/interfaces/pdf_compositor.mojom
[modify] https://crrev.com/2093bb8f1a80cdafbf24832d8662dbabaa92311a/printing/common/pdf_metafile_utils.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Feb 28 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/af231fc661b1a4eff56168c27cb10538014dac73

commit af231fc661b1a4eff56168c27cb10538014dac73
Author: Wei Li <weili@chromium.org>
Date: Wed Feb 28 16:26:42 2018

Revert "Revert "Print with dead subframes for pdf composition""

This reverts commit 2093bb8f1a80cdafbf24832d8662dbabaa92311a.

Reason for revert: The last flaky test shown in  crbug.com/817068  is before the fix https://chromium.googlesource.com/chromium/src.git/+/788c11139b869d1fc5b82cbbd238af2fd543535e. According to flaky dashboard, there is no test failure from these browser tests after the fix landed. So it should be good to stay.

Original change's description:
> Revert "Print with dead subframes for pdf composition"
> 
> This reverts commit d20c01231eb8acf9d38bc3aa37a108f2cee5f8d2.
> 
> 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: Colin Blundell <blundell@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#539818}

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

Change-Id: Iab653dd94f117956059bf950a4c54fd133270c07
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  814086 
Reviewed-on: https://chromium-review.googlesource.com/941501
Reviewed-by: Wei Li <weili@chromium.org>
Commit-Queue: Wei Li <weili@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539843}
[modify] https://crrev.com/af231fc661b1a4eff56168c27cb10538014dac73/chrome/browser/printing/print_browsertest.cc
[modify] https://crrev.com/af231fc661b1a4eff56168c27cb10538014dac73/components/printing/browser/print_composite_client.cc
[modify] https://crrev.com/af231fc661b1a4eff56168c27cb10538014dac73/components/printing/browser/print_composite_client.h
[modify] https://crrev.com/af231fc661b1a4eff56168c27cb10538014dac73/components/printing/service/pdf_compositor_impl.cc
[modify] https://crrev.com/af231fc661b1a4eff56168c27cb10538014dac73/components/printing/service/pdf_compositor_impl.h
[modify] https://crrev.com/af231fc661b1a4eff56168c27cb10538014dac73/components/printing/service/pdf_compositor_impl_unittest.cc
[modify] https://crrev.com/af231fc661b1a4eff56168c27cb10538014dac73/components/printing/service/public/interfaces/pdf_compositor.mojom
[modify] https://crrev.com/af231fc661b1a4eff56168c27cb10538014dac73/printing/common/pdf_metafile_utils.cc

Sign in to add a comment