466258 - [Windows] Painting regression, on 2008-11-04: related to 'border: solid' on a 'table'

Status: VERIFIED FIXED

(Core :: Graphics, defect, P2)

Description

[Serge Gautherie (:sgautherie)]

Work fine:
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081104 SeaMonkey/2.0a2pre] (nightly) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/f923f7a759e2) "21292"
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081104 Minefield/3.1b2pre] (nightly) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/fbae114d6133) "21305"

Regressed:
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081105 SeaMonkey/2.0a2pre] (nightly) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/bf2f8ad8f1ce) "21366"
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081105 Minefield/3.1b2pre] (nightly) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/dcec193ba5d7) "21367"

Regression timeframe:
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-11-04+01%3A31%3A47&enddate=2008-11-04+23%3A38%3A24

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: Saved page, as testcase

Site access needs a free account ... but the affected feature/page needs a level 10 character, which takes days to get :-/

This is an unreduced testcase:
the white mountains are meant to be at the upper right corner...

Comment 2

[Serge Gautherie (:sgautherie)]

Ah ... This bug should have the "security" flag set,
as the "picture" I attached is supposed to be undisclosed in the Shinobi game :-<
(Consider it as private data.)

If someone can set the flag... Thanks.

Comment 3

[cmtalbert]

Flag set, not sure if that's a proper use for it or not, but it's easy to unset if I'm wrong.

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to comment #0)
> [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081104
> Minefield/3.1b2pre] (nightly) (W2Ksp4)
> (http://hg.mozilla.org/mozilla-central/rev/fbae114d6133) "21305"

Note to self: "21302" !

Comment 5

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081122 Minefield/3.1b2pre] (home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/755f505ee163) "21346"

No bug.

http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-11-04+11%3A58%3A50&enddate=2008-11-04+23%3A38%3A24

Comment 6

[Bob Clary [:bc] (inactive)]

I marked the attachment as private. Is that sufficient to open the rest of the bug?

