Re-land scrollbar visual rect correction. |
||||
Issue descriptionFollow-up from http://crbug.com/597391 where http://crrev.com/1839533002/ reverted http://crrev.com/1825193002 due to apparently breaking: - rasterize_and_record_micro.key_mobile_sites_smooth - rasterize_and_record_micro.top_25_smooth on Linux.
,
Mar 29 2016
The crash is due to pictureBuilder.endRecording() returning nullptr due to a disabled GraphicsContext. See line 835 in PaintLayerCompositor.cpp in http://crrev.com/1825193002. However, the code here is an idiom seen many other places: ./third_party/WebKit/Source/core/paint/SVGShapePainter.cpp: pictureBuilder.endRecording()->playback(paintInfo.context.canvas()); ./third_party/WebKit/Source/core/page/PrintContextTest.cpp: pictureBuilder.endRecording()->playback(&canvas); ./third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp: pictureBuilder.endRecording()->playback(context.canvas()); ./third_party/WebKit/Source/web/PageWidgetDelegate.cpp: pictureBuilder.endRecording()->playback(canvas); ./third_party/WebKit/Source/web/tests/WebViewTest.cpp: pictureBuilder.endRecording()->playback(&canvas); ./third_party/WebKit/Source/web/WebLocalFrameImpl.cpp: pictureBuilder.endRecording()->playback(canvas); ./third_party/WebKit/Source/web/WebLocalFrameImpl.cpp: pictureBuilder.endRecording()->playback(canvas); ./third_party/WebKit/Source/web/PageOverlayTest.cpp: graphicsContext.endRecording()->playback(&canvas); ./third_party/WebKit/Source/platform/exported/WebScrollbarThemePainter.cpp: pictureBuilder.endRecording()->playback(canvas); ./third_party/WebKit/Source/platform/exported/WebScrollbarThemePainter.cpp: pictureBuilder.endRecording()->playback(canvas); ./third_party/WebKit/Source/platform/exported/WebScrollbarThemePainter.cpp: pictureBuilder.endRecording()->playback(canvas); ./third_party/WebKit/Source/platform/exported/WebScrollbarThemePainter.cpp: pictureBuilder.endRecording()->playback(canvas); ./third_party/WebKit/Source/platform/exported/WebScrollbarThemePainter.cpp: pictureBuilder.endRecording()->playback(canvas); ./third_party/WebKit/Source/platform/exported/WebScrollbarThemePainter.cpp: pictureBuilder.endRecording()->playback(canvas); ./third_party/WebKit/Source/platform/exported/WebScrollbarThemePainter.cpp: pictureBuilder.endRecording()->playback(canvas); ./third_party/WebKit/Source/platform/exported/WebScrollbarThemePainter.cpp: pictureBuilder.endRecording()->playback(canvas); ./third_party/WebKit/Source/platform/exported/WebScrollbarThemePainter.cpp: pictureBuilder.endRecording()->playback(canvas); ./third_party/WebKit/Source/platform/exported/WebScrollbarThemePainter.cpp: pictureBuilder.endRecording()->playback(canvas); ./third_party/WebKit/Source/platform/exported/WebFont.cpp: pictureBuilder.endRecording()->playback(canvas); so just adding a nullptr check to skip playback if the recording returns null rather than an SkPicture shouldn't be done without deeper understanding. Continuing to research re: what leads to a disabled context. Repro via applying patch, building and running: % ./tools/perf/run_benchmark run rasterize_and_record_micro.top_25_smooth --browser=content-shell-debug --chromium-output-directory=/ssd2/git/chrome/src/out/Debug -v --story-filter=https://www.google.com Crash seen on first test for https://www.google.com/#hl=en&q=barack+obama and Ctrl-C following.
,
Mar 29 2016
,
Mar 29 2016
It appears there are unsafe references to SkPictureBuilder.endRecording() in a number of places that must just happen not to be triggered by existing perf benchmarks. Though technically all calls should expect potential null return value and deal accordingly. The SkPictureBuilder class looks like it's a temporary solution per class header comment: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/platform/graphics/paint/SkPictureBuilder.h&sq=package:chromium&type=cs&rcl=1459265830&l=17 I believe that anywhere we create an SkPictureBuilder where we pass a non-null containingContext, that context could accordingly be disabled by the rasterize and record micro benchmark, and calls to endRecording() will then return nullptr. Other callsites should never return null, but are still technically in violation of the endRecording() API by ignoring potential null return value. Example usage where SkPictureBuilder is given a containing graphics context in: LayoutSVGResourceClipper LayoutSVGResourceMasker SVGShapePainter SVGImage GeneratedImage and in the patch associated with this bug, in PaintLayerCompositor.
,
Mar 30 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/825563e67be0a696f375fce91e6016cabcf0a673 commit 825563e67be0a696f375fce91e6016cabcf0a673 Author: wkorman <wkorman@chromium.org> Date: Wed Mar 30 13:58:16 2016 Deal gracefully with null {GraphicsContext,SkPictureBuilder}.endRecording() results. GraphicsContext supports the notion of being "disabled". Currently this is only used by the rasterize_and_record_micro benchmark. In this mode, both GraphicsContext.endRecording() and SkPictureBuilder.endRecording() can return null and our code is not currently robust against this result. Update callsites to deal with a null picture, and add documentation to GraphicsContext and SkPictureBuilder to make this clearer. BUG= 598358 Review URL: https://codereview.chromium.org/1841833002 Cr-Commit-Position: refs/heads/master@{#383971} [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/core/frame/LocalFrame.cpp [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/core/inspector/InspectorLayerTreeAgent.cpp [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceClipper.h [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceMasker.h [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/core/layout/svg/LayoutSVGResourcePattern.cpp [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/core/layout/svg/LayoutSVGResourcePattern.h [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/core/paint/SVGClipPainter.cpp [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/core/paint/SVGMaskPainter.cpp [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/core/paint/SVGShapePainter.cpp [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/core/svg/graphics/filters/SVGFEImage.cpp [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/platform/exported/WebFont.cpp [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/platform/exported/WebScrollbarThemePainter.cpp [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/platform/graphics/GeneratedImage.cpp [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/platform/graphics/GraphicsContext.h [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/platform/graphics/paint/DrawingRecorder.cpp [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/platform/graphics/paint/SkPictureBuilder.h [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/web/PageWidgetDelegate.cpp [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/web/WebLocalFrameImpl.cpp
,
Mar 30 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/825563e67be0a696f375fce91e6016cabcf0a673 commit 825563e67be0a696f375fce91e6016cabcf0a673 Author: wkorman <wkorman@chromium.org> Date: Wed Mar 30 13:58:16 2016 Deal gracefully with null {GraphicsContext,SkPictureBuilder}.endRecording() results. GraphicsContext supports the notion of being "disabled". Currently this is only used by the rasterize_and_record_micro benchmark. In this mode, both GraphicsContext.endRecording() and SkPictureBuilder.endRecording() can return null and our code is not currently robust against this result. Update callsites to deal with a null picture, and add documentation to GraphicsContext and SkPictureBuilder to make this clearer. BUG= 598358 Review URL: https://codereview.chromium.org/1841833002 Cr-Commit-Position: refs/heads/master@{#383971} [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/core/frame/LocalFrame.cpp [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/core/inspector/InspectorLayerTreeAgent.cpp [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceClipper.h [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceMasker.h [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/core/layout/svg/LayoutSVGResourcePattern.cpp [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/core/layout/svg/LayoutSVGResourcePattern.h [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/core/paint/SVGClipPainter.cpp [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/core/paint/SVGMaskPainter.cpp [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/core/paint/SVGShapePainter.cpp [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/core/svg/graphics/filters/SVGFEImage.cpp [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/platform/exported/WebFont.cpp [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/platform/exported/WebScrollbarThemePainter.cpp [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/platform/graphics/GeneratedImage.cpp [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/platform/graphics/GraphicsContext.h [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/platform/graphics/paint/DrawingRecorder.cpp [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/platform/graphics/paint/SkPictureBuilder.h [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/web/PageWidgetDelegate.cpp [modify] https://crrev.com/825563e67be0a696f375fce91e6016cabcf0a673/third_party/WebKit/Source/web/WebLocalFrameImpl.cpp
,
Mar 30 2016
Re-landed, forgot to update BUG= https://codereview.chromium.org/1844993002 |
||||
►
Sign in to add a comment |
||||
Comment 1 by wkorman@chromium.org
, Mar 28 2016