| Summary: | Crash in RenderBox::paintMaskImages when GraphicsContext's painting is disabled | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Julien Chaffraix <jchaffraix> |
| Component: | Layout and Rendering | Assignee: | Julien Chaffraix <jchaffraix> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | darin, dglazkov, gustavo.noronha, gustavo, inferno, skylined, webkit.review.bot, xan.lopez |
| Priority: | P2 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | All | ||
| OS: | All | ||
| Bug Depends on: | |||
| Bug Blocks: | 68566 | ||
| Attachments: | |||
|
Description
Julien Chaffraix
2011-09-14 16:38:15 PDT
Created attachment 107438 [details]
WIP patch, missing Internal definition for several platform.
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 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 Created attachment 107506 [details]
WIP patch 2, missing Internal definition for windows.
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 Created attachment 107684 [details]
Patch 1: Solved compilation issue on Mac and hopefully Gtk. Missing Windows bits.
Created attachment 107947 [details]
Patch 2: Rebased patch 1. Missing Windows bits.
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? 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 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). (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. (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. Created attachment 108080 [details]
Patch 3: With Windows bits and reworded CL.
(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. Created attachment 108220 [details]
Patch 4: Fix GraphicsContext::getCTM on all platform.
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.
(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). (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. Comment on attachment 108220 [details]
Patch 4: Fix GraphicsContext::getCTM on all platform.
Thanks Darin.
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 (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. Created attachment 108243 [details]
Patch for landing
Comment on attachment 108243 [details] Patch for landing Clearing flags on attachment: 108243 Committed r95685: <http://trac.webkit.org/changeset/95685> All reviewed patches have been landed. Closing bug. |