New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 883963 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: Dec 27
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Does not print correctly, missing parts of page.

Reported by thoux2...@gmail.com, Sep 13

Issue description

UserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/69.0.3497.92 Safari/537.36

Steps to reproduce the problem:
1. Log into identifix.com and click on Free Test Drive
2. click on View traditional Maintenance Schedules
3. click print

What is the expected behavior?
Print the document 

What went wrong?
Check marks within the document do not appear in print

Did this work before? Yes 68

Chrome version: 69.0.3497.92  Channel: stable
OS Version: 10.0
Flash Version:
 
PrintingIssueWith Chrome69.jpg
209 KB View Download
Labels: Needs-Bisect Needs-Triage-M69
Components: -UI Internals>Printing
Labels: Needs-Feedback
Do you have a set of test credentials for this page, or a page that does not require a login that has the same problem, so that we can try to reproduce the issue?
I've attached a video to reproduce the issue without a log on.

https://www.identifix.com/
PrintissueWIthChrome.mp4
1.6 MB View Download
Project Member

Comment 5 by sheriffbot@chromium.org, Sep 14

Cc: rbpotter@chromium.org
Labels: -Needs-Feedback
Thank you for providing more feedback. Adding the requester to the cc list.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: kojii@chromium.org
Components: Blink
This bisects to https://chromium.googlesource.com/chromium/src/+log/cef2ea9d623b14a4d7e270e76419c222e96478b3..66da0a7ac0283b79781efed0f59cd9305a939230

Two of those changes are Mac and iOS only, so it has to be one of the two blink changes. Guessing https://chromium-review.googlesource.com/c/chromium/src/+/1095894? cc-ing kojii@.
Labels: -Needs-Bisect OS-Linux
Cc: swarnasree.mukkala@chromium.org
Labels: Triaged-ET Needs-Feedback
Unable to reproduce the issue on reported chrome version #69.0.3497.92 and latest canary 71.0.3552.2 using windows 10 by following below steps.

Steps:
=====
1.Launched chrome.
2.Navigated to "identifix.com".
3.Clicked on "A free test drive".
4.When a page opened clicked on "Test Drive".
5.After it navigates to a page under maintenance tab clicked on "maintenance schedule".
6.Opened the print preview of the page.
7.Under more settings options, checked "background graphics".
8.Observed check marks within the document appear in print preview.

Attached screencast for reference.
@reporter: Could you please review the attached screencast and let us know if anything is being missed her and requesting you to confirm whether the issue is seen on printed page or print preview.

Thanks.!
883963.mp4
4.6 MB View Download
Comment 8: You need to click the print button in the webpage (far right side), rather than directly opening print preview.
Clicking Background Graphics does not change the check marks being displayed.
PrintChromeIssue.mp4
1.1 MB View Download
Project Member

Comment 11 by sheriffbot@chromium.org, Sep 26

Labels: -Needs-Feedback
Thank you for providing more feedback. Adding the requester to the cc list.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: -kojii@chromium.org
Components: -Internals>Printing -Blink Blink>Layout
Labels: -Pri-2 Target-70 RegressedIn-69 FoundIn-71 FoundIn-70 FoundIn-69 Pri-1
Owner: kojii@chromium.org
Status: Assigned (was: Unconfirmed)
Confirmed the bisect in comment #6. Assigning.
I can't imagine how the CL in #6 affects print preview, but I also don't know how print preview kicks layout. I'll try to revert it and see if it fixes, I hope it's not too hard.
Project Member

Comment 14 by bugdroid1@chromium.org, Oct 9

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

commit 209909630cf94d4568bc62157053bae5791908eb
Author: Koji Ishii <kojii@chromium.org>
Date: Tue Oct 09 11:01:14 2018

Revert "Improve performance of PreviousBreakOpportunity"

This reverts commit 99d504fd35a7e07c4cf16bbbe1464044e2195b4e.

Reason for revert:

 crbug.com/883963  bisected to this CL. I'm not sure how this
affects print preview, reverting to see the effect.

This revert may affect LayoutNG performance, but should not
affect the currently shipping line breaker.

