Switch IconLoader::ReadIconThreadID from FILE to UI

Move IconLoader::ReadIconThreadID from FILE to UI

gfx::Image is not thread safe due to non-thread-safe ref count of its
gfx::ImageStorage. Especially, Image instances in ResourceBundle are
cached and shared globally. So, touching it from non-UI thread causes
a data race on its ref count.

BUG=688072, 468010
Review-Url: https://codereview.chromium.org/2707753004
Cr-Commit-Position: refs/heads/master@{#452250}
Committed: https://chromium.googlesource.com/chromium/src/+/65452e0405106b58dd554d134679ace6acc26d00

[tzik, 2017-02-20 11:38:46]

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

[tzik, 2017-02-20 11:40:44]

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

[commit-bot: I haz the power, 2017-02-20 11:41:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2017-02-20 11:44:59]

Description was changed from

==========
Move IconLoader::ReadIconThread from FILE thread to UI thread
==========

to

==========
Move IconLoader::ReadIconThreadID from FILE to UI

gfx::Image is not thread safe due to non-thread-safe ref count of its
gfx::ImageStorage. Especially, Image instances in ResourceBundle are
cached and shared globally. So, touching it from non-UI thread causes
a data race on its ref count.
==========

[tzik, 2017-02-20 11:44:59]

tzik@chromium.org changed reviewers:
+ oshima@chromium.org

[tzik, 2017-02-20 11:45:26]

Description was changed from

==========
Move IconLoader::ReadIconThreadID from FILE to UI

gfx::Image is not thread safe due to non-thread-safe ref count of its
gfx::ImageStorage. Especially, Image instances in ResourceBundle are
cached and shared globally. So, touching it from non-UI thread causes
a data race on its ref count.
==========

to

==========
Move IconLoader::ReadIconThreadID from FILE to UI

gfx::Image is not thread safe due to non-thread-safe ref count of its
gfx::ImageStorage. Especially, Image instances in ResourceBundle are
cached and shared globally. So, touching it from non-UI thread causes
a data race on its ref count.

BUG=688072
==========

[tzik, 2017-02-20 11:45:40]

PTAL

[commit-bot: I haz the power, 2017-02-20 12:08:52]

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

[commit-bot: I haz the power, 2017-02-20 12:08:52]

Dry run: This issue passed the CQ dry run.

[tzik, 2017-02-20 20:33:34]

Description was changed from

==========
Move IconLoader::ReadIconThreadID from FILE to UI

gfx::Image is not thread safe due to non-thread-safe ref count of its
gfx::ImageStorage. Especially, Image instances in ResourceBundle are
cached and shared globally. So, touching it from non-UI thread causes
a data race on its ref count.

BUG=688072
==========

to

==========
Move IconLoader::ReadIconThreadID from FILE to UI

gfx::Image is not thread safe due to non-thread-safe ref count of its
gfx::ImageStorage. Especially, Image instances in ResourceBundle are
cached and shared globally. So, touching it from non-UI thread causes
a data race on its ref count.

BUG=688072, 468010
==========

[oshima, 2017-02-21 00:32:45]

While I understand that gfx::Image itself isn't safe (which makes the locking in
RB meaningless, sadly), all these resources for file icons are accessed only in
FILE thread, so it must be safe.

[tzik, 2017-02-21 01:00:37]

On 2017/02/21 00:32:45, oshima wrote:
> While I understand that gfx::Image itself isn't safe (which makes the locking
in
> RB meaningless, sadly), all these resources for file icons are accessed only
in
> FILE thread, so it must be safe.

I think it's still unsafe since RB stores a copy of the image in it as a cache,
and clears it on UI thread.

Do you mean there is any other call site that touches the same resource in RB,
or the resulting icons are accessed only on FILE thread?
If the former, we should fix it too.
Otherwise, unlike the icon image in RB, IconLoader::image_ here is detached from
the image in RB and looks safe to use on FILE thread.

[oshima, 2017-02-21 01:50:25]

On 2017/02/21 01:00:37, tzik wrote:
> On 2017/02/21 00:32:45, oshima wrote:
> > While I understand that gfx::Image itself isn't safe (which makes the
locking
> in
> > RB meaningless, sadly), all these resources for file icons are accessed only
> in
> > FILE thread, so it must be safe.
> 
> I think it's still unsafe since RB stores a copy of the image in it as a
cache,
> and clears it on UI thread.

IIRC, the resource bundle is deleted very last moment, after FILE thread is
terminated.

> 
> Do you mean there is any other call site that touches the same resource in RB,

There is no other places that use these resources, and the reference to these
resources
are detached in the same call in FILE thread.

> or the resulting icons are accessed only on FILE thread?
> If the former, we should fix it too.
> Otherwise, unlike the icon image in RB, IconLoader::image_ here is detached
from
> the image in RB and looks safe to use on FILE thread.

Do we need image_? looks like it's passed to another thread, so it must be empty
after this.

[tzik, 2017-02-21 04:17:35]

