1074743 - Background thumbnailer captures pictures with black right and bottom borders

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Screenshot

Not exactly sure when this started but pages solely captured by the background thumbnailer have a quite large right and bottom left border.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Last good nightly: 2014-07-15
First bad nightly: 2014-07-16

Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=095d2a9c2be5&tochange=869971ad9fd6

Comment 2

[:Gijs (out for now; he/him)]

My money is on bug 1022612, but then again, that's a cheap shot considering the number of patches...

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Yeah, had the same suspicion. Bisecting currently, almost done :)

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

The first bad revision is:
changeset:   193934:692a0f99d09d
user:        Mike Conley <[e-mail]>
date:        Mon Jul 14 13:22:26 2014 -0400
summary:     Bug 1002354 - Proxy nsIScreenManager and nsIScreen's from the child process to the parent process, with caching. r=roc,jimm,smichaud,snorp.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Hrm. Strange. I don't know much about how we thumbnail... do we do it out of process? Would the thumbnailing system somehow use the nsScreenManagerProxy I introduced in bug 1002354?

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Yes, we indeed use a remote browser. I will investigate later when I find time what's going wrong here.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

I'm also seeing this bug, though only on certain thumbnails. I see it on the Zimbra sign-in page thumbnail, for example. But not for reddit.com.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

You'll only see it for pages captured with the background service. The bg service kicks in when we don't have a thumbnail captured while browsing the site, that could be because it's simply missing or because we didn't take one for privacy reasons.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Looks like nsIScreenManager.primaryScreen.GetRectDisplayPix() reports device pixels, not display pixels. Likely a retina-only bug where it reports 2x the size compared to before bug 1002354 landed.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1074743-Forward-display-pixel-values-to-screen-p.patch

Works fine for me now and returns display pixels when requested.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

[Tracking Requested - why for this release]:

I think that the fix here is quite simple and the issue is very visual. For mostly OS X users - but actually all users with HiDPI devices - the background thumbnailer will capture images with large black borders. The issue has been introduced with Firefox 33.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

The build of beta 9 starts tomorrow. This will have to land soon!

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Yeah, I know :/ Let's see.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Thanks!

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

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8497900 [details] [diff] [review]
0001-Bug-1074743-Forward-display-pixel-values-to-screen-p.patch

Sorry, Mike. I'm sure you would have liked it! :)

Comment 16

[Mike Conley (:mconley) (:⚙️)]

I do like it! :)

I guess we just need to make sure to request uplift approval if we want this stuff. And, uh, "my bad" on failing to provide those numbers the first time.

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8497900 [details] [diff] [review]
0001-Bug-1074743-Forward-display-pixel-values-to-screen-p.patch

Approval Request Comment
[Feature/regressing bug #]: bug 1002354
[User impact if declined]: large black right and bottom borders on some tiles of the newtab page
[Describe test coverage new/current, TBPL]: no test coverage for the thumbnails itself, Nightly is looking great though
[Risks and why]: Low-risk change
[String/UUID change made/needed]: None.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Tim, If regressions are caused by the bug, what could they be? Thanks

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #19)
> Tim, If regressions are caused by the bug, what could they be? Thanks

I think... regressions introduced by that bug would affect the nsIScreenManager API but only if it is used from a remote process. The only things I know that use it in Firefox from a remote process would probably be the background thumbnailer and e10s. The impact of any regression should thus be fairly small.

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e6104aae6991
https://hg.mozilla.org/releases/mozilla-beta/rev/fa58aaa6863e

Comment 22

[Camelia Badau [:cbadau], Desktop Test Engineering]

I was unable to reproduce the initial issue on Nightly 2014-07-16 and Nightly 2014-09-29. 

Tim, cam you please confirm the fix for this bug?

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

Verified manually as fixed for Nightly and Aurora. I assume that we don't distribute the Beta build yet? Is it built yet? Did this make b9?

Comment 24

[Sylvestre Ledru [:Sylvestre]]

(In reply to Tim Taubert [:ttaubert] (limited availability, work week) from comment #23)
> Verified manually as fixed for Nightly and Aurora. I assume that we don't
> distribute the Beta build yet?
Yes: https://ftp.mozilla.org/pub/mozilla.org/firefox/releases/33.0b9/

> Is it built yet? 
yes

> Did this make b9?
yes

hope this helps!

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

Got a new version now. Fixed on beta as well!

Comment 26

[Camelia Badau [:cbadau], Desktop Test Engineering]

Thank you, Tim!