634762 - Page CSS causes corruption in chrome

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[bugzilla]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:2.0b12pre) Gecko/20110215 Firefox/4.0b12pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:2.0b12pre) Gecko/20110215 Firefox/4.0b12pre

The site, which only differs from other reddits because of some custom css (my eyes!) causes elements in toolbar chrome to go black.

Reproducible: Always

Steps to Reproduce:
1. Visit site
2. ..
3. mouse over toolbar elements

Comment 1

[bugzilla]

Attachment: screenshot

Comment 2

[bugzilla]

I should probably add that I think this might be DirectX issue, maybe even unique to my graphics card issue (X1950pro). I've had similar issues with nightlies from a month or two back, caused by flash elements. 
The issue doesn't happen in a VM running 32 W7 without DX.

Comment 3

[Alice0775 White]

Attachment: reduced html

http://hg.mozilla.org/mozilla-central/rev/b4aa47ca42c1
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b12pre) Gecko/20110216 Firefox/4.0b12pre ID:20110216030352

The problem happens if _disabled_ hardware acceleration.

Graphics
  Adapter Description: ATI Radeon HD 4300/4500 Series
  Vendor ID: 1002
  Device ID: 954f
  Adapter RAM: 512
  Adapter Drivers: aticfx64 aticfx64 aticfx32 aticfx32 atiumd64 atidxx64 atiumdag atidxx32 atiumdva atiumd6a atitmm64
  Driver Version: 8.812.0.0
  Driver Date: 1-4-2011
  Direct2D Enabled: false
  DirectWrite Enabled: false (6.1.7600.20830, font cache n/a)
  WebGL Renderer: Google Inc. -- ANGLE -- OpenGL ES 2.0 (ANGLE 0.0.0.541)
  GPU Accelerated Windows: 0/1

Comment 4

[Alice0775 White]

Regression window:
Works:
http://hg.mozilla.org/mozilla-central/rev/7aaf30721c48
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6pre) Gecko/20100902 Firefox/4.0b6pre ID:20100902204148
Fails:
http://hg.mozilla.org/mozilla-central/rev/4b879b793eb6
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6pre) Gecko/20100902 Firefox/4.0b6pre ID:20100902234753
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7aaf30721c48&tochange=4b879b793eb6

Comment 5

[Alice0775 White]

in local build,
build from 6e70745e6665 : fails
build from 6b4c1d5a9c14 : works

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

(Note: when viewing that screenshot, the rainbows and picture of Stalin is expected. Just the missing toolbar content is not.)

This needs to block, the question is: beta or final?

Comment 7

[Joe Drew (not getting mail)]

Rob, this is a regression from bug 579276, so I'm provisionally giving this to you.

I hate doing this, but I think this needs to block beta, especially if it's a clipping issue. They often have unforeseen consequences.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

The content here seems pretty unusual. It seems to require a 0 sized text frame with a text shadow. Seems like it shouldn't be that risky for a fix. I'd suggest that this doesn't need to block beta.

Comment 9

[Edo Karabas]

Hmm WFM.
Mozilla/5.0 (Windows NT 6.1; rv:2.0b12pre) Gecko/20110217 Firefox/4.0b12pre
Win 7 Pro 32bit SP1
Graphics
  Adapter Description: Intel(R) Graphics Media Accelerator 3150
  Vendor ID: 8086
  Device ID: a011
  Adapter RAM: Unknown
  Adapter Drivers: igdumdx32
  Driver Version: 8.14.10.2230
  Driver Date: 10-24-2010
  Direct2D Enabled: false
  DirectWrite Enabled: false (6.1.7601.17514, font cache 7,27 MB)
  WebGL Renderer: Google Inc. -- ANGLE -- OpenGL ES 2.0 (ANGLE 0.0.0.541)
  GPU Accelerated Windows: 0/1

Comment 10

[Jonathan Kew [:jfkthame]]

I'm able to reproduce with a current trunk build, but only after disabling both D2D and accelerated layers.

Adapter Description: NVIDIA Quadro FX 880M
Vendor ID: 10de
Device ID: 0a3c
Adapter RAM: 1024
Adapter Drivers: nvd3dumx,nvwgf2umx,nvwgf2umx nvd3dum,nvwgf2um,nvwgf2um
Driver Version: 8.17.12.5738
Driver Date: 6-27-2010
Direct2D Enabled: false
DirectWrite Enabled: false (6.1.7600.16699, font cache n/a)
WebGL Renderer: NVIDIA Corporation -- Quadro FX 880M/PCI/SSE2 -- 3.3.0
GPU Accelerated Windows: 0/1

