344883 - print previewing url freezes bon echo (and minefield if you zoom too)

Status: RESOLVED FIXED

(Core :: Print Preview, defect)

Description

[John Mellor (Jomel)]

Print previewing the url above consistently freezes Bon Echo, at the "Preparing" stage.

Note that the url above is actually an article on how to embed images at higher resolution than normal so they print at high quality. Clearly this less usual technique might be something to do with the problem; but if so it's embarassing to crash as a result of a technique recommended on a high-profile web-design site.

I can also consistently freeze Minefield, though that takes more steps and could actually be a different problem:
  1. Visit above url
  2. Enter print preview (so far so good)
  3. Change scale factor to 800% (you have to use custom, and yes I realise this is a bit of an edge case, but it works fine for 400% say)
  *  Minefield freezes (again at the "Preparing" stage)

The two UAs are 
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060714 BonEcho/2.0b1
and
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060714 Minefield/3.0a1
respectively.

Comment 1

[Mats Palmgren (inactive)]

I can reproduce the hang in a 1.8 branch debug build on Linux.
I see this assertion before the hang:
###!!! ASSERTION: running past end: 'mCurrent != mListLink', file nsLineBox.h, line 591

Comment 2

[Leon Sha]

Attachment: stack of this bug

http://lxr.mozilla.org/seamonkey/source/layout/generic/nsSplittableFrame.cpp#187
187   while (nsIFrame *prev = firstInFlow->GetPrevInFlow())  {
188     firstInFlow = NS_STATIC_CAST(nsSplittableFrame*, prev);
189   }
This while never end.

Comment 3

[Leon Sha]

Is this a duplicate of bug 255982?

Comment 4

[Leon Sha]

Attachment: Patch

nsTableFrame::OrderRowGroups will almost always add the head to the rowgroup.
So kidframe will always consider it not at the top of the page.
It will try to put this frame to the next page.
That cause the infinite loop.

Comment 5

[Bernd]

is bug 318138 a dupe of this?

Comment 6

[Leon Sha]

(In reply to comment #5)
> is bug 318138 a dupe of this?
> 

It's possible. After apply my patch that bug disappears either.

Comment 7

[Bernd]

Please add a comment into the code what this is doing, as it looks pretty much like a hack that needs to be explained.

Comment 8

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

Comment on attachment 230872 [details] [diff] [review]
Patch

I'd like to see the new patch with the comment.

Comment 9

[Leon Sha]

Attachment: Patch with comments.

Comment 10

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

Comment on attachment 232490 [details] [diff] [review]
Patch with comments.

// When a new page start,

"starts"

Thanks!!

Comment 11

[Leon Sha]

Commited with roc's comments.
Thanks bernd_mozilla and roc.

cvs commit: Examining .
Checking in nsTableFrame.cpp;
/cvsroot/mozilla/layout/tables/nsTableFrame.cpp,v  <--  nsTableFrame.cpp
new revision: 3.652; previous revision: 3.651
done

Comment 12

[Bernd]

Comment on attachment 232490 [details] [diff] [review]
Patch with comments.

I guess it wouldn't be bad to have this in 1.8.1

Comment 13

[Mike Schroepfer]

Roc/Bernd can you comment on risk for this patch on the branch?

Comment 14

[Bernd]

From my point of view the code change is low risk. However the regression spotting from printing/print preview is very slow. So I guess one can not rely on the "did not cause regressions" criteria as we would simply not hear about them in time.

Comment 15

[Bernd]

*** Bug 318138 has been marked as a duplicate of this bug. ***

Comment 16

[Mats Palmgren (inactive)]

Comment on attachment 232490 [details] [diff] [review]
Patch with comments.

+ // When a new page start, a head row group may be added automatically.
+ // We also consider the row groups just after the head as the top of the page.
+ // That is to prevent the infinite loop in some circumstance. See bug 344883.
+ if (childX > (thead ? 1 : 0)) {

If you really meant to only test for repeated THEADs you should test
this using |IsRepeatedFrame(thead)|. On the other hand, in paginated
mode we know that splitting the table even after the first "real"
THEAD will cause the same thing to happen on the second page so
I guess the test should look like this:

+ if (childX > (thead && (presContext->IsPaginated() ||
                          IsRepeatedFrame(thead)) ? 1 : 0)) {

And since we know we won't have repeated frames in the
non-paginated case, we can shorten this to:

+ if (childX > (thead && presContext->IsPaginated() ? 1 : 0)) {

As far as I know we use |mIsTopOfPage| in non-paginated mode as well
and I'm a bit worried that this change affects those parts.
Limiting the change to paginated mode only seems less risky.

Also, it might be worth testing if this patch changes the behaviour
of page-break-before/after at all.

Comment 17

[Mike Schroepfer]

Comment on attachment 232490 [details] [diff] [review]
Patch with comments.

until the issues covered in comment 16 get worked out we cannot take this patch for 1.8.1 branch.  Since this is an edge case the fix would have to be *super* safe for us to take a patch this close to RC1.

Comment 18

[Darryl Trimming]

Although this bug has been marked as RESOLVED, as well as 318138, the problem persists even in FF 2.0.0.6.

To duplicate results:

Visit Guenter's "test case" URL (see attachments in 318138). You may need to toggle between landscape and portrait in Print Preview mode.

-- OR -- 

Try to do a Print Preview while browsing at http://md5deep.sourceforge.net/