Bug 67543 – Text rendered with a simple (i.e. 0px blur) shadow inside a transparency layer has a double shadow

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Tim Horton]

Splitting this off from 65643, since it's totally unrelated to that bug (and was just exposed by a bug in the patch):

> If we set the shadow on a group, a transparency layer is created, and the shadow is cleared (because it's applied to the group, not the children). Then, when we go to draw the text shadow, if it has no blur (and can be rendered by simply redrawing the text), we a) store the shadow b) draw the text by hand in the shadow color c) restore the shadow. But, if we're in a transparency layer, the shadow is not clearable!
>
> So what happens in effect is that we draw the text shadow into the transparency layer, then when it's composited onto the screen, the group's shadow is applied, leading to the *shadow* having its own shadow!
>
>So, we can't use simple shadows inside a transparency layer. I assume the behavior is the same on other platforms (with the shadow clearing and all) because that makes the most sense. I think perhaps we should make the increment/decrement/inTransparencyLayer() stuff enabled on all platforms.
>
>I might be able to concoct a test for this that still exhibits after we fix the overdraw, but for right now it's made blindingly obvious in a variety of other tests because of that flaw in this patch.

[Radar WebKit Bug Importer]

<rdar://problem/10070536>

[Tim Horton]

Created attachment 106229 [details]
testcase

[Tim Horton]

Created attachment 106233 [details]
initial patch

[WebKit Review Bot]

Comment on attachment 106233 [details]
initial patch

Attachment 106233 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9583831

New failing tests:
svg/custom/simple-text-double-shadow.svg

[Darin Adler]

Windows build failure:

3>..\platform\win\ScrollbarThemeWin.cpp(274) : error C2039: 'inTransparencyLayer' : is not a member of 'WebCore::GraphicsContext'
3>..\platform\win\ScrollbarThemeWin.cpp(328) : error C2039: 'inTransparencyLayer' : is not a member of 'WebCore::GraphicsContext'
3>..\platform\win\ScrollbarThemeWin.cpp(387) : error C2039: 'inTransparencyLayer' : is not a member of 'WebCore::GraphicsContext'

3>c:\cygwin\home\buildbot\webkit\source\webcore\rendering\RenderThemeWin.cpp(677) : error C2039: 'inTransparencyLayer' : is not a member of 'WebCore::GraphicsContext'

3>..\plugins\win\PluginViewWin.cpp(639) : error C2039: 'inTransparencyLayer' : is not a member of 'WebCore::GraphicsContext'

[Tim Horton]

Created attachment 106477 [details]
fix windows build

[WebKit Review Bot]

Comment on attachment 106477 [details]
fix windows build

Attachment 106477 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9601403

New failing tests:
svg/custom/simple-text-double-shadow.svg

[Tim Horton]

Landed in 94714; now for grabbing new baselines and checking for build failures!

[Ryosuke Niwa]

It seems like this patch caused a whole bunch of tests to crash on GTK:
http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r94717%20(25820)/results.html

[Ryosuke Niwa]

(In reply to comment #9)
> It seems like this patch caused a whole bunch of tests to crash on GTK:
> http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r94717%20(25820)/results.html

On second thought, this might be an issue with the bot. Other GTK+ bots are fine even after your changeset.

[Ryosuke Niwa]

(In reply to comment #10)
> (In reply to comment #9)
> > It seems like this patch caused a whole bunch of tests to crash on GTK:
> > http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r94717%20(25820)/results.html
> 
> On second thought, this might be an issue with the bot. Other GTK+ bots are fine even after your changeset.

Nope. The same thing started to happen on Windows XP (Debug):
http://build.webkit.org/results/Windows%20XP%20Debug%20(Tests)/r94714%20(31934)/results.html

[Tim Horton]

Reopening due to rollout.

[Tim Horton]

Quick debugging on gtk-linux shows that m_transparencyCount rolls over to 32768. Not sure why; it's only manipulated in one place which has an assert (which isn't being hit) to guard against this (also, stepping through and printing m_transparencyCount shows that it's not a problem there, it becomes garbage in between). I'll debug more carefully tonight, since it's less clear what's going on now...

Also, I found an extra m_transparencyCount(0) on GraphicsContextPlatformPrivateCairo which would break gtk-windows build, so I'll remove that too. (that's certainly not causing this problem, though).

[Tim Horton]

Created attachment 106901 [details]
Fix crashes on Windows and Gtk

Turns out WindowsCG, WindowsCairo and Cairo overload the GraphicsContext constructor, so I just needed to add my initialization there (showed up instantly in valgrind).

[Darin Adler]

Comment on attachment 106901 [details]
Fix crashes on Windows and Gtk

review+ on the fix; Simon is still the reviewer on the change itself

[WebKit Review Bot]

Comment on attachment 106901 [details]
Fix crashes on Windows and Gtk

Clearing flags on attachment: 106901

Committed r94897: <http://trac.webkit.org/changeset/94897>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Nikolas Zimmermann]

This patch caused Qt bug 78332, uncovered by my repaint.js refactoring.
See my comment there:
"Hmpf, the problem is the m_transparencyCount generalization. Qt maintains a different count "layerCount" returned by isInTransparentLayer(), and it calls endTransparencyLayer too often, compared to all other ports - to support image clipping.

Cairo has similar needs (mask image operation during restore()) but doesn't suffer from the problem - can this be reused instead? I hope Zoltan can have a look."

The Qt code uses for (i=0; i < layerCount) endTransparencyLayer().
Layer count may be higher than m_transparencyCount, leading to the assertion that m_transparenyCount should always be > 0. Why is that? Qt doesn't always perform --m_layerCount, as endTransparencyLayer() does for m_transparencyCount.

The whole code is only needed to properly support ImageBuffer::clip() - Zoltan Herczeg is currently working on eliminating that, replacing by composite operations. Once that's done the custom layer count stuff could be removed from Qt again, fixing the assertion. That's probably the easiest - the current code from Tim makes a lot of sense!

(Note: Cairo/Gtk don't suffer from this problem, they don't maintain a separated 'layer count', but rely on m_transparencyCount)

[Eric Seidel (no email)]

Comment on attachment 106477 [details]
fix windows build

Cleared Simon Fraser's review+ from obsolete attachment 106477 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Tim Horton]

Niko, is there a reason you reopened this? The fix is still in, and bug 78332 is mysteriously fixed. I'm going to re-close it, feel free to let me know/reopen again if there was a reason.