Original change's description:
> Improve performance of PreviousBreakOpportunity
>
> In the current layout engine, PreviousBreakOpportunity is used
> only when mid-word break (break-all or break-word), but it is
> much more heavily used in LayoutNG.
>
> LazyLineBreakIterator is designed for forward only.
> PreviousBreakOpportunity is implemented by repeatedly calling
> NextBreakablePosition, but since NextBreakablePosition look
> for the next break opportunity until the end of the string,
> when a very long word without break opportunity is given,
> PreviousBreakOpportunity is O(n!).
>
> This patch changes it to O(n) by limiting the end position
> NextBreakablePosition can look for.
>
> blink/perf_tests/layout/word-break-break-word.html consumes
> 78% of the total time in LayoutNG. The average run is:
>   Current engine:    469ms
>   LayoutNG:       26,644ms
>   This patch:      2,250ms
>
> It's still 4-5 times slower, more improvements will be in
> following patches.
>
> Bug: 636993
> Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_layout_ng
> Change-Id: I814e2c45c8030aa682c7f5e3a3b785b3c0733c84
> Reviewed-on: https://chromium-review.googlesource.com/1095894
> Commit-Queue: Koji Ishii <kojii@chromium.org>
> Reviewed-by: Emil A Eklund <eae@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#567133}

TBR=eae@chromium.org,kojii@chromium.org

Bug: 636993,  883963 
Change-Id: I2c453a011208b014e342d7b44646eb90b5b7bbd2
Reviewed-on: https://chromium-review.googlesource.com/c/1270396
Commit-Queue: Koji Ishii <kojii@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597876}
[modify] https://crrev.com/209909630cf94d4568bc62157053bae5791908eb/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/fast/text/midword-break-before-surrogate-pair-expected.png
[modify] https://crrev.com/209909630cf94d4568bc62157053bae5791908eb/third_party/blink/renderer/platform/text/text_break_iterator.cc
[modify] https://crrev.com/209909630cf94d4568bc62157053bae5791908eb/third_party/blink/renderer/platform/text/text_break_iterator.h

Cc: kojii@chromium.org
Labels: Needs-Bisect
Owner: ----
The revert in #14 landed in according to: 71.0.3575.0
https://storage.googleapis.com/chromium-find-releases-static/index.html

Now as I test 71.0.3576.0, this still reproduces.

Can you please run bisect again?
Project Member

Comment 16 by bugdroid1@chromium.org, Oct 11

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

commit 786b1994bf6f43f2ff038894a1891435f2de22b9
Author: Koji Ishii <kojii@chromium.org>
Date: Thu Oct 11 14:22:14 2018

Reland "Improve performance of PreviousBreakOpportunity"

This reverts commit 209909630cf94d4568bc62157053bae5791908eb.

Reason for revert: The revert did not fix  crbug.com/883963 .
Requested to re-run bisect.

Original change's description:
> Revert "Improve performance of PreviousBreakOpportunity"
> 
> This reverts commit 99d504fd35a7e07c4cf16bbbe1464044e2195b4e.
> 
> Reason for revert:
> 
>  crbug.com/883963  bisected to this CL. I'm not sure how this
> affects print preview, reverting to see the effect.
> 
> This revert may affect LayoutNG performance, but should not
> affect the currently shipping line breaker.
> 
> Original change's description:
> > Improve performance of PreviousBreakOpportunity
> >
> > In the current layout engine, PreviousBreakOpportunity is used
> > only when mid-word break (break-all or break-word), but it is
> > much more heavily used in LayoutNG.
> >
> > LazyLineBreakIterator is designed for forward only.
> > PreviousBreakOpportunity is implemented by repeatedly calling
> > NextBreakablePosition, but since NextBreakablePosition look
> > for the next break opportunity until the end of the string,
> > when a very long word without break opportunity is given,
> > PreviousBreakOpportunity is O(n!).
> >
> > This patch changes it to O(n) by limiting the end position
> > NextBreakablePosition can look for.
> >
> > blink/perf_tests/layout/word-break-break-word.html consumes
> > 78% of the total time in LayoutNG. The average run is:
> >   Current engine:    469ms
> >   LayoutNG:       26,644ms
> >   This patch:      2,250ms
> >
> > It's still 4-5 times slower, more improvements will be in
> > following patches.
> >
> > Bug: 636993
> > Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_layout_ng
> > Change-Id: I814e2c45c8030aa682c7f5e3a3b785b3c0733c84
> > Reviewed-on: https://chromium-review.googlesource.com/1095894
> > Commit-Queue: Koji Ishii <kojii@chromium.org>
> > Reviewed-by: Emil A Eklund <eae@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#567133}
> 
> TBR=eae@chromium.org,kojii@chromium.org
> 
> Bug: 636993,  883963 
> Change-Id: I2c453a011208b014e342d7b44646eb90b5b7bbd2
> Reviewed-on: https://chromium-review.googlesource.com/c/1270396
> Commit-Queue: Koji Ishii <kojii@chromium.org>
> Reviewed-by: Koji Ishii <kojii@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#597876}

