Fix crashes due to gfx::Image unsafe thread passing

Fix crashes due to gfx::Image unsafe thread passing

gfx::Image has |storage_| member which is base::RefCounted, not thread
safe. Thus passing around gfx::Image to other threads by copying is
incorrect.

Workaround by using scoped_ptr and making ImageRecord class
RefCountedThreadSafe.

Related discussion:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/8LqVoXQ_2bo

BUG=471800
Committed: https://crrev.com/16a0d2a22d17e3932432dab4559e63c5134aaccc
Cr-Commit-Position: refs/heads/master@{#323720}

[Kibeom Kim (inactive), 2015-03-30 17:54:17]

The CQ bit was checked by kkimlabs@chromium.org

[commit-bot: I haz the power, 2015-03-30 17:54:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1031293002/40001

[commit-bot: I haz the power, 2015-03-30 17:54:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-30 17:54:32]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Kibeom Kim (inactive), 2015-03-30 18:01:17]

kkimlabs@chromium.org changed reviewers:
+ danduong@chromium.org, noyau@chromium.org

[Kibeom Kim (inactive), 2015-03-30 18:01:18]

Worked around by making ImageRecord base::RefCountedTheadSafe.

[Kibeom Kim (inactive), 2015-03-31 21:02:06]

ptal ready for review.

[noyau (Ping after 24h), 2015-04-01 11:07:13]

https://codereview.chromium.org/1031293002/diff/200001/chrome/browser/enhance...
File chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc
(right):

https://codereview.chromium.org/1031293002/diff/200001/chrome/browser/enhance...
chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc:216:
scoped_ptr<gfx::Image> *image) {
I'm not a fan of this API. It'd prefer the argument to be a const gfx::Image and
returning a scoped_ptr<>. It would make it explicit that this is creating a new
image, and I'm pretty sure it is not the responsibility of this function to
delete the old one.

[Kibeom Kim (inactive), 2015-04-01 19:09:06]

https://codereview.chromium.org/1031293002/diff/200001/chrome/browser/enhance...
File chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc
(right):

https://codereview.chromium.org/1031293002/diff/200001/chrome/browser/enhance...
chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc:216:
scoped_ptr<gfx::Image> *image) {
On 2015/04/01 11:07:12, noyau wrote:
> I'm not a fan of this API. It'd prefer the argument to be a const gfx::Image
and
> returning a scoped_ptr<>. It would make it explicit that this is creating a
new
> image, and I'm pretty sure it is not the responsibility of this function to
> delete the old one.

agree. Done.

[noyau (Ping after 24h), 2015-04-02 07:48:20]

lgtm with nits.

https://codereview.chromium.org/1031293002/diff/220001/chrome/browser/enhance...
File chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc
(right):

https://codereview.chromium.org/1031293002/diff/220001/chrome/browser/enhance...
chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc:219:
scoped_ptr<gfx::Image> result(new gfx::Image(image));
I know it doesn't cost that much, but still, an image is copied here an
potentially discarded if a resize happens.

https://codereview.chromium.org/1031293002/diff/220001/chrome/browser/enhance...
chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc:232:
*result = gfx::Image::CreateFrom1xBitmap(
Use result.reset(...) instead? It seems to be the most common pattern.

[Kibeom Kim (inactive), 2015-04-02 08:48:18]

https://codereview.chromium.org/1031293002/diff/220001/chrome/browser/enhance...
File chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc
(right):

https://codereview.chromium.org/1031293002/diff/220001/chrome/browser/enhance...
chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc:219:
scoped_ptr<gfx::Image> result(new gfx::Image(image));
On 2015/04/02 07:48:20, noyau wrote:
> I know it doesn't cost that much, but still, an image is copied here an
> potentially discarded if a resize happens.

Done.

https://codereview.chromium.org/1031293002/diff/220001/chrome/browser/enhance...
chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc:232:
*result = gfx::Image::CreateFrom1xBitmap(
On 2015/04/02 07:48:20, noyau wrote:
> Use result.reset(...) instead? It seems to be the most common pattern.

but reset(...) takes pointer type so I think this is the only option :/....

[Kibeom Kim (inactive), 2015-04-02 08:48:23]

The CQ bit was checked by kkimlabs@chromium.org

[Kibeom Kim (inactive), 2015-04-02 08:48:23]

The patchset sent to the CQ was uploaded after l-g-t-m from noyau@chromium.org
Link to the patchset: https://codereview.chromium.org/1031293002/#ps240001
(title: "nits")

[commit-bot: I haz the power, 2015-04-02 08:48:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1031293002/240001

[noyau (Ping after 24h), 2015-04-02 09:39:31]

https://codereview.chromium.org/1031293002/diff/240001/chrome/browser/enhance...
File chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc
(right):

https://codereview.chromium.org/1031293002/diff/240001/chrome/browser/enhance...
chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc:219:
scoped_ptr<gfx::Image> result(new gfx::Image());
scoped_ptr<gfx::Image> result;

Why the new image? A scoped_pr might be empty…

[Kibeom Kim (inactive), 2015-04-02 09:43:07]

The CQ bit was unchecked by kkimlabs@chromium.org

[Kibeom Kim (inactive), 2015-04-02 09:45:50]

https://codereview.chromium.org/1031293002/diff/240001/chrome/browser/enhance...
File chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc
(right):

https://codereview.chromium.org/1031293002/diff/240001/chrome/browser/enhance...
chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc:219:
scoped_ptr<gfx::Image> result(new gfx::Image());
On 2015/04/02 09:39:30, noyau wrote:
> scoped_ptr<gfx::Image> result;
> 
> Why the new image? A scoped_pr might be empty…

But isn't it that, when we copy to |result| later at #232 and #239, we need an
allocated space?

[Kibeom Kim (inactive), 2015-04-02 09:59:22]

https://codereview.chromium.org/1031293002/diff/240001/chrome/browser/enhance...
File chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc
(right):

https://codereview.chromium.org/1031293002/diff/240001/chrome/browser/enhance...
chrome/browser/enhanced_bookmarks/android/bookmark_image_service_android.cc:219:
scoped_ptr<gfx::Image> result(new gfx::Image());
On 2015/04/02 09:45:50, Kibeom Kim wrote:
> On 2015/04/02 09:39:30, noyau wrote:
> > scoped_ptr<gfx::Image> result;
> > 
> > Why the new image? A scoped_pr might be empty…
> 
> But isn't it that, when we copy to |result| later at #232 and #239, we need an
> allocated space?

Hmm.. maybe another option is using placement new here.

[lpromero, 2015-04-02 11:48:10]

lpromero@chromium.org changed reviewers:
+ lpromero@chromium.org

[lpromero, 2015-04-02 11:48:11]

Rather unrelated comments to the beef of your discussion.

https://codereview.chromium.org/1031293002/diff/240001/components/enhanced_bo...
File components/enhanced_bookmarks/bookmark_image_service.cc (right):

https://codereview.chromium.org/1031293002/diff/240001/components/enhanced_bo...
components/enhanced_bookmarks/bookmark_image_service.cc:233: return image_info;
Why not Pass? See comment later about this.

https://codereview.chromium.org/1031293002/diff/240001/components/enhanced_bo...
File components/enhanced_bookmarks/image_store_unittest.cc (right):

https://codereview.chromium.org/1031293002/diff/240001/components/enhanced_bo...
components/enhanced_bookmarks/image_store_unittest.cc:47: return
GenerateBlackImage(42, 24).Pass();
When exactly do you need .Pass()?
In this file, every time you return a scoped_refptr, you use Pass(), but not in
components/enhanced_bookmarks/persistent_image_store.cc.

https://codereview.chromium.org/1031293002/diff/240001/components/enhanced_bo...
File components/enhanced_bookmarks/persistent_image_store.cc (right):

https://codereview.chromium.org/1031293002/diff/240001/components/enhanced_bo...
components/enhanced_bookmarks/persistent_image_store.cc:215: return
image_record;
.Pass() ?
cf earlier comment about Pass().

https://codereview.chromium.org/1031293002/diff/240001/components/enhanced_bo...
components/enhanced_bookmarks/persistent_image_store.cc:229: return
image_record;
Ditto.

https://codereview.chromium.org/1031293002/diff/240001/components/enhanced_bo...
components/enhanced_bookmarks/persistent_image_store.cc:262: return
image_record;
Ditto.

[noyau (Ping after 24h), 2015-04-02 12:46:35]

https://codereview.chromium.org/1031293002/diff/240001/components/enhanced_bo...
File components/enhanced_bookmarks/bookmark_image_service.cc (right):

https://codereview.chromium.org/1031293002/diff/240001/components/enhanced_bo...
components/enhanced_bookmarks/bookmark_image_service.cc:194:
scoped_ptr<gfx::Image> image) {
What is going to happen if an image is created, and then a copy is passed to
this method? There is still an issue. The copy is passed with a scoped object
but it still references a storage referenced on the original thread.

To be passed safely to another thread the reference count on the storage for the
passed image must be 1, if more, it's a potential crasher. Maybe we should add a
way to DCHECK this?

A method on gfx::Image called MakeThreadSafe() that would make a copy of the
storage if the refcount is >1? Of just a IsThreadSafe() checking that the
refcount is exactly one?

[Kibeom Kim (inactive), 2015-04-02 18:37:14]

The CQ bit was checked by kkimlabs@chromium.org

[Kibeom Kim (inactive), 2015-04-02 18:37:14]

The patchset sent to the CQ was uploaded after l-g-t-m from noyau@chromium.org
Link to the patchset: https://codereview.chromium.org/1031293002/#ps260001
(title: "Remove unnecessary Pass()")

[commit-bot: I haz the power, 2015-04-02 18:39:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1031293002/260001

[Kibeom Kim (inactive), 2015-04-02 18:41:09]

https://codereview.chromium.org/1031293002/diff/240001/components/enhanced_bo...
File components/enhanced_bookmarks/bookmark_image_service.cc (right):

https://codereview.chromium.org/1031293002/diff/240001/components/enhanced_bo...
components/enhanced_bookmarks/bookmark_image_service.cc:194:
scoped_ptr<gfx::Image> image) {
On 2015/04/02 12:46:34, noyau wrote:
> What is going to happen if an image is created, and then a copy is passed to
> this method? There is still an issue. The copy is passed with a scoped object
> but it still references a storage referenced on the original thread.
> 

Right... and unfortunately I don't think there is a way to DCHECK that in this
context...


> To be passed safely to another thread the reference count on the storage for
the
> passed image must be 1, if more, it's a potential crasher. Maybe we should add
a
> way to DCHECK this?
> 
> A method on gfx::Image called MakeThreadSafe() that would make a copy of the
> storage if the refcount is >1? Of just a IsThreadSafe() checking that the
> refcount is exactly one?

MakeThreadSafe() is gfx::ImageSkia's method, and it just make it read-only. Note
that ImageSkia::storage_ is RefCountedThreadSafe :/


I'm planning to continue the ref_counted discussion on chromium-dev. (but I
think we should still land this CL independently)

https://codereview.chromium.org/1031293002/diff/240001/components/enhanced_bo...
File components/enhanced_bookmarks/image_store_unittest.cc (right):

https://codereview.chromium.org/1031293002/diff/240001/components/enhanced_bo...
components/enhanced_bookmarks/image_store_unittest.cc:47: return
GenerateBlackImage(42, 24).Pass();
On 2015/04/02 11:48:11, lpromero wrote:
> When exactly do you need .Pass()?
> In this file, every time you return a scoped_refptr, you use Pass(), but not
in
> components/enhanced_bookmarks/persistent_image_store.cc.

Actually Pass() is just typecasting, { return static_cast<type&&>(*this); }. In
general, instances will be moved or copied, but copying scoped_ptr is compile
time error. So depending in context, Pass() needs to be specified to enforce
moving, to avoid copy compile time error, but if it's moving already without it,
I believe putting .Pass() doesn't affect anything.

Anyways, removed all the unnecessary .Pass(). Done.

[Kibeom Kim (inactive), 2015-04-02 18:41:14]

The CQ bit was unchecked by kkimlabs@chromium.org

[Kibeom Kim (inactive), 2015-04-02 18:43:29]

ptal

[Kibeom Kim (inactive), 2015-04-02 20:50:45]

The CQ bit was checked by kkimlabs@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-04-02 20:54:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1031293002/260001

[commit-bot: I haz the power, 2015-04-03 04:52:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-03 04:52:39]

This issue passed the CQ dry run.

[noyau (Ping after 24h), 2015-04-03 10:06:34]

On 2015/04/03 04:52:39, I haz the power (commit-bot) wrote:
> This issue passed the CQ dry run.

still lgtm.

[Kibeom Kim (inactive), 2015-04-03 10:06:48]

The CQ bit was checked by kkimlabs@chromium.org

[commit-bot: I haz the power, 2015-04-03 10:07:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1031293002/260001

[commit-bot: I haz the power, 2015-04-03 10:08:20]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-04-03 20:36:49]

Patchset 14 (id:??) landed as
https://crrev.com/16a0d2a22d17e3932432dab4559e63c5134aaccc
Cr-Commit-Position: refs/heads/master@{#323720}