139384 - Data URLs can take up to 10 minutes to Print (or more depending on the length)

Status: VERIFIED FIXED

(Core :: Printing: Output, defect)

Description

[Dan McGuirk]

The code in nsPageFrame::DrawHeaderFooter to truncate the header to the length
that will fit on the page is extremely slow if a very long string happens to be
passed in.  

For example, I have an application that embeds Gecko where I navigate to long
'data:' URLs.  If I try to print without removing the URL from the header first,
and the URL is, say, 30k long, its width is recomputed approximately 30k times,
removing one character at a time.  That hangs the machine for 5-10 minutes.  It
would be much smarter to add up the width one character at a time starting from
the beginning; such an algorithm would be efficient with strings of any length.

Comment 1

[rods (gone)]

Attachment: patch

The patch removes the n-order search for the length and reuses a funciton
"BinarySearchForPosition" code from the nsTextFrame for finding cursor
position. Instead of cursor position, it is just uses the available width.

This simplifies the code and makes it very fast.

Comment 2

[rods (gone)]

I test this with no title and a very long title and a short title. Works great!

Comment 3

[rods (gone)]

Since the diff is hard to decypher here is what the code looks like now:

nsAutoString str;
ProcessSpecialCodes(aStr, str);

PRInt32 indx;
PRInt32 textWidth = 0;
const PRUnichar* text = str.get();

PRInt32 len = (PRInt32)str.Length();
if (!len) {
  return; // bail is empty string
}
// find how much text fits, the "position" is the size of the avilable area
if (BinarySearchForPosition(&aRenderingContext, text, 0, 0, 0, len,
                            PRInt32(contentWidth), indx, textWidth)) {
  if (indx < len-1 && len > 3) {
    str.SetLength(indx-3);
    str.Append(NS_LITERAL_STRING("..."));
  }
} else { 
  return; // bail if couldn't find the correct length
}

Comment 4

[Kevin McCluskey (gone)]

nsbeta1+

Comment 5

[dcone (gone)]

Comment on attachment 80940 [details] [diff] [review]
patch

r = dcone

Comment 6

[Marc Attinasi]

Comment on attachment 80940 [details] [diff] [review]
patch

sr=attinasi (though I really think the function BinarySearchForPosition should
be a static method on nsTextFrame, or at least put into the header so you don't
have to redeclare it - #include is your friend)

Comment 7

[rods (gone)]

Attachment: patch with reviewer comments

Adding this patch with changes from reviewers comments we can eaily apply it to
the branch, I'll carry the r= and sr= forward

Comment 8

[rods (gone)]

fixed.

To verify print a page with no title, a very long title, a short title, and a
somewhat sizeable data url.

Comment 9

[rods (gone)]

really fixed

Comment 10

[sujay]

Dan this should be working for you now....please verify...let us know...
thanks.

Comment 11

[Jaime Rodriguez, Jr.]

Sujay - Can you pls verify this one on the trunk? Also, any data you could
provide on the perf win would be great, too. thanks!

Comment 12

[sujay]

verified in 5/2 trunk build.

looks fine to me...I copied and pasted a lot of text into the <title> of
a document and then printed...printed in a reasonable amount of time
like any other document.

Comment 13

[Jaime Rodriguez, Jr.]

Let's get this one in after RC2. adt1.0.0- [adt2 RTM]

Comment 14

[rods (gone)]

ADT: low risk fix. If anyone prints and sizable data URL Mozilla begins to look
like it is hung.

Comment 15

[Jaime Rodriguez, Jr.]

Risk assessment from rods email, "small risk, well tested, big pay off in
performance ... reused some existing code to calculate the length of the page
title in order to chop it off to to fit." This has been verified by QA to fix
the bug.

Impact on User - Without this fix some data urls take 10 minutes to print. 

Comment 16

[chris hofmann]

Comment on attachment 81132 [details] [diff] [review]
patch with reviewer comments

a=chofmann for the 1.0 branch.

Comment 17

[scottputterman]

adding adt1.0.0+.  Please get drivers approval again since this fix is more than
3 days old before checking this in to the 1.0 branch.

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Re-approval granted; please checkin ASAP

Comment 19

[rods (gone)]

fixed on branch

Comment 20

[sujay]

verified in 5/23 branch build.