Clean up the IconLoader.

Clean up the IconLoader.

The text was weirdly vague, trying to be cross-platform but not succeeding, there were odd conditions that could cause leaks, and there was about twice the complexity built in than it actually ended up using.

BUG=555865
Committed: https://crrev.com/381f719fd68a0b32d41ad0605528aab75d4e9380
Cr-Commit-Position: refs/heads/master@{#438961}

[Avi (use Gerrit), 2016-10-22 21:14:24]

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

[commit-bot: I haz the power, 2016-10-22 21:14:30]

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, 2016-10-22 21:57:50]

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

[commit-bot: I haz the power, 2016-10-22 21:57:51]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-10-22 21:59:38]

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

[sky, 2016-10-24 15:56:25]

https://codereview.chromium.org/2440273002/diff/1/chrome/browser/icon_loader_...
File chrome/browser/icon_loader_auralinux.cc (right):

https://codereview.chromium.org/2440273002/diff/1/chrome/browser/icon_loader_...
chrome/browser/icon_loader_auralinux.cc:15: return
base::nix::GetFileMimeType(filepath);
I believe this may trigger io:
std::string GetFileMimeType(const FilePath& filepath) {
  ...
  ThreadRestrictions::AssertIOAllowed();

AFAICT you call this from LookupIconFromFilepath, which I believe is meant to be
called from the main thread.

[Avi (use Gerrit), 2016-10-24 16:02:18]

https://codereview.chromium.org/2440273002/diff/1/chrome/browser/icon_loader_...
File chrome/browser/icon_loader_auralinux.cc (right):

https://codereview.chromium.org/2440273002/diff/1/chrome/browser/icon_loader_...
chrome/browser/icon_loader_auralinux.cc:15: return
base::nix::GetFileMimeType(filepath);
On 2016/10/24 15:56:25, sky wrote:
> I believe this may trigger io:
> std::string GetFileMimeType(const FilePath& filepath) {
>   ...
>   ThreadRestrictions::AssertIOAllowed();
> 
> AFAICT you call this from LookupIconFromFilepath, which I believe is meant to
be
> called from the main thread.

:( Missed that platform. Let me try again.

[Avi (use Gerrit), 2016-12-15 20:59:39]

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

[commit-bot: I haz the power, 2016-12-15 21:00:29]

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

[Avi (use Gerrit), 2016-12-15 21:47:26]

Scott, please take another look.

[commit-bot: I haz the power, 2016-12-15 22:10:42]

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

[commit-bot: I haz the power, 2016-12-15 22:10:43]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sky, 2016-12-15 22:36:05]

LGTM - bonus points if you want to fix the following, but I get they are out of
scope of your change.

https://codereview.chromium.org/2440273002/diff/20001/chrome/browser/icon_loa...
File chrome/browser/icon_loader.cc (right):

https://codereview.chromium.org/2440273002/diff/20001/chrome/browser/icon_loa...
chrome/browser/icon_loader.cc:16: : target_task_runner_(nullptr),
optional: remove the initializer entirely (null is the default).

https://codereview.chromium.org/2440273002/diff/20001/chrome/browser/icon_man...
File chrome/browser/icon_manager.cc (right):

https://codereview.chromium.org/2440273002/diff/20001/chrome/browser/icon_man...
chrome/browser/icon_manager.cc:37: IconManager::~IconManager() {
In reviewing this code it seems like IconManager is racy. In particular
IconManager is not ref counted, but IconLoader is. IconLoader references
IconManager through it's a delegate. This means it's possible for IconLoader to
outlive it's delegate. Seems this function should null out the delegates (and
you should keep the null check in IconLoader::NotifyDelegate).

But, this isn't new. If you could file a bug with todo that would be great.

https://codereview.chromium.org/2440273002/diff/20001/chrome/browser/icon_man...
chrome/browser/icon_manager.cc:60: loader->AddRef();
This code is equally error prone. Seems like the map key should be a
scoped_refptr, that way no explicit addref/release. But again, this isn't new.

[Avi (use Gerrit), 2016-12-15 23:01:10]

https://codereview.chromium.org/2440273002/diff/20001/chrome/browser/icon_loa...
File chrome/browser/icon_loader.cc (right):

https://codereview.chromium.org/2440273002/diff/20001/chrome/browser/icon_loa...
chrome/browser/icon_loader.cc:16: : target_task_runner_(nullptr),
On 2016/12/15 22:36:05, sky wrote:
> optional: remove the initializer entirely (null is the default).

Done.

https://codereview.chromium.org/2440273002/diff/20001/chrome/browser/icon_man...
File chrome/browser/icon_manager.cc (right):

https://codereview.chromium.org/2440273002/diff/20001/chrome/browser/icon_man...
chrome/browser/icon_manager.cc:37: IconManager::~IconManager() {
On 2016/12/15 22:36:05, sky wrote:
> In reviewing this code it seems like IconManager is racy. In particular
> IconManager is not ref counted, but IconLoader is. IconLoader references
> IconManager through it's a delegate. This means it's possible for IconLoader
to
> outlive it's delegate. Seems this function should null out the delegates (and
> you should keep the null check in IconLoader::NotifyDelegate).
> 
> But, this isn't new. If you could file a bug with todo that would be great.

IconManager is the only user of IconLoader. Why is IconLoader ref counted at
all?

I'm going to land this, but then want to make one more pass at cleanup.

https://codereview.chromium.org/2440273002/diff/20001/chrome/browser/icon_man...
chrome/browser/icon_manager.cc:60: loader->AddRef();
On 2016/12/15 22:36:05, sky wrote:
> This code is equally error prone. Seems like the map key should be a
> scoped_refptr, that way no explicit addref/release. But again, this isn't new.

Will address in followup.

[Avi (use Gerrit), 2016-12-15 23:01:38]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2016-12-15 23:01:39]

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

[commit-bot: I haz the power, 2016-12-15 23:02:45]

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

[sky, 2016-12-15 23:15:15]

Thanks for filing a bug!

On Thu, Dec 15, 2016 at 3:01 PM,  <avi@chromium.org> wrote:
>
>
https://codereview.chromium.org/2440273002/diff/20001/chrome/browser/icon_loa...
> File chrome/browser/icon_loader.cc (right):
>
>
https://codereview.chromium.org/2440273002/diff/20001/chrome/browser/icon_loa...
> chrome/browser/icon_loader.cc:16: : target_task_runner_(nullptr),
> On 2016/12/15 22:36:05, sky wrote:
>> optional: remove the initializer entirely (null is the default).
>
> Done.
>
>
https://codereview.chromium.org/2440273002/diff/20001/chrome/browser/icon_man...
> File chrome/browser/icon_manager.cc (right):
>
>
https://codereview.chromium.org/2440273002/diff/20001/chrome/browser/icon_man...
> chrome/browser/icon_manager.cc:37: IconManager::~IconManager() {
> On 2016/12/15 22:36:05, sky wrote:
>> In reviewing this code it seems like IconManager is racy. In
> particular
>> IconManager is not ref counted, but IconLoader is. IconLoader
> references
>> IconManager through it's a delegate. This means it's possible for
> IconLoader to
>> outlive it's delegate. Seems this function should null out the
> delegates (and
>> you should keep the null check in IconLoader::NotifyDelegate).
>>
>> But, this isn't new. If you could file a bug with todo that would be
> great.
>
> IconManager is the only user of IconLoader. Why is IconLoader ref
> counted at all?
>
> I'm going to land this, but then want to make one more pass at cleanup.
>
>
https://codereview.chromium.org/2440273002/diff/20001/chrome/browser/icon_man...
> chrome/browser/icon_manager.cc:60: loader->AddRef();
> On 2016/12/15 22:36:05, sky wrote:
>> This code is equally error prone. Seems like the map key should be a
>> scoped_refptr, that way no explicit addref/release. But again, this
> isn't new.
>
> Will address in followup.
>
> https://codereview.chromium.org/2440273002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Avi (use Gerrit), 2016-12-15 23:37:31]

On 2016/12/15 23:15:15, sky wrote:
> Thanks for filing a bug!
> 
> On Thu, Dec 15, 2016 at 3:01 PM,  <mailto:avi@chromium.org> wrote:
> >
> >
>
https://codereview.chromium.org/2440273002/diff/20001/chrome/browser/icon_loa...
> > File chrome/browser/icon_loader.cc (right):
> >
> >
>
https://codereview.chromium.org/2440273002/diff/20001/chrome/browser/icon_loa...
> > chrome/browser/icon_loader.cc:16: : target_task_runner_(nullptr),
> > On 2016/12/15 22:36:05, sky wrote:
> >> optional: remove the initializer entirely (null is the default).
> >
> > Done.
> >
> >
>
https://codereview.chromium.org/2440273002/diff/20001/chrome/browser/icon_man...
> > File chrome/browser/icon_manager.cc (right):
> >
> >
>
https://codereview.chromium.org/2440273002/diff/20001/chrome/browser/icon_man...
> > chrome/browser/icon_manager.cc:37: IconManager::~IconManager() {
> > On 2016/12/15 22:36:05, sky wrote:
> >> In reviewing this code it seems like IconManager is racy. In
> > particular
> >> IconManager is not ref counted, but IconLoader is. IconLoader
> > references
> >> IconManager through it's a delegate. This means it's possible for
> > IconLoader to
> >> outlive it's delegate. Seems this function should null out the
> > delegates (and
> >> you should keep the null check in IconLoader::NotifyDelegate).
> >>
> >> But, this isn't new. If you could file a bug with todo that would be
> > great.
> >
> > IconManager is the only user of IconLoader. Why is IconLoader ref
> > counted at all?
> >
> > I'm going to land this, but then want to make one more pass at cleanup.
> >
> >
>
https://codereview.chromium.org/2440273002/diff/20001/chrome/browser/icon_man...
> > chrome/browser/icon_manager.cc:60: loader->AddRef();
> > On 2016/12/15 22:36:05, sky wrote:
> >> This code is equally error prone. Seems like the map key should be a
> >> scoped_refptr, that way no explicit addref/release. But again, this
> > isn't new.
> >
> > Will address in followup.
> >
> > https://codereview.chromium.org/2440273002/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
> 

Gotta have a BUG line in my followup.

[commit-bot: I haz the power, 2016-12-16 00:05:06]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481842898791440,
"parent_rev": "181f1f42d27f121c390e7a2b0543c3e095d07bd2", "commit_rev":
"2948656d60f5c6bbf7d50cfb7a799a22a1308915"}

[commit-bot: I haz the power, 2016-12-16 00:05:40]

Description was changed from

==========
Clean up the IconLoader.

The text was weirdly vague, trying to be cross-platform but not succeeding,
there were odd conditions that could cause leaks, and there was about twice the
complexity built in than it actually ended up using.

BUG=555865
==========

to

==========
Clean up the IconLoader.

The text was weirdly vague, trying to be cross-platform but not succeeding,
there were odd conditions that could cause leaks, and there was about twice the
complexity built in than it actually ended up using.

BUG=555865

Review-Url: https://codereview.chromium.org/2440273002
==========

[commit-bot: I haz the power, 2016-12-16 00:05:43]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-16 00:07:39]

Description was changed from

==========
Clean up the IconLoader.

The text was weirdly vague, trying to be cross-platform but not succeeding,
there were odd conditions that could cause leaks, and there was about twice the
complexity built in than it actually ended up using.

BUG=555865

Review-Url: https://codereview.chromium.org/2440273002
==========

to

==========
Clean up the IconLoader.

The text was weirdly vague, trying to be cross-platform but not succeeding,
there were odd conditions that could cause leaks, and there was about twice the
complexity built in than it actually ended up using.

BUG=555865

Committed: https://crrev.com/381f719fd68a0b32d41ad0605528aab75d4e9380
Cr-Commit-Position: refs/heads/master@{#438961}
==========

[commit-bot: I haz the power, 2016-12-16 00:07:40]

Patchset 3 (id:??) landed as
https://crrev.com/381f719fd68a0b32d41ad0605528aab75d4e9380
Cr-Commit-Position: refs/heads/master@{#438961}