Assigning to the concern owner from CL --
https://chromium.googlesource.com/chromium/src/+log/18281c41a2b809619b3f533c76033e1a14729944..8811aed463ee87b0302ff8a80a730ebd86dee515?pretty=fuller

Suspecting Commit#
https://chromium.googlesource.com/chromium/src/+/e49d70a44bf12043cc781390f7fc53ce7b4ac807

@enne -- Could you please look into the issue, kindly re-assign if this is not related to your changes.
Thank You.

ClusterFuzz has detected this issue as fixed in range 465194:465207.

Detailed report: https://clusterfuzz.com/testcase?key=5697340318154752

Fuzzer: attekett_surku_fuzzer
Job Type: linux_lsan_chrome_mp
Platform Id: linux

Crash Type: Direct-leak
Crash Address: 
Crash State:
  SkPictureShader::Make
  SkShader::MakePictureShader
  MakePaintShaderRecord
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_lsan_chrome_mp&range=464801:464815
Fixed: https://clusterfuzz.com/revisions?job=linux_lsan_chrome_mp&range=465194:465207

Reproducer Testcase: https://clusterfuzz.com/download/AMIfv97njNVxkMm5o1vE2meC-2nJx5AznEGouMs7-53DyYL2iET2R_hy5DPc1oqHfbOZt3sJg50R9c5qRDKkic2EqWBttO3iLHFkhCpJ73d6IbNnmPFyWQ3AdX5daExcgeL1rSjsy1F8y60wCxieMdvO1ZfAhZE1mUYXaEi20ym_sRgT7l8xnhRWhp0yXd143pPhbK8WGLgj0NhrGIlxRVZI1Os5EyIn2prVd7INATKc3Lc-YV9o_6mq6QqBtXZECC5caAFRcBBTbtOR6f6kF9EFj0XqoOQhjIgQyrKnGvkGmwFTJR_swqIN61_AZxBcAUCLoalHE6Q5gE8eMq8m6v1L0EfGs3aTzPaBx8xcLpb9o5iVXFNqUEw?testcase_id=5697340318154752


Additional requirements: Requires Gestures

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

ClusterFuzz testcase 5697340318154752 is verified as fixed, so closing issue.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.

I see 2 SkPictureShaders get created and 1 get destroyed during startup. And none get destroyed when I Control-Shift-Q and exit chrome, both on TOT and with the patch. Which means:
- No change in behaviour happened
- Or, clusterfuzz is getting chrome to run in a different path than me, like it's somehow getting a slow shutdown path and i'm getting a fast one?
- Or, it was suppressed before but isn't with the patch - but I don't see any leak get reported locally with the patch.

I have discovered ASAN_OPTIONS. Too bad clusterfuzz doesn't tell you what environment it is using. Now I get this on TOT:


[142784:142798:0419/190233.945784:ERROR:SkPictureShader.cpp(111)] ~SkPictureShader

=================================================================
==142728==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 96 byte(s) in 4 object(s) allocated from:
    #0 0x557b6e5b1756 in __interceptor_calloc (/usr/local/google/home/danakj/s/c/src/out_desktop/Release/chrome+0x308a756)
    #1 0x7f4ef0fe6668 in g_malloc0 (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4e668)

-----------------------------------------------------
Suppressions used:
  count      bytes template
    419      21696 libfontconfig
    303       4060 __strdup
-----------------------------------------------------

SUMMARY: AddressSanitizer: 96 byte(s) leaked in 4 allocation(s).



And I get this with the patch:



[146917:146934:0419/190528.964677:ERROR:SkPictureShader.cpp(111)] ~SkPictureShader

=================================================================
==146734==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 96 byte(s) in 4 object(s) allocated from:
    #0 0x55db83af08d6 in __interceptor_calloc (/usr/local/google/home/danakj/s/c/src/out_desktop/Release/chrome+0x308b8d6)
    #1 0x7f4461ccd668 in g_malloc0 (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4e668)

-----------------------------------------------------
Suppressions used:
  count      bytes template
    419      21696 libfontconfig
    303       4060 __strdup
-----------------------------------------------------