TBR=eae@chromium.org,kojii@chromium.org

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

Bug: 636993,  883963 
Change-Id: I8e9881deca736c5b574dc287d04f9c2fffe76680
Reviewed-on: https://chromium-review.googlesource.com/c/1275985
Reviewed-by: Koji Ishii <kojii@chromium.org>
Commit-Queue: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598754}
[modify] https://crrev.com/786b1994bf6f43f2ff038894a1891435f2de22b9/third_party/WebKit/LayoutTests/flag-specific/enable-blink-features=LayoutNG/fast/text/midword-break-before-surrogate-pair-expected.png
[modify] https://crrev.com/786b1994bf6f43f2ff038894a1891435f2de22b9/third_party/blink/renderer/platform/text/text_break_iterator.cc
[modify] https://crrev.com/786b1994bf6f43f2ff038894a1891435f2de22b9/third_party/blink/renderer/platform/text/text_break_iterator.h

Components: -Blink>Layout Internals>Printing
Owner: schenney@chromium.org
Revering the change didn't help, would you mind re-running the bisect schenney or are we better of having the printing team triage this?
I'll look but probably not until late Monday or Tuesday.
Labels: M-72
Able to reproduce the issue on chrome version #71.0.3576.0 as per comment #15 on Windows 10 and Ubuntu 17.10 by following steps as per comment #9.The following is the bisect information.
Note: Could not check the issue on Mac Os as the canary in Mac is still in M-71.

Bisect information:
==============
Good build - 71.0.3578.0
Bad Build - 72.0.3579.0

Note: Unable to provide Change Log from per-revision-bisect and chromium bisect as it was giving an error as "RuntimeError: We don't have enough builds to bisect."
Thanks.!
Can you please bisect between 68 and 69.0.3497.92, as in #0?

Also, this seems sometimes flaky. For one I noticed, it does not reproduce once I open Inspector, but there maybe more.
Labels: -Needs-Bisect
@kojii: As per comment#20 tried bisecting the issue but found inconsistent behavior, hence unable to provide bisect information from our end. Requesting schenney@ to look into the issue.

Thanks.!
Cc: pmeenan@chromium.org
I bisected to r562150. Does that sound a more plausible culprit?
I get the same bisect range as comment #6, which again suggests kojii@'s patch.

I'll try to bisect around the revert to see if that did or did not fix it.
The revert apparently did not fix it, so maybe we're blaming the wrong thing.

I redid the original bisect and get the same range. Now to try to verify on a ToT build and locally revert the other patches in the revision range.
As commented in #20, this sometimes does not repro, like after I opened Inspector. If r562150 is possible, maybe CSSInBodyDoesNotBlockPaint causes paint before we evaluate CSS?
I also bisected to r567133. I between r562150 and r567133, the bug is flaky, so sometimes I cannot repro and that makes reverse bisecting difficult.
It could be the flakiness is a race condition introduced by r562150 and then triggered by r567133. This may also explain why the bug does not repro when you use the browser print instead of the JS print.

Does anyone happen to know if we force different lifecycle behavior for the two print modes?
pmeenan@ might know better, but from the name "CSSInBodyDoesNotBlockPaint", I'm guessing we might be taking the PDF before we evaluate CSS in body because paint occurred? That could explain the flakiness if so.
Any status update on this bug? 
Owner: pmeenan@chromium.org
I shouldn't own this. Over to pmeenan@ to see if disabling r562150 solves the problem.
Owner: chrishtr@chromium.org
Status: WontFix (was: Assigned)
This issue was indeed caused by the CSSInBodyDoesNotBlockPaint feature.

What happens is that when printing, the app dynamically creates an iframe
with the printed contents, presumably in response to the beforeprint event
or when clicking the print button. This iframe has multiple external
stylesheets, but the one that contains the checkbox image CSS is the same
stylesheet as is present in the main frame.

Before CSSInBodyDoesNotBlockPaint was turned on, the parser would not stop
parsing while waiting for that style sheet. By the end of parsing, the style sheet
had loaded because it was already in the memory cache (due to it being in the
style sheet list for the main frame).

Now it does, which appears to cause a brief yield of the document parser, and then a render, without the style sheet.

The site should not be relying on this behavior, and instead set up the iframe in advance
and use media queries to change style.

Sign in to add a comment