New issue
Advanced search Search tips

Issue 619893 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Jun 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug



Sign in to add a comment

Direct-leak in sk_sp<SkImage_Raster> sk_make_sp<SkImage_Raster, SkBitmap const&>

Project Member Reported by ClusterFuzz, Jun 14 2016

Issue description

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5447346372739072

Fuzzer: inferno_layout_test_unmodified
Job Type: linux_lsan_chrome_mp
Platform Id: linux

Crash Type: Direct-leak
Crash Address: 
Crash State:
  sk_sp<SkImage_Raster> sk_make_sp<SkImage_Raster, SkBitmap const&>
  SkMakeImageFromRasterBitmap
  SkImage::MakeFromBitmap
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_lsan_chrome_mp&range=209699:209703

Unminimized Testcase: https://cluster-fuzz.appspot.com/download/AMIfv96thKP2CjeemGVshKdR6Q_alCRR4SXUUtU_NCOuRn3-yKCqquI9gfsz1U2i7bmNIrh0KK-Npr3OluQQFD3_FGZzy-t6071xIMkyqSPhNsQZe5UEYutWqG-8-nrK883FHlVweBMLRLHiu54aSrIz82AGLsLsBw


Additional requirements: Requires Gestures

Filer: durga.behera

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.
 
Cc: xidac...@chromium.org
Components: Internals>Skia Tools>Test>FindIt>CorrectResult
Labels: findit-for-crash Te-Logged M-53
Owner: yutak@chromium.org
Status: Assigned (was: Available)
Suspected CLs:
=================
No CL in the regression range changes the crashed files. The result is the blame information.

Author: xidachen
Project: chromium
Changelist: https://chromium.googlesource.com/chromium/src//+/13834522ca6d7969f80743285dff0e09050b8227
Time: Tue Mar 01 02:37:15 2016
The CL last changed line 111 of file ImageBitmap.cpp, which is stack frame 5.

Author: yutak
Project: chromium
Changelist: https://chromium.googlesource.com/chromium/src//+/995cd784dac8a41053410ca3fae7f7b57cd4ebad
Time: Mon May 16 07:50:26 2016
The CL last changed line 241 of file ImageBitmapFactories.cpp, which is stack frame 6.

Author: finnur
Project: chromium
Changelist: https://chromium.googlesource.com/chromium/src//+/54fbc836be8606f1a4f65e5b32411f5ffc68750c
Time: Thu Mar 03 10:41:04 2016
The CL last changed line 350 of file Functional.h, which is stack frame 7.

Suspected Project: chromium-skia
Suspected Component: Internals>Skia
==========================
Currently its impacting the Head.
From the above Cl list suspecting the changes made to file ImageBitmapFactories.cpp, which is stack frame 6.
Suspect : https://codereview.chromium.org/1983753002
yutak@ : Could you please take a look into this if its related to your change.
xidachen@ : cced you referring to  issue#611339 .

Comment 2 by hcm@chromium.org, Jun 14 2016

Cc: robertphillips@chromium.org hcm@chromium.org yutak@chromium.org reed@google.com
Owner: fmalita@chromium.org
Skia probably needs to take a look here with a lot of recent changes to smart pointers- Robert is out, but perhaps fmalita or reed can help.

Comment 3 by reed@google.com, Jun 14 2016

The leak appears to be the SkImage object itself, and not the underlying pixels.

That image (frame) is passed to an (async?) queue via

    taskRunner->postTask(BLINK_FROM_HERE, threadSafeBind(&ImageBitmapFactories::ImageBitmapLoader::resolvePromiseOnOriginalThread, wrapCrossThreadPersistent(this), frame.release()));

Can't tell from the stack-trace who is responsible for picking up the frame on the otherside of that task.

I think the SkImage ends up in https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/imagebitmap/ImageBitmapFactories.cpp?rcl=0&l=246, and AFAICT it's not supposed to escape that frame.  Looking.
fmalita@: I am a little confused, the stack trace seems to say that the leak happens on this line:
adoptRef(SkImage::NewFromBitmap(bitmap));

By looking at this line, I cannot tell how it would cause any leak. We can certain change it to use MakeFromBitmap, but I don't think that is going to change anything.
The stack is only telling us where the leaked object was allocated, but gives no indication of where the leak occurred (someone can call an unbalanced ref() at any point after the allocation).

I think I see the problem though: https://codereview.chromium.org/2064273003/
Project Member

Comment 7 by bugdroid1@chromium.org, Jun 15 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9315dfe2897c1586d4d54bfae5fc876345bf1c8f

commit 9315dfe2897c1586d4d54bfae5fc876345bf1c8f
Author: fmalita <fmalita@chromium.org>
Date: Wed Jun 15 09:43:36 2016

Fix a SkImage leak in ImageBitmap:cropImage()

We're currently using the raw pointer API for image subsetting, and not
adopting the result immediately.  Then if we take the
unPremulSkImageToPremul() path, we leak.

Convert to the safer sk_sp API and adopt into a RefPtr to avoid leaking.

R=reed@google.com,xidachen@chromium.org
BUG= 619893 

Review-Url: https://codereview.chromium.org/2064273003
Cr-Commit-Position: refs/heads/master@{#399869}

[modify] https://crrev.com/9315dfe2897c1586d4d54bfae5fc876345bf1c8f/third_party/WebKit/Source/core/frame/ImageBitmap.cpp

Project Member

Comment 8 by ClusterFuzz, Jun 15 2016

ClusterFuzz has detected this issue as fixed in range 399855:399884.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5447346372739072

Fuzzer: inferno_layout_test_unmodified
Job Type: linux_lsan_chrome_mp
Platform Id: linux

Crash Type: Direct-leak
Crash Address: 
Crash State:
  sk_sp<SkImage_Raster> sk_make_sp<SkImage_Raster, SkBitmap const&>
  SkMakeImageFromRasterBitmap
  SkImage::MakeFromBitmap
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_lsan_chrome_mp&range=209699:209703
Fixed: https://cluster-fuzz.appspot.com/revisions?job=linux_lsan_chrome_mp&range=399855:399884

Unminimized Testcase: https://cluster-fuzz.appspot.com/download/AMIfv96thKP2CjeemGVshKdR6Q_alCRR4SXUUtU_NCOuRn3-yKCqquI9gfsz1U2i7bmNIrh0KK-Npr3OluQQFD3_FGZzy-t6071xIMkyqSPhNsQZe5UEYutWqG-8-nrK883FHlVweBMLRLHiu54aSrIz82AGLsLsBw


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.
Project Member

Comment 9 by ClusterFuzz, Jun 16 2016

Labels: ClusterFuzz-Verified
Status: Verified (was: Assigned)
ClusterFuzz testcase is verified as fixed, closing issue.

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

Comment 10 by sheriffbot@chromium.org, Nov 22 2016

Labels: -Restrict-View-EditIssue
Removing EditIssue view restrictions from ClusterFuzz filed bugs. If you believe that this issue should still be restricted, please reapply the label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Sign in to add a comment