SUMMARY: AddressSanitizer: 96 byte(s) leaked in 4 allocation(s).
=================================================================
==146917==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 88 byte(s) in 1 object(s) allocated from:
    #0 0x5624e9e8e262 in operator new(unsigned long) (/usr/local/google/home/danakj/s/c/src/out_desktop/Release/chrome+0x30b7262)
    #1 0x5624f1ac0f27 in SkPictureShader::Make(sk_sp<SkPicture>, SkShader::TileMode, SkShader::TileMode, SkMatrix const*, SkRect const*) third_party/skia/src/core/SkPictureShader.cpp:119:28
    #2 0x5624f1b518c6 in SkShader::MakePictureShader(sk_sp<SkPicture>, SkShader::TileMode, SkShader::TileMode, SkMatrix const*, SkRect const*) third_party/skia/src/core/SkShader.cpp:243:12
    #3 0x5624ffa9cd0f in MakePaintShaderRecord cc/paint/paint_shader.h:31:10
    #4 0x5624ffa9cd0f in blink::PaintRecordPattern::CreateShader(SkMatrix const&) third_party/WebKit/Source/platform/graphics/PaintRecordPattern.cpp:35
    #5 0x5624ff8e7497 in blink::Pattern::ApplyToFlags(cc::PaintFlags&, SkMatrix const&) third_party/WebKit/Source/platform/graphics/Pattern.cpp:63:22
    #6 0x5624ffa9a804 in blink::GeneratedImage::DrawPattern(blink::GraphicsContext&, blink::FloatRect const&, blink::FloatSize const&, blink::FloatPoint const&, SkBlendMode, blink::FloatRect const&, blink::FloatSize const&) third_party/WebKit/Source/platform/graphics/GeneratedImage.cpp:67:12
    #7 0x5624f83c0b72 in blink::Image::DrawTiledBackground(blink::GraphicsContext&, blink::FloatRect const&, blink::FloatPoint const&, blink::FloatSize const&, SkBlendMode, blink::FloatSize const&) third_party/WebKit/Source/platform/graphics/Image.cpp:122:3
    #8 0x5624f83b9a62 in blink::GraphicsContext::DrawTiledImage(blink::Image*, blink::FloatRect const&, blink::FloatPoint const&, blink::FloatSize const&, SkBlendMode, blink::FloatSize const&) third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp:933:10
    #9 0x5624fa4b5431 in blink::BoxPainter::PaintFillLayer(blink::LayoutBoxModelObject const&, blink::PaintInfo const&, blink::Color const&, blink::FillLayer const&, blink::LayoutRect const&, blink::BackgroundBleedAvoidance, blink::InlineFlowBox const*, blink::LayoutSize const&, SkBlendMode, blink::LayoutObject const*) third_party/WebKit/Source/core/paint/BoxPainter.cpp:737:15
    #10 0x5624fa4b1962 in blink::BoxPainter::PaintFillLayers(blink::PaintInfo const&, blink::Color const&, blink::FillLayer const&, blink::LayoutRect const&, blink::BackgroundBleedAvoidance, SkBlendMode, blink::LayoutObject const*) third_party/WebKit/Source/core/paint/BoxPainter.cpp:281:5
    #11 0x5624fa4b1083 in blink::BoxPainter::PaintBackground(blink::PaintInfo const&, blink::LayoutRect const&, blink::Color const&, blink::BackgroundBleedAvoidance) third_party/WebKit/Source/core/paint/BoxPainter.cpp:222:3
    #12 0x5624fa4adf3c in blink::BoxPainter::PaintBoxDecorationBackgroundWithRect(blink::PaintInfo const&, blink::LayoutPoint const&, blink::LayoutRect const&) third_party/WebKit/Source/core/paint/BoxPainter.cpp:182:5
    #13 0x5624fa4ac98f in blink::BoxPainter::PaintBoxDecorationBackground(blink::PaintInfo const&, blink::LayoutPoint const&) third_party/WebKit/Source/core/paint/BoxPainter.cpp:88:3
    #14 0x5624f9d38f6e in blink::LayoutBox::PaintBoxDecorationBackground(blink::PaintInfo const&, blink::LayoutPoint const&) const third_party/WebKit/Source/core/layout/LayoutBox.cpp:1608:21
    #15 0x5624fa4a194e in blink::BlockPainter::PaintObject(blink::PaintInfo const&, blink::LayoutPoint const&) third_party/WebKit/Source/core/paint/BlockPainter.cpp:175:21
    #16 0x5624f9c6f42e in blink::LayoutBlock::PaintObject(blink::PaintInfo const&, blink::LayoutPoint const&) const third_party/WebKit/Source/core/layout/LayoutBlock.cpp:901:23
    #17 0x5624fa49e2b5 in blink::BlockPainter::Paint(blink::PaintInfo const&, blink::LayoutPoint const&) third_party/WebKit/Source/core/paint/BlockPainter.cpp:53:19
    #18 0x5624f9c6f18e in blink::LayoutBlock::Paint(blink::PaintInfo const&, blink::LayoutPoint const&) const third_party/WebKit/Source/core/layout/LayoutBlock.cpp:891:23
    #19 0x5624fa523f03 in blink::ObjectPainter::PaintAllPhasesAtomically(blink::PaintInfo const&, blink::LayoutPoint const&) third_party/WebKit/Source/core/paint/ObjectPainter.cpp:695:18
    #20 0x5624fa4a0ce4 in PaintAllChildPhasesAtomically third_party/WebKit/Source/core/paint/BlockPainter.cpp:133:26
    #21 0x5624fa4a0ce4 in blink::BlockPainter::PaintChildrenOfFlexibleBox(blink::LayoutFlexibleBox const&, blink::PaintInfo const&, blink::LayoutPoint const&) third_party/WebKit/Source/core/paint/BlockPainter.cpp:123
    #22 0x5624fa4a2a8f in PaintContents third_party/WebKit/Source/core/paint/BlockPainter.cpp:309:17
    #23 0x5624fa4a2a8f in blink::BlockPainter::PaintObject(blink::PaintInfo const&, blink::LayoutPoint const&) third_party/WebKit/Source/core/paint/BlockPainter.cpp:242
    #24 0x5624f9c6f42e in blink::LayoutBlock::PaintObject(blink::PaintInfo const&, blink::LayoutPoint const&) const third_party/WebKit/Source/core/layout/LayoutBlock.cpp:901:23
    #25 0x5624fa49e3d9 in blink::BlockPainter::Paint(blink::PaintInfo const&, blink::LayoutPoint const&) third_party/WebKit/Source/core/paint/BlockPainter.cpp:62:19
    #26 0x5624f9c6f18e in blink::LayoutBlock::Paint(blink::PaintInfo const&, blink::LayoutPoint const&) const third_party/WebKit/Source/core/layout/LayoutBlock.cpp:891:23
    #27 0x5624fa4a0856 in PaintChild third_party/WebKit/Source/core/paint/BlockPainter.cpp:113:11
    #28 0x5624fa4a0856 in blink::BlockPainter::PaintChildren(blink::PaintInfo const&, blink::LayoutPoint const&) third_party/WebKit/Source/core/paint/BlockPainter.cpp:103
    #29 0x5624f9c6f2de in blink::LayoutBlock::PaintChildren(blink::PaintInfo const&, blink::LayoutPoint const&) const third_party/WebKit/Source/core/layout/LayoutBlock.cpp:896:23
    #30 0x5624fa4a35ad in blink::BlockPainter::PaintContents(blink::PaintInfo const&, blink::LayoutPoint const&) third_party/WebKit/Source/core/paint/BlockPainter.cpp:309:17
    #31 0x5624fa527477 in blink::BlockFlowPainter::PaintContents(blink::PaintInfo const&, blink::LayoutPoint const&) third_party/WebKit/Source/core/paint/BlockFlowPainter.cpp:27:38
    #32 0x5624fa4a2b72 in blink::BlockPainter::PaintObject(blink::PaintInfo const&, blink::LayoutPoint const&) third_party/WebKit/Source/core/paint/BlockPainter.cpp:236:26
    #33 0x5624f9c6f42e in blink::LayoutBlock::PaintObject(blink::PaintInfo const&, blink::LayoutPoint const&) const third_party/WebKit/Source/core/layout/LayoutBlock.cpp:901:23