Comment 7

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081122 Minefield/3.1b2pre] (home, optim default) (W2Ksp4)
Bad:
(http://hg.mozilla.org/mozilla-central/rev/e16da35c4223) "21358"
(http://hg.mozilla.org/mozilla-central/rev/49dd935efff3) "21351"
Good:
(http://hg.mozilla.org/mozilla-central/rev/2d35868314cf) "21350"
(http://hg.mozilla.org/mozilla-central/rev/755f505ee163) "21346"

This is bug 458487 !

I suggest this should block 1.9.1b2.

Comment 8

[Daniel Veditz [:dveditz]]

(In reply to comment #6)
> I marked the attachment as private. Is that sufficient to open the rest of the
> bug?

Maybe, if the people needing to fix the bug have security-group access. Otherwise you should un-hide the attachment and leave the bug private until it's patched, and then re-hide the attachment before opening the bug after the fix. Or get an admin to delete the attachment. Either use is an abuse of the security-group flags and the site's privacy policy saying everything posted is public, a better approach would be to put the image on a private server (where it can easily be deleted later) or offer to email it as needed.

Since everyone involved with bug 458487 can see private attachments (I think) we can just leave it this way for now.

Note that all the images and css used by the attachment are loaded remotely from shinobi.fr so there's no guarantee it will keep working. For long-term regression tests we can build into the tree we should come up with a reduced self-contained file that demonstrates the problem.

Comment 9

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

What is the actual bug here? Can you attach screenshots showing what it's supposed to look like? Better still reduce the testcase...

Comment 10

[Serge Gautherie (:sgautherie)]

Attachment: Image of expected layout

Comment 11

[Serge Gautherie (:sgautherie)]

Attachment: Image of regressed layout

Comment 12

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

(In reply to comment #10)
> Created an attachment (id=349666) [details]
> Image of expected layout

This is what I see on Mac trunk.

Comment 13

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081123 Minefield/3.1b2pre] (nightly) (W2Ksp4)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081123 SeaMonkey/2.0a2pre] (nightly) (W2Ksp4)

Windows only bug ?

Comment 14

[Martijn Wargers (dead)]

I can see the bug with today's trunk build. It looks like a painting bug, when overlaying the image with the context menu, it disappears.
Perhaps related to bug 465799?

Comment 15

[Serge Gautherie (:sgautherie)]

(In reply to comment #14)

Why did you remove bug 458487 dependency ?
It's what caused/triggered this !

> I can see the bug with today's trunk build. It looks like a painting bug, when

On which platform ?

> overlaying the image with the context menu, it disappears.

I can confirm this.
Also, on the website, there is a dynamic tooltip, which does repaint under itself too.

> Perhaps related to bug 465799?

Maybe, though it would be more related to 'border' than to 'background-position'.

Comment 16

[Martijn Wargers (dead)]

(In reply to comment #15)
> Why did you remove bug 458487 dependency ?
> It's what caused/triggered this !

Sorry, that didn't happen on purpose.

Comment 17

[Serge Gautherie (:sgautherie)]

Attachment: Saved page (99% reduced), as testcase

|border: solid;| comes from the linked css of the initial page;
it's what triggers the wrong painting.

I left 1 (much bigger) cell in the table, so we can still see something :->

Comment 18

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

What's the expected rendering of that testcase? Is this Windows only perhaps?

Comment 19

[Serge Gautherie (:sgautherie)]

(In reply to comment #18)
> What's the expected rendering of that testcase?

The expected rendering is the upper left of the expected image.
Basically, the testcase should render the "same" (image) with/without the border style !

> Is this Windows only perhaps?

That's what I asked in comment 13...
Martijn didn't answer on which platform he reproduced the bug.

Comment 20

[Martijn Wargers (dead)]

I'm on Windows, yes.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I see this on Windows but not on Linux.

Comment 22

[Serge Gautherie (:sgautherie)]

Not Linux, not MacOSX: Windows only, then.

Comment 23

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

Attachment: image for testcase

Comment 24

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

Attachment: testcase

Comment 25

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

I'm pretty sure this is a gfx/cairo bug. I've stepped through and everything down to and including nsThebesImage looks right. In this testcase, we're simply setting the pattern to the image surface and the userSpaceToImageSpace transform is a plain translation by -11,-11 which is correct. The bug only happens on Windows and it only happens when the top-left of the image is outside the fill area.

Comment 26

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

Attachment: slightly simpler testcase

This testcase is slightly simpler.

Comment 27

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

(In reply to comment #25)
> it only happens when the top-left of the image is
> outside the fill area.

Oops. I mean it only happens when the top-left of the image is in the interior of the fill area.

Comment 28

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

Attachment: even simpler testcase

We can trigger the bug using background-position instead of border.

Comment 29

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

This is happening because of a bug here:
http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-win32-surface.c#1217

src_r.x is -1, src_extents.width is 600 *and* it's an uint32_t. So -1 is coerced to unsigned int (i.e., 2^32-1) and then we take that mod 600, which returns 495. Not what we wanted.

Comment 30

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

Attachment: fix

This fixes it. Jeff, could you upstream this patch or something like it?

Comment 31

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

Bug 458487 triggered this because, before that change, we normalized tile coordinates in nsCSSRendering::PaintBackground in a way that avoided this bug. I got rid of all that normalization code because it wasn't needed, but that allowed this cairo bug to be exposed.

Comment 33

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 351353 [details] [diff] [review]
fix

Looks good to me. I'll upstream it.

Comment 34

[Serge Gautherie (:sgautherie)]

roc, what does "needs landing" whiteboard mean ?
Should it be "checkin-needed" keyword ? Does it mean to wait for update from upstream ? ...

Comment 35

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

It meas the patch needs to be landed. It doesn't mean the same as checkin-needed, which means "please land this patch for me".

Comment 36

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

Pushed eb91ec5673df

Comment 37

[Alfred Kayser]

Note, the commit message in HG has the wrong bug number:
"Bug 466268. Fix cairo-win32 bug that needed MOD but was using the C % operator. r=jmuizelaar"

Comment 38

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

Pushed af7b8328c34a to 1.9.1

Comment 39

[Serge Gautherie (:sgautherie)]

(In reply to comment #35)

Ah, noted :-|

*****

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20081212 Minefield/3.2a1pre] (nightly) (W2Ksp4)

V.Fixed

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b3pre) Gecko/20081212 SeaMonkey/2.0a3pre] (comm-central-win32/1229097559) (W2Ksp4)
(... + http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b4f285d733b6)

verified1.9.1