On 2017/02/21 01:50:25, oshima wrote:
> On 2017/02/21 01:00:37, tzik wrote:
> > On 2017/02/21 00:32:45, oshima wrote:
> > > While I understand that gfx::Image itself isn't safe (which makes the
> locking
> > in
> > > RB meaningless, sadly), all these resources for file icons are accessed
only
> > in
> > > FILE thread, so it must be safe.
> > 
> > I think it's still unsafe since RB stores a copy of the image in it as a
> cache,
> > and clears it on UI thread.
> 
> IIRC, the resource bundle is deleted very last moment, after FILE thread is
> terminated.

Ah, OK. So, it's not racy.

> 
> > 
> > Do you mean there is any other call site that touches the same resource in
RB,
> 
> There is no other places that use these resources, and the reference to these
> resources
> are detached in the same call in FILE thread.

That means it's also safe to use UI thread here?

> 
> > or the resulting icons are accessed only on FILE thread?
> > If the former, we should fix it too.
> > Otherwise, unlike the icon image in RB, IconLoader::image_ here is detached
> from
> > the image in RB and looks safe to use on FILE thread.
> 
> Do we need image_? looks like it's passed to another thread, so it must be
empty
> after this.

I meant the instance stored in image_.


Though IconLoader is not race, can we use UI thread if there's no strong
requirement to use FILE thread?
Allowing the access to the RB images from FILE thread implies that each items in
RB are implicitly bound to different threads. I think it's error prone and hard
to audit.
Since IconLoader seems the last non-UI thread user of RB images, after this CL,
we can ban the access from non-UI thread by DCHECK and prevent further
thread-unsafe usage of RB images.

[tzik, 2017-02-21 04:37:32]

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

[commit-bot: I haz the power, 2017-02-21 04:37:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-21 06:04:11]

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

[commit-bot: I haz the power, 2017-02-21 06:04:12]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[oshima, 2017-02-21 07:07:53]

On 2017/02/21 04:17:35, tzik wrote:
> On 2017/02/21 01:50:25, oshima wrote:
> > On 2017/02/21 01:00:37, tzik wrote:
> > > On 2017/02/21 00:32:45, oshima wrote:
> > > > While I understand that gfx::Image itself isn't safe (which makes the
> > locking
> > > in
> > > > RB meaningless, sadly), all these resources for file icons are accessed
> only
> > > in
> > > > FILE thread, so it must be safe.
> > > 
> > > I think it's still unsafe since RB stores a copy of the image in it as a
> > cache,
> > > and clears it on UI thread.
> > 
> > IIRC, the resource bundle is deleted very last moment, after FILE thread is
> > terminated.
> 
> Ah, OK. So, it's not racy.
> 
> > 
> > > 
> > > Do you mean there is any other call site that touches the same resource in
> RB,
> > 
> > There is no other places that use these resources, and the reference to
these
> > resources
> > are detached in the same call in FILE thread.
> 
> That means it's also safe to use UI thread here?

Yes.

> 
> > 
> > > or the resulting icons are accessed only on FILE thread?
> > > If the former, we should fix it too.
> > > Otherwise, unlike the icon image in RB, IconLoader::image_ here is
detached
> > from
> > > the image in RB and looks safe to use on FILE thread.
> > 
> > Do we need image_? looks like it's passed to another thread, so it must be
> empty
> > after this.
> 
> I meant the instance stored in image_.

From what I see, there is no need fr image_ because image_ is passed (thus, it's
moved)
to another thread. I'm not sure exactly why, but probably it was left this way
due to historical reason.

> 
> 
> Though IconLoader is not race, can we use UI thread if there's no strong
> requirement to use FILE thread?
> Allowing the access to the RB images from FILE thread implies that each items
in
> RB are implicitly bound to different threads. I think it's error prone and
hard
> to audit.
> Since IconLoader seems the last non-UI thread user of RB images, after this
CL,
> we can ban the access from non-UI thread by DCHECK and prevent further
> thread-unsafe usage of RB images.

If this is the last one, and that's what we're going to do, then i'm fine.
Are we going to remove the lock in RB?

and can you remove image_?

[oshima, 2017-02-21 07:12:21]

Actually I wonder if we need to post the task to read. Can't we just read
synchronously on UI thread?

[tzik, 2017-02-21 09:36:33]