SUMMARY: AddressSanitizer: 88 byte(s) leaked in 1 allocation(s).


Implying it sees a leak in the browser process on TOT but not in the renderer, even though the destructor doesn't run in either case.

To be even more explicit here's printing in SkPictureShader constructor and destructor on TOT:

[152121:152121:0419/191312.653908:ERROR:SkPictureShader.cpp(108)] new SkPictureShader
[152121:152121:0419/191312.700246:ERROR:SkPictureShader.cpp(108)] new SkPictureShader
[152121:152136:0419/191312.709276:ERROR:SkPictureShader.cpp(112)] ~SkPictureShader


The 2nd one is leaked.

Here's more from TOT, in case the memory is being freed but the destructor isn't run? SkLiteDL is not used at all afaict there for this case.

[153919:153919:0419/192355.570639:ERROR:SkMiniRecorder.cpp(35)] 0x60d000045890 SkMiniPicture()
[153919:153919:0419/192357.647583:ERROR:SkMiniRecorder.cpp(39)] 0x60d000045890 ~SkMiniPicture()
[153863:153863:0419/192357.701440:ERROR:SkMiniRecorder.cpp(35)] 0x60d0001438b0 SkMiniPicture()
[153863:153863:0419/192357.705750:ERROR:SkMiniRecorder.cpp(35)] 0x60d000143b20 SkMiniPicture()
[153919:153919:0419/192357.746951:ERROR:SkMiniRecorder.cpp(35)] 0x60d000080980 SkMiniPicture()
[153919:153919:0419/192357.747724:ERROR:SkMiniRecorder.cpp(35)] 0x60d000080a50 SkMiniPicture()
[153919:153919:0419/192357.747929:ERROR:SkPictureShader.cpp(108)] 0x608000043720 SkPictureShader()
[153919:153919:0419/192357.748390:ERROR:SkMiniRecorder.cpp(35)] 0x60d000080b20 SkMiniPicture()
[153919:153919:0419/192357.748682:ERROR:SkMiniRecorder.cpp(35)] 0x60d000080bf0 SkMiniPicture()
[153919:153919:0419/192357.817768:ERROR:SkMiniRecorder.cpp(35)] 0x60d000087250 SkMiniPicture()
[153919:153919:0419/192357.818258:ERROR:SkMiniRecorder.cpp(35)] 0x60d000087320 SkMiniPicture()
[153919:153919:0419/192357.818476:ERROR:SkPictureShader.cpp(108)] 0x608000045a20 SkPictureShader()
[153919:153919:0419/192357.818778:ERROR:SkMiniRecorder.cpp(35)] 0x60d0000873f0 SkMiniPicture()
[153919:153919:0419/192357.819097:ERROR:SkMiniRecorder.cpp(35)] 0x60d0000874c0 SkMiniPicture()
[153919:153931:0419/192357.823538:ERROR:SkMiniRecorder.cpp(39)] 0x60d000080980 ~SkMiniPicture()
[153919:153931:0419/192357.823838:ERROR:SkPictureShader.cpp(112)] 0x608000043720 ~SkPictureShader()
[153919:153931:0419/192357.824065:ERROR:SkMiniRecorder.cpp(39)] 0x60d000080b20 ~SkMiniPicture()
[153919:153931:0419/192357.828937:ERROR:SkMiniRecorder.cpp(39)] 0x60d000080bf0 ~SkMiniPicture()
<--- I exit chrome here --->
[153863:153863:0419/192411.810912:ERROR:SkMiniRecorder.cpp(39)] 0x60d0001438b0 ~SkMiniPicture()
[153863:153863:0419/192411.811098:ERROR:SkMiniRecorder.cpp(39)] 0x60d000143b20 ~SkMiniPicture()