Comment 11

[Timothy Nikkel (:tnikkel)]

Taking a blind stab in the dark that it is http://hg.mozilla.org/mozilla-central/rev/6e70745e6665 . Making that PushGroup always use CONTENT_COLOR_ALPHA would be a way to test that.

Comment 12

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

I see a lot of messages on the console:
_cairo_win32_surface_composite(BitBlt): The parameter is incorrect.
These messages are emitted when we paint the backbuffer to the window DC.

I don't see any other calls to _cairo_error.

Jeff, does comment #8 mean you have a minimized testcase? If not I'll try to make my own minimized testcase.

Comment 13

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

Needless to say the BitBlt parameters look OK. Perhaps some state in the source DC is bad.

Comment 14

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #12)
> Jeff, does comment #8 mean you have a minimized testcase? If not I'll try to
> make my own minimized testcase.

There's a reduced testcase attached to the bug.

Comment 15

[Please Ignore This Troll (Account Disabled)]

WFM on Win 7, Fx latest nightly (2011-02-17), graphics card is HD3850.

Direct2D Enabled true
DirectWrite Enabled true(6.1.7600.16699, font cache 27,31 MB)
WebGL RendererGoogle Inc. -- ANGLE -- OpenGL ES 2.0 (ANGLE 0.0.0.541)
GPU Accelerated Windows1/1 Direct3D 10

It is also WFM with d2d and dw disabled.

Comment 16

[Jonathan Kew [:jfkthame]]

