744610 - [Page Thumbnails] WARNING: Surface width or height < 0!: file gfx/thebes/gfxASurface.cpp, line 359

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Mats Palmgren (inactive)]

Attachment: stack

STEPS TO REPRODUCE
1. reload a Bugzilla page in a window with small height (100px or so)
(I had a few other tabs as well, not sure what was in them)

ACTUAL RESULT
WARNING: Surface width or height < 0!: gfx/thebes/gfxASurface.cpp, line 359
from JS: PageThumbs_capture() ["resource:///modules/PageThumbs.jsm":94]
(see attached stack)

PLATFORMS AND BUILDS TESTED
Bug occurs in a trunk debug build on Linux x86-64

Comment 1

[Daniel Holbert [:dholbert]]

I noticed this in my debug build today.

This was in a call to:
  nsCanvasRenderingContext2D::DrawWindow() with aX=0, aY=0, aW=-276, aH=154
(notice the negative aW == width value there)

My JS stack (from call DumpJSStack()) was:

0 <TOP LEVEL> ["<unknown>":0]
    <failed to get 'this' value>
1 PageThumbs_capture() ["resource:///modules/PageThumbs.jsm":131]
    this = [object Object]
2 PageThumbs_captureAndStore() ["resource:///modules/PageThumbs.jsm":157]
    this = [object Object]
3 <TOP LEVEL> ["<unknown>":0]
    <failed to get 'this' value>
4 Thumbnails_capture() ["chrome://browser/content/browser.js":5145]
    this = [object Object]
5 anonymous() ["chrome://browser/content/browser.js":5156]
    this = [object Object]
6 <TOP LEVEL> ["<unknown>":0]
    <failed to get 'this' value>

It looks like the width & height come from "_determineCropSize".  Perhaps that functions should get some code to either check or enforce that the values they're returning are nonnegative?

Comment 2

[Bellindira Castillo [:bellindira]]

Attachment: Patch: Fixed crop size and scale

It was enforced the minimum inner height to 1 because when its value was 0, the scale value got undefined (thumbnailWidth / 0 => infinity). Minimum inner width was not enforced because it is always higher than 0.

Also, it was changed the scale calculation. Basically, it was split into two separate scales: one for width and the other for height. The main reason of this change was because it causes negative values when having only one scale for both values (small height and big width or big height and small width).

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 652329 [details] [diff] [review]
Patch: Fixed crop size and scale

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

Thank you Bellindira! I think your solution is a little too complicated for the little fix we'd need here. I explained my thoughts below:

::: browser/components/thumbnails/PageThumbs.jsm
@@ -199,5 @@
>      let sw = aWindow.innerWidth;
>      let sh = aWindow.innerHeight;
>  
>      let [thumbnailWidth, thumbnailHeight] = this._getThumbnailSize();
> -    let scale = Math.max(thumbnailWidth / sw, thumbnailHeight / sh);

All we actually need to do is ensure that $scale doesn't get bigger than 1. We never want to upscale thumbnails - they should always be smaller or equal to $innerWidthx$innerHeight.

That's a situation the algorithm doesn't expect and clearly shouldn't need to handle.

Comment 4

[Bellindira Castillo [:bellindira]]

Attachment: Fixed error by ensuring that scale doesn't get bigger than 1

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 661063 [details] [diff] [review]
Fixed error by ensuring that scale doesn't get bigger than 1

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

Thank you!

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/98843500771b

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/98843500771b