153919 is the renderer. You see 0x60d0000873f0 and 0x60d0000874c0 get created but never destroyed. One of them has the SkPictureShader in it judging from the other create-free pattern.

So what is suppressing the leak on TOT?

+inferno halp why are skia leaks being suppressed I can't find the suppressions.

OK I was advised by gab@ to check if maybe the timing of things vs the leak checking are different at shutdown, but it seems not related.

TOT sees the child_thread_impl do its leak check when it loses its IPC channel to the browser and pass.

However when this happens with the patch, there is a leak. This means there's some allocation that lsan knows about but can't find a pointer to.

It seems possible to me that SkMiniPicture stores the Op which has a pointer that the sanitizer can understand, but our PaintOpBuffer is using a char array which the sanitizer doesn't understand so it thinks the pointer is gone.

SkMiniPicture has an Op of type T: https://cs.chromium.org/chromium/src/third_party/skia/src/core/SkMiniRecorder.cpp?rcl=d0ce148ed4945aa75fb7eeaaffcfd345dd9f85fb&l=53

I believe the op would be a DrawRect https://cs.chromium.org/chromium/src/third_party/skia/include/private/SkRecords.h?rcl=5338f99a8a75d0e7622a37c5d1c05fcce49f55f5&l=303 which has an SkPaint with the shader pointer in it.