On 2017/02/21 07:07:53, oshima wrote:
> On 2017/02/21 04:17:35, tzik wrote:
> > On 2017/02/21 01:50:25, oshima wrote:
> > > On 2017/02/21 01:00:37, tzik wrote:
> > > > On 2017/02/21 00:32:45, oshima wrote:
> > > > > While I understand that gfx::Image itself isn't safe (which makes the
> > > locking
> > > > in
> > > > > RB meaningless, sadly), all these resources for file icons are
accessed
> > only
> > > > in
> > > > > FILE thread, so it must be safe.
> > > > 
> > > > I think it's still unsafe since RB stores a copy of the image in it as a
> > > cache,
> > > > and clears it on UI thread.
> > > 
> > > IIRC, the resource bundle is deleted very last moment, after FILE thread
is
> > > terminated.
> > 
> > Ah, OK. So, it's not racy.
> > 
> > > 
> > > > 
> > > > Do you mean there is any other call site that touches the same resource
in
> > RB,
> > > 
> > > There is no other places that use these resources, and the reference to
> these
> > > resources
> > > are detached in the same call in FILE thread.
> > 
> > That means it's also safe to use UI thread here?
> 
> Yes.
> 
> > 
> > > 
> > > > or the resulting icons are accessed only on FILE thread?
> > > > If the former, we should fix it too.
> > > > Otherwise, unlike the icon image in RB, IconLoader::image_ here is
> detached
> > > from
> > > > the image in RB and looks safe to use on FILE thread.
> > > 
> > > Do we need image_? looks like it's passed to another thread, so it must be
> > empty
> > > after this.
> > 
> > I meant the instance stored in image_.
> 
> From what I see, there is no need fr image_ because image_ is passed (thus,
it's
> moved)
> to another thread. I'm not sure exactly why, but probably it was left this way
> due to historical reason.
> 
> > 
> > 
> > Though IconLoader is not race, can we use UI thread if there's no strong
> > requirement to use FILE thread?
> > Allowing the access to the RB images from FILE thread implies that each
items
> in
> > RB are implicitly bound to different threads. I think it's error prone and
> hard
> > to audit.
> > Since IconLoader seems the last non-UI thread user of RB images, after this
> CL,
> > we can ban the access from non-UI thread by DCHECK and prevent further
> > thread-unsafe usage of RB images.
> 
> If this is the last one, and that's what we're going to do, then i'm fine.

(This is one of the last two, actually. The other is
http://crrev.com/2701183002/)

> Are we going to remove the lock in RB?

Yes, I'm proposing that as http://crrev.com/2699323002/

> 
> and can you remove image_?

Ok, made a CL for that: http://crrev.com/2709683002/

[oshima, 2017-02-22 04:40:24]

looks like windows is still using FILE thread, so lgtm

[tzik, 2017-02-22 06:32:24]

The CQ bit was checked by tzik@chromium.org

[tzik, 2017-02-22 06:32:24]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/2707753004/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2017-02-22 06:34:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 06:50:45]

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

[commit-bot: I haz the power, 2017-02-22 06:50:46]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[tzik, 2017-02-22 06:56:06]

tzik@chromium.org changed reviewers:
+ sky@chromium.org

[tzik, 2017-02-22 06:56:06]

+sky. PTAL as a //chrome owner.

[sky, 2017-02-22 17:12:02]

LGTM

https://codereview.chromium.org/2707753004/diff/40001/chrome/browser/icon_loa...
File chrome/browser/icon_loader_chromeos.cc (right):

https://codereview.chromium.org/2707753004/diff/40001/chrome/browser/icon_loa...
chrome/browser/icon_loader_chromeos.cc:195: return content::BrowserThread::UI;
Please add a comment. It isn't readily obvious why it's ok to do the processing
on the ui thread.

[tzik, 2017-02-22 22:02:56]

https://codereview.chromium.org/2707753004/diff/40001/chrome/browser/icon_loa...
File chrome/browser/icon_loader_chromeos.cc (right):

https://codereview.chromium.org/2707753004/diff/40001/chrome/browser/icon_loa...
chrome/browser/icon_loader_chromeos.cc:195: return content::BrowserThread::UI;
On 2017/02/22 17:12:02, sky wrote:
> Please add a comment. It isn't readily obvious why it's ok to do the
processing
> on the ui thread.

Done.

[tzik, 2017-02-22 22:03:02]

The CQ bit was checked by tzik@chromium.org

[tzik, 2017-02-22 22:03:02]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2707753004/#ps60001
(title: "+comment")

[commit-bot: I haz the power, 2017-02-22 22:03:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 22:53:24]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487800982602400,
"parent_rev": "4950173d4782941bd49f90be89c1d77422e8ca73", "commit_rev":
"65452e0405106b58dd554d134679ace6acc26d00"}

[commit-bot: I haz the power, 2017-02-22 22:54:03]

Description was changed from

==========
Move IconLoader::ReadIconThreadID from FILE to UI

gfx::Image is not thread safe due to non-thread-safe ref count of its
gfx::ImageStorage. Especially, Image instances in ResourceBundle are
cached and shared globally. So, touching it from non-UI thread causes
a data race on its ref count.

BUG=688072, 468010
==========

to

==========
Move IconLoader::ReadIconThreadID from FILE to UI

gfx::Image is not thread safe due to non-thread-safe ref count of its
gfx::ImageStorage. Especially, Image instances in ResourceBundle are
cached and shared globally. So, touching it from non-UI thread causes
a data race on its ref count.

BUG=688072, 468010

Review-Url: https://codereview.chromium.org/2707753004
Cr-Commit-Position: refs/heads/master@{#452250}
Committed:
https://chromium.googlesource.com/chromium/src/+/65452e0405106b58dd554d134679...
==========

[commit-bot: I haz the power, 2017-02-22 22:54:04]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/65452e0405106b58dd554d134679...