Bug 68133 – Crash in RenderBox::paintMaskImages when GraphicsContext's painting is disabled

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Julien Chaffraix]

Follow-up on bug 50151. See https://bugs.webkit.org/show_bug.cgi?id=50151#c2 for the test cases.

Those cases are crashing when run as part of a control tints update because the GraphicsContext is disabled and the code does not check this out. Running under control tints update is quite flakey, thus I intent to expose a way of testing this code path from DRT.

[Julien Chaffraix]

Created attachment 107438 [details]
WIP patch, missing Internal definition for several platform.

[WebKit Review Bot]

Comment on attachment 107438 [details]
WIP patch, missing Internal definition for several platform.

Attachment 107438 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9661774

[Collabora GTK+ EWS bot]

Comment on attachment 107438 [details]
WIP patch, missing Internal definition for several platform.

Attachment 107438 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9659980

[Julien Chaffraix]

Created attachment 107506 [details]
WIP patch 2, missing Internal definition for windows.

[Collabora GTK+ EWS bot]

Comment on attachment 107506 [details]
WIP patch 2, missing Internal definition for windows.

Attachment 107506 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9694214

[Julien Chaffraix]

Created attachment 107684 [details]
Patch 1: Solved compilation issue on Mac and hopefully Gtk. Missing Windows bits.

[Julien Chaffraix]

Created attachment 107947 [details]
Patch 2: Rebased patch 1. Missing Windows bits.

[Darin Adler]

Comment on attachment 107947 [details]
Patch 2: Rebased patch 1. Missing Windows bits.

View in context: https://bugs.webkit.org/attachment.cgi?id=107947&action=review

> Source/WebCore/ChangeLog:16
> +        RenderBox::paintMaskImages could not be called with GraphicsContext's disabled
> +        as it did not check for this condition. Pushed the checks up the stack to all its
> +        callers.

Sorry, I don’t understand. It’s supposed to be legal to work with a graphics context that has painting disabled. Checking whether it’s disabled is supposed to be for optimization purposes only. Maybe there is some kind of bug in the graphics context class? Or maybe something unusual in the paintMaskImages code that relies on painting not being disabled?

[WebKit Review Bot]

Comment on attachment 107947 [details]
Patch 2: Rebased patch 1. Missing Windows bits.

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

New failing tests:
scrollbars/scrollbar-orientation.html
scrollbars/scrollbar-middleclick-nopaste.html
scrollbars/overflow-scrollbar-combinations.html
svg/as-object/deep-nested-embedded-svg-size-changes-no-layout-triggers-1.html
scrollbars/disabled-scrollbar.html
scrollbars/scrollbar-buttons.html
svg/as-object/nested-embedded-svg-size-changes-no-layout-triggers-1.html
scrollbars/listbox-scrollbar-combinations.html
platform/mac-snowleopard/platform/mac/fast/text/international/Geeza-Pro-vertical-metrics-adjustment.html
svg/as-object/embedded-svg-size-changes-no-layout-triggers.html
scrollbars/scrollbar-click-does-not-blur-content.html
scrollbars/scrollbars-on-positioned-content.html
svg/as-object/deep-nested-embedded-svg-size-changes-no-layout-triggers-2.html
scrollbars/basic-scrollbar.html

[Julien Chaffraix]

Comment on attachment 107947 [details]
Patch 2: Rebased patch 1. Missing Windows bits.

View in context: https://bugs.webkit.org/attachment.cgi?id=107947&action=review

>> Source/WebCore/ChangeLog:16
>> +        callers.
> 
> Sorry, I don’t understand. It’s supposed to be legal to work with a graphics context that has painting disabled. Checking whether it’s disabled is supposed to be for optimization purposes only. Maybe there is some kind of bug in the graphics context class? Or maybe something unusual in the paintMaskImages code that relies on painting not being disabled?

Legal likely but that does not mean that all of the code is bullet proof to a GraphicsContext with painting disabled. In this case, paintMaskImages calls GraphicsContext::getCTM() which is not painting-disabled-aware and will happily crashes. The rest of the code mitigates with this very issue in the same way I did.

Looking back, my comment is misleading: it is GraphicsContext which is the culprit. I just wanted to underline that we pushed the painting disabled checks up the calling stack with this statement. An alternative fix would be to take care of this function (not sure what the downside would be).

[Darin Adler]

(In reply to comment #10)
> paintMaskImages calls GraphicsContext::getCTM() which is not painting-disabled-aware and will happily crashes.

We should fix that.

It’s OK if we also want to optimize by adding paintingDisabled checks at callers, but the right way to fix the crash is to fix the getCTM function.

[Julien Chaffraix]

(In reply to comment #11)
> (In reply to comment #10)
> > paintMaskImages calls GraphicsContext::getCTM() which is not painting-disabled-aware and will happily crashes.
> 
> We should fix that.
> 
> It’s OK if we also want to optimize by adding paintingDisabled checks at callers, but the right way to fix the crash is to fix the getCTM function.

Do you mind if me pushing fixing getCTM to another bug? (I feel this will need some unit testing as nobody really ensure that). This change is an optimization + a crash fix that is proven.

[Julien Chaffraix]

Created attachment 108080 [details]
Patch 3: With Windows bits and reworded CL.

[Darin Adler]

(In reply to comment #12)
> Do you mind if me pushing fixing getCTM to another bug?

I’d prefer not to do that. To me it seems wrong to deal with the getCTM problem by adding code at another level to work around it.

I am not sure these optimizations are valuable; checking them in as the crash fix doesn’t seem right to me.

[Julien Chaffraix]

Created attachment 108220 [details]
Patch 4: Fix GraphicsContext::getCTM on all platform.

[Darin Adler]

Comment on attachment 108220 [details]
Patch 4: Fix GraphicsContext::getCTM on all platform.

Lets land the getCTM changes and the tests only. Then we can land the other changes separately.

[Julien Chaffraix]

(In reply to comment #16)
> (From update of attachment 108220 [details])
> Lets land the getCTM changes and the tests only. Then we can land the other changes separately.

Sorry I am not following, what other changes is there? If you mean the window.internals plumbing, it is needed for the tests to work (furthermore it passed all the EWS on the previous patch).

[Darin Adler]

(In reply to comment #17)
> If you mean the window.internals plumbing, it is needed for the tests to work (furthermore it passed all the EWS on the previous patch).

I was confused by the paintControlTints refactoring.

[Julien Chaffraix]

Comment on attachment 108220 [details]
Patch 4: Fix GraphicsContext::getCTM on all platform.

Thanks Darin.

[WebKit Review Bot]

Comment on attachment 108220 [details]
Patch 4: Fix GraphicsContext::getCTM on all platform.

Rejecting attachment 108220 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
e384e115e51178b37357bb35abeb8e29f21f0fb3
r95669 = 9f83881096286587ca5f93e7e56fed85dff61091
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.
Updating chromium port dependencies using gclient...

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/9767898

[Julien Chaffraix]

(In reply to comment #20)
> (From update of attachment 108220 [details])
> Rejecting attachment 108220 [details] from commit-queue.

The ChangeLog got borked when I updated it for patch 4. I will update it soon for landing.

[Julien Chaffraix]

Created attachment 108243 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 108243 [details]
Patch for landing

Clearing flags on attachment: 108243

Committed r95685: <http://trac.webkit.org/changeset/95685>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.