1427553 - Print preview for composition window stuck on first run at 'Preparing...', works on second attempt

Status: RESOLVED FIXED

(Core :: Print Preview, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Thunderbird 59.0a1 20180101030201 on Windows 8.1, also 58.0b2

The print preview for the composition window gets stuck on first run at 'Preparing...', works on second attempt.

Steps to reproduce:
1. Start writing a new message (plain text or HTML, doesn't matter).
2. Use "File" > "Print preview".
Actual result:
Modal popup for preparing print preview gets stuck at "Preparing..."
Expected result:
Print preview shown.
3. Close print preview.
4. Use "File" > "Print preview".
Actual and expected result:
Print preview shown.

Comment 1

[Jorg K (CEST = GMT+2)]

Yes, I can confirm this, but we should report this to our M-C friends. The error console says:

TypeError: aStr is null[Learn More]  printPreviewProgress.js:21:1
	ellipseString chrome://global/content/printPreviewProgress.js:21:1
	onLoad chrome://global/content/printPreviewProgress.js:79:18
	onload chrome://global/content/printPreviewProgress.xul:1:1

And the line in question is:
function ellipseString(aStr, doFront) {
  if (aStr.length > 3 && (aStr.substr(0, 3) == "..." || aStr.substr(aStr.length - 4, 3) == "..."))

Could you try an M-C patch with 
if (aStr && aStr.length > 3 ... etc.

Comment 2

[Jorg K (CEST = GMT+2)]

Since this appears to be a regression, I hope Alice can help us find the culprit which usually gives an indication of the way forward.

Comment 3

[Alice0775 White]

Regression window:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=a400770aa52773348935dc6a638ae7caa64eb882&tochange=0b0cba8d70bd35997965afe4c02010c49e402980
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ce1a86d3b4db161c95d1147676bbed839d7a4732&tochange=d49501f258b105c5e2dcd0a59896ec1ceabf726b

Suspect: Bug 1410288

Comment 4

[Alice0775 White]

or Bug 1410850

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: 1427553-ellipseString-handle-null.patch

This fixes the issue. I guess before bug 1410288 an empty string was passed in here, now it's null and things fall apart :-(

Bob, can you please take the review, I can't r? you.

Comment 6

[Bob Owen (:bobowen)]

Comment on attachment 8939558 [details] [diff] [review]
1427553-ellipseString-handle-null.patch

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

I'm assuming we should add similar to toolkit/components/printing/content/printProgress.js version as well.

::: toolkit/components/printing/content/printPreviewProgress.js
@@ +18,5 @@
>  var progressParams = null;
>  
>  function ellipseString(aStr, doFront) {
> +  if (!aStr)
> +    return "";

nit: curly braces even for single line

Comment 7

[Jorg K (CEST = GMT+2)]

Thanks.

(In reply to Bob Owen (:bobowen) from comment #6)
> I'm assuming we should add similar to
> toolkit/components/printing/content/printProgress.js version as well.
OK, I'll do that.

> > +  if (!aStr)
> > +    return "";
> nit: curly braces even for single line
Really? If you insist, I'll add them. There is plenty of code without them.

Comment 8

[Bob Owen (:bobowen)]

(In reply to Jorg K (GMT+1) from comment #7)
...
> > nit: curly braces even for single line
> Really? If you insist, I'll add them. There is plenty of code without them.

That code doesn't match the coding style guide.
For purely aesthetic things we tend to say match the existing code if it differs, but this style of if can lead to bugs if people try to add a line and miss the fact that the braces aren't there.

(Personally I find it easier to read code when it always has braces anyway, but I'm sure some people would find otherwise.)

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 1427553-ellipseString-handle-null.patch (v2)

OK, I added the other hunk in printProgress.js with braces, but in printPreviewProgress.js I left it without the braces since there isn't a single brace in the entire function. Do you really want to mix styles there? Somehow you have the same code written in two different ways :-(

Comment 10

[Bob Owen (:bobowen)]

Comment on attachment 8940211 [details] [diff] [review]
1427553-ellipseString-handle-null.patch (v2)

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

(In reply to Jorg K (GMT+1) from comment #9)
...
> ... but in
> printPreviewProgress.js I left it without the braces since there isn't a
> single brace in the entire function. Do you really want to mix styles there?
> Somehow you have the same code written in two different ways :-(

As it isn't purely a style thing, I would prefer the braces, but I won't cry too much if you don't. :-)
Unfortunately there is a lot of duplication like this in the printing code, but most of it is pretty old.

Comment 11

[Jorg K (CEST = GMT+2)]

Thanks again.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3cd9270f7d9
Handle mull gracefully in ellipseString(). r=bobowen

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f3cd9270f7d9

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 15

[Jorg K (CEST = GMT+2)]

As per conversation with Ryan on IRC: Print preview works fine in FF 58, TB won't ship another beta, so no uplift required. Thanks for the consideration :-)