Whereas PaintOpBuffer has a char array: https://chromium.googlesource.com/chromium/src/+/e49d70a44bf12043cc781390f7fc53ce7b4ac807%5E%21/#F10

+  char first_op_[sizeof(LargestPaintOp)];

I confirmed that if I get rid of the first_op_ optimization and always put NoopOp there, then the leak stops being reported. But data_ is a char array reinterpret-casted around also. Maybe first_op_ is not alligned right?

Also confirmed if I replace char[] with base::AlignedMemory the leak goes away. But diff ops have different alignments so this seems sketchy and I think we need to use a union.

vmpstr confirms it's ok as long as the alignment of T <= the alignment of the storage, since they are always powers of 2 a thing that needs 4 is still aligned if the storage is using 16.

Here's a patch to fix this bug:

diff --git a/cc/paint/paint_op_buffer.h b/cc/paint/paint_op_buffer.h
index b5f60a4dff15..d625153eb05e 100644
--- a/cc/paint/paint_op_buffer.h
+++ b/cc/paint/paint_op_buffer.h
@@ -8,6 +8,7 @@
 #include <stdint.h>
 
 #include "base/logging.h"
+#include "base/memory/aligned_memory.h"
 #include "cc/paint/paint_canvas.h"
 #include "cc/paint/paint_export.h"
 #include "cc/paint/paint_flags.h"
@@ -571,7 +572,7 @@ class CC_PAINT_EXPORT PaintOpBuffer : public SkRefCnt {
   const SkRect& cullRect() const { return cull_rect_; }
 
   PaintOp* GetFirstOp() const {
-    return reinterpret_cast<PaintOp*>(const_cast<char*>(&first_op_[0]));
+    return const_cast<PaintOp*>(first_op_.data_as<PaintOp>());
   }
 
   template <typename T, typename... Args>
@@ -728,7 +729,13 @@ class CC_PAINT_EXPORT PaintOpBuffer : public SkRefCnt {
           // to be a common case.
           push<NoopOp>();
         } else {
-          T* op = reinterpret_cast<T*>(&first_op_[0]);
+          // |first_op_| is aligned to LargestPaintOp. If T needs a smaller
+          // alignment, this is okay because it will be a factor of the actual
+          // alignment being used (as they are always a power of 2). If T needs
+          // a larger alignment, that is bad and we should use T to choose the
+          // alignment of |first_op_| instead.
+          static_assert(ALIGNOF(T) <= ALIGNOF(LargestPaintOp), "");
+          auto* op = reinterpret_cast<T*>(first_op_.data_as<T>());
           new (op) T{std::forward<Args>(args)...};
           op->type = static_cast<uint32_t>(T::kType);
           op->skip = 0;
@@ -762,7 +769,8 @@ class CC_PAINT_EXPORT PaintOpBuffer : public SkRefCnt {
 
   // As a performance optimization because n=1 is an extremely common case just
   // store the first op in the PaintOpBuffer itself to avoid an extra alloc.
-  char first_op_[sizeof(LargestPaintOp)];
+  base::AlignedMemory<sizeof(LargestPaintOp), ALIGNOF(LargestPaintOp)>
+      first_op_;
   SkAutoTMalloc<char> data_;
   size_t used_ = 0;
   size_t reserved_ = 0;

 Issue 711903  has been merged into this issue.

Issue 712336 has been merged into this issue.

 Issue 711964  has been merged into this issue.

Issue 712102 has been merged into this issue.

Could this be the cause of the crash in SkFindAndPlaceGlyph::ArbitraryPositions::nextPoint() in  issue 712204 ?

It's not clear to me from the comments here whether this fixes a leak or fixes memory corruption that could lead to crashes.