909078 - Browser hangs attempting to print preview on certain page

Status: RESOLVED FIXED

(Core :: Print Preview, defect)

Description

[Alice0775 White]

Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/17143a9a0d83
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130824030203

Steps To Reproduce:
1. Open http://en.wikipedia.org/wiki/H-II_Transfer_Vehicle
2. Print Preview (Alt > File > Print Preview)

Actual Results:
Browser hang up


Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/e0d7b394462b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121108022104
Bad:
http://hg.mozilla.org/mozilla-central/rev/27ae6c86237f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121108165332
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e0d7b394462b&tochange=27ae6c86237f

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/fdcb6474f4b4
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121108080032
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/ac1fdaa261f2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121108081030
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=fdcb6474f4b4&tochange=ac1fdaa261f2

Suspected :Bug 685012

Comment 2

[Alice0775 White]

Attachment: testcase html

Comment 3

[Alice0775 White]

STR
1. Open attachment 795140 [details]
2. Print Preview (Alt > File > Print Preview)
3  Set scale 200% if necessary
   
In local build
Last Good: dbf63c3f34e9 
First Bad: ea3dd343ee5e

Triggered by: ea3dd343ee5e	Mats Palmgren — Bug 685012 - Implement page-break-inside:avoid in layout. r=fantasai,sr=roc

Comment 4

[Mats Palmgren (inactive)]

Attachment: fix+tests

What happens is that the inner table doesn't fit, and since the <caption>
caused the top-of-page flag for the inner table reflow state to be false,
it thinks it's OK to request a forced page break before it even though
the outer table frame is top-of-page.

The fix is to check the outer table frame's reflow state for top-of-page.
I'm doing that rather than lying and making the inner top-of-page flag
be a copy of the outer to avoid affecting other stuff -- it's just
break-before that is special, since it really is for the whole <table>
including the <caption>.

https://tbpl.mozilla.org/?tree=Try&rev=84b655588269

Comment 5

[Mats Palmgren (inactive)]

Comment on attachment 795163 [details] [diff] [review]
fix+tests

Hmm, now that I think about it, the exact same thing probably happens
when the break-before comes from descendants as well...

Comment 6

[Mats Palmgren (inactive)]

Attachment: fix+tests, v2

Actually, I changed my mind about pretending the inner table frame is
top-of-page.  After investigating how we use the mIsTopOfPage flag it's
clear that is already used as a "logical" flag that really should be used
only for deciding on page breaks.  I was initially worried it would affect
applying top margins, but we use a separate IsAdjacentWithTop() method for
that which is "physical" (based on the distance from the edge).
http://hg.mozilla.org/mozilla-central/annotate/60c382ba1773/layout/generic/nsBlockFrame.cpp#l2753

So, let's make the inner table frame be top-of-page when the outer table
frame is (by removing the the code clears the flag when there's a top-side
caption -- the value is already a copy of the outer state at this point).

https://tbpl.mozilla.org/?tree=Try&rev=d5c161812642

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 795235 [details] [diff] [review]
fix+tests, v2

Review of attachment 795235 [details] [diff] [review]:
-----------------------------------------------------------------

avoid-8-ref.html is missing.

Comment 8

[Mats Palmgren (inactive)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> avoid-8-ref.html is missing.

I'm intentionally re-using the avoid-6-ref.html since the result is the same.

Comment 9

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/05670d699bdc

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/05670d699bdc

Comment 11

[Alice0775 White]

Is this possible to uplift the patch to fix in Firefox24 and 24esr?

Comment 12

[Mats Palmgren (inactive)]

I think the patch would apply cleanly, but I don't think it meets the
branch criteria for 24.  Aurora perhaps...

Comment 13

[Mats Palmgren (inactive)]

Comment on attachment 795235 [details] [diff] [review]
fix+tests, v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 685012
User impact if declined: hang in Print Preview on pages that use page-break-inside:avoid under rare conditions
Testing completed (on m-c, etc.): on m-c since 2013-08-26
Risk to taking this patch (and alternatives if risky): low, but the change affects all paginated layout, not just page-break-inside:avoid (which is rare) so the risk/reward doesn't seem very attractive to me.  Should be good for Aurora though, to get wider testing.
String or IDL/UUID changes made by this patch: none

Comment 14

[Alex Keybl [:akeybl]]

Too late to take for FF24, but we can take in our first ESR update (24.1). I'm setting tracking-firefox24 to ? until we have a tracking-firefox-esr24 flag.

Comment 15

[Mats Palmgren (inactive)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/889ee00342d6

Comment 16

[Mats Palmgren (inactive)]

Comment on attachment 795235 [details] [diff] [review]
fix+tests, v2

[Approval Request Comment]
see comment 13

Comment 17

[Mats Palmgren (inactive)]

https://hg.mozilla.org/releases/mozilla-esr24/rev/20c584f64fec

Comment 18

[Manuela Muntean [Away]]

Verified as fixed with latest ESR 24 nightly build from 2013-12-04, on: Ubuntu 13.10 32-bit, Win 7 64-bit, and Mac OS X 10.9, using both the STR from comment 0 and comment 3.

Note: on Mac, I've used the Open PDF in Preview option to test this.