(In reply to comment #15)
> It is also WFM with d2d and dw disabled.

Did you also set layers.acceleration.disabled=true? On my system that's required in order to reproduce this.

(In reply to comment #11)
> Taking a blind stab in the dark that it is
> http://hg.mozilla.org/mozilla-central/rev/6e70745e6665 . Making that PushGroup
> always use CONTENT_COLOR_ALPHA would be a way to test that.

I tried this but it didn't help; the bug is still present.

Comment 17

[Please Ignore This Troll (Account Disabled)]

(In reply to comment #16)
> Did you also set layers.acceleration.disabled=true? On my system that's
> required in order to reproduce this.

I tried and it is still the same: WFM.

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

roc: any ETA or ideas, here? Jeff and Joe asserted that this would be a betaN+ hardblocker, which means it gets a lot of my interest and focus :)

Comment 19

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #18)
> roc: any ETA or ideas, here? Jeff and Joe asserted that this would be a betaN+
> hardblocker, which means it gets a lot of my interest and focus :)

See comment 8 for my feelings on this. Further, we have a reproducible test case, so we should be pretty clear when this is fixed.

Comment 20

[Joe Drew (not getting mail)]

Timothy, since it's Saturday in NZ, I'm going to provisionally assign this to you. Ehsan has also been made aware of this bug.

Comment 21

[Bas Schouten (:bas.schouten)]

(In reply to comment #16)
> (In reply to comment #15)
> > It is also WFM with d2d and dw disabled.
> 
> Did you also set layers.acceleration.disabled=true? On my system that's
> required in order to reproduce this.

Wait, -just- setting layers.acceleration.disabled to true is not a supported rendering mode as it does D2D with basic layers. Make sure you use the menu's acceleration setting so D2D also gets disabled.

Comment 22

[Bas Schouten (:bas.schouten)]

I can reproduce this locally by switching off D2D and using D3D9 layers, -or- by using no accelerated layers or D2D.

Whenever we use the Direct2D cairo backend this does -not- occur.

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

tn: by when do you think we can expect a fix?

Comment 24

[Timothy Nikkel (:tnikkel)]

I don't think I can work on this as it doesn't reproduce in Linux or my XP vm and I can't debug in Windows Vista/7.

Comment 25

[(no longer active)]

(In reply to comment #11)
> Taking a blind stab in the dark that it is
> http://hg.mozilla.org/mozilla-central/rev/6e70745e6665 . Making that PushGroup
> always use CONTENT_COLOR_ALPHA would be a way to test that.

Bisection confirms this revision.

Comment 26

[(no longer active)]

So, something which is not right here is that gfxFont::Draw is calling SetupCairoFont, which in turn calls cairo_win32_scaled_font_select_font without wrapping it in SaveDC/ReleaseDC as it should.

We end up passing a matrix of NaN's to SetWorldTransform for the zero sized font, which fails, and corrupts our DC when we try to draw to it later on.

The quick fix for this bug would be to not pass that matrix to SetWorldTransform, but we should also investigate the problem in the first paragraph.

Comment 27

[(no longer active)]

Attachment: Patch (v1)

Comment 28

[(no longer active)]

Jeff believes that this was only blocking for investigation, so it doesn't need to block betaN any more.

Comment 29

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 513623 [details] [diff] [review]
Patch (v1)

Let's take this for now.

Comment 30

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

Paragraph #1 of comment #26 is exactly right. I think --- for drawing at least --- SetupCairoFont shouldn't call cairo_win32_scaled_font_select_font, since cairo's going to call it internally anyway. We only need that when we're going to bypass cairo APIs and use Uniscribe or GDI APIs directly. So I think the right fix would be for DCFromContext to be responsible for calling  cairo_win32_scaled_font_select_font on the currently selected scaled-font, and doing a SaveDC/RestoreDC.

Comment 31

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #30)

I agree.

Comment 32

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

Attachment: Deeper fix

This fixes the bug alone and seems like the right thing.

I'm not sure whether we need the first patch as well as this one. In theory (although not in practice, looking at the code), the error status could propagate out and contaminate the scaled font or even a cairo_t.

Comment 33

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

Although, this patch will make gfxGDIFont::GetGlyphWidth apply cairo_win32_scaled_font_select_font where it didn't before, as far as I can tell, which might break stuff :-(. Maybe DCFromContext needs a parameter to tell it whether to apply cairo_win32_scaled_font_select_font?

Comment 34

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 513623 [details] [diff] [review]
Patch (v1)

My review was too hasty. I think it would be safer to return CAIRO_STATUS_SUCCESS to avoid propagating the error code.

This also isn't a great place for _is_nan.

Plus, if we can make our usage of cairo_select_font correct I think we'd be better off.

Comment 35

[(no longer active)]

(In reply to comment #34)
> Comment on attachment 513623 [details] [diff] [review]
> Patch (v1)
> 
> My review was too hasty. I think it would be safer to return
> CAIRO_STATUS_SUCCESS to avoid propagating the error code.

Well, the caller handles it gracefully: <http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-win32-font.c#2073>, and I think it depends on the correct error code to be returned if the world transform matrix is set incorrectly.  What do you think?

> This also isn't a great place for _is_nan.

Where is a good place for that?

> Plus, if we can make our usage of cairo_select_font correct I think we'd be
> better off.

I would argue that we still need this patch, since passing a matrix with bogus values to GDI is really bad (especially since Win Vista/7 seem to happily take it and return a success code, as you observed today).  I don't think that a variant of attachment 513659 [details] [diff] [review] would make this unnecessary (and I also think that patch should be moved to its own bug).

Comment 36

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #35)
> (In reply to comment #34)
> > Comment on attachment 513623 [details] [diff] [review]
> > Patch (v1)
> > 
> > My review was too hasty. I think it would be safer to return
> > CAIRO_STATUS_SUCCESS to avoid propagating the error code.
> 
> Well, the caller handles it gracefully:
> <http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-win32-font.c#2073>,
> and I think it depends on the correct error code to be returned if the world
> transform matrix is set incorrectly.  What do you think?

But drawing with a zero sized font shouldn't be an error. See
http://lists.freedesktop.org/archives/cairo/2011-February/021633.html

> 
> > This also isn't a great place for _is_nan.
> 
> Where is a good place for that?

cairoint.h is probably the best place. There are also broken isfinite macro's scattered around the source code... I had a patch to fix that long ago, but it never got finished. 
> 
> > Plus, if we can make our usage of cairo_select_font correct I think we'd be
> > better off.
> 
> I would argue that we still need this patch, since passing a matrix with bogus
> values to GDI is really bad (especially since Win Vista/7 seem to happily take
> it and return a success code, as you observed today).  I don't think that a
> variant of attachment 513659 [details] [diff] [review] would make this unnecessary (and I also think that
> patch should be moved to its own bug).

I'm not convinced that passing a invalid matrix is bad. As long as we don't depend on something that's not true. It's not clear what effect different matrices have on other GDI functions. For example, if you're blocking a matrix with NaN's should we still accept a matrix with all 0's?

Further, the fact that Windows 7 doesn't return an error suggests that perhaps it's not that bad?

Comment 37

[Jonathan Kew [:jfkthame]]

Comment on attachment 513659 [details] [diff] [review]
Deeper fix

AFAICT, this doesn't work, because there are times when we call DCFromContext before the context has had its cairo_font set up. When this happens, we'll end up with the wrong font (at best), but it's worse than that -- if we're running under GDI, we end up with a cairo_dwrite_font_face (because cairo uses its "toy" font functions to create a default fallback, and that resolves to a dwrite implementation). That path leads to trouble.... the following crash stack "ought" to be impossible (note the gfxGDIFont frame - we should not end up in _cairo_dwrite_font_face_scaled_font_create within that), but it originates in this failure which led to internal use of a "toy" font:

>	xul.dll!_cairo_dwrite_font_face_scaled_font_create(void * abstract_face, const _cairo_matrix * font_matrix, const _cairo_matrix * ctm, const _cairo_font_options * options, _cairo_scaled_font * * font)  Line 443 + 0x10 bytes	C++
 	xul.dll!_moz_cairo_scaled_font_create(_cairo_font_face * font_face, const _cairo_matrix * font_matrix, const _cairo_matrix * ctm, const _cairo_font_options * options)  Line 1035 + 0x22 bytes	C
 	xul.dll!_cairo_gstate_ensure_scaled_font(_cairo_gstate * gstate)  Line 1699 + 0x21 bytes	C
 	xul.dll!_cairo_gstate_get_scaled_font(_cairo_gstate * gstate, _cairo_scaled_font * * scaled_font)  Line 1573 + 0x9 bytes	C
 	xul.dll!_moz_cairo_get_scaled_font(_cairo * cr)  Line 3139 + 0x10 bytes	C
 	xul.dll!DCFromContext::DCFromContext(gfxContext * aContext)  Line 91 + 0xe bytes	C++
 	xul.dll!gfxUniscribeShaper::InitTextRun(gfxContext * aContext, gfxTextRun * aTextRun, const wchar_t * aString, unsigned int aRunStart, unsigned int aRunLength, int aRunScript)  Line 471	C++
 	xul.dll!gfxGDIFont::InitTextRun(gfxContext * aContext, gfxTextRun * aTextRun, const wchar_t * aString, unsigned int aRunStart, unsigned int aRunLength, int aRunScript, int aPreferPlatformShaping)  Line 182 + 0x3f bytes	C++

Comment 38

[Jonathan Kew [:jfkthame]]

NB: the problem noted in comment #37 appears to only relate to gfxUniscribeShaper, not to text rendered through via harfbuzz. Therefore, to reproduce it you need to either set harfbuzz.level to zero, or visit a page with text in a script (such as Indic or Korean) that we don't handle via harfbuzz.

Comment 39

[Jonathan Kew [:jfkthame]]

Attachment: deeper fix, extended to fix resulting issues in uniscribe and gdi shapers

I think this addresses the problems caused by the preceding version: it changes gfxUniscribeShaper and gfxGDIShaper so that instead of calling DCFromContext and then ensuring the proper font is selected into the DC, they first ensure the font is set up in the context, and only then DCFromContext.

I've pushed this to try, as I'd like to check whether it affects WinXP Tp4 results at all - I'm not sure how much effect the changed Save/Restore behavior might have.

Comment 40

[Jonathan Kew [:jfkthame]]

Attachment: deeper fix, extended to fix resulting issues in uniscribe and gdi shapers (refreshed with proper context)

Sorry for the bugspam - the previous version didn't have the preferred context-diff settings, making it potentially harder to review.

Comment 41

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

Comment on attachment 513747 [details] [diff] [review]
deeper fix, extended to fix resulting issues in uniscribe and gdi shapers (refreshed with proper context)

This makes a lot more sense to me than our current code! I don't understand how doing SetupCairoFont followed by AutoSelectFont was working.

Comment 42

[Jonathan Kew [:jfkthame]]

http://hg.mozilla.org/mozilla-central/rev/36b58f89446d

Comment 43

[Philip Chee]

Did this cause the really bad font formatting in Bug 635768 ?

Comment 44

[Jonathan Kew [:jfkthame]]

(In reply to comment #43)
> Did this cause the really bad font formatting in Bug 635768 ?

Yes, it did. Happily, that is now fixed.