Implement an ObjC wrapper for fetching account chooser avatar images.

Implement an ObjC wrapper for fetching account chooser avatar images.

Depends on https://codereview.chromium.org/878743007/

BUG=448011
Committed: https://crrev.com/544576f1a93a472f178f9e7da558b7dd685b7781
Cr-Commit-Position: refs/heads/master@{#319249}

[dconnelly, 2015-02-06 16:15:24]

dconnelly@chromium.org changed reviewers:
+ groby@chromium.org

[groby-ooo-7-16, 2015-02-06 16:31:09]

Why do we need something that's different from ImageFetcher?

[dconnelly, 2015-02-09 14:50:57]

On 2015/02/06 16:31:09, groby wrote:
> Why do we need something that's different from ImageFetcher?

This uses ImageFetcher transitively through AvatarFetcher.
AccountAvatarFetcherManager just extracts logic around fetching avatars for
specific views that would otherwise be dumped into a view controller.

[groby-ooo-7-16, 2015-02-12 01:07:57]

LGTM w/nit

https://codereview.chromium.org/864963004/diff/1/chrome/browser/ui/cocoa/pass...
File chrome/browser/ui/cocoa/passwords/account_avatar_fetcher_manager.mm
(right):

https://codereview.chromium.org/864963004/diff/1/chrome/browser/ui/cocoa/pass...
chrome/browser/ui/cocoa/passwords/account_avatar_fetcher_manager.mm:80: auto it
= std::find(bridges_.begin(), bridges_.end(), bridge);
You could simplify the three lines to
bridges_.erase(std::remove(bridges_.begin(), bridges_.end(), bridge),
bridges_.end());

Your call if you think that's simpler or more complex.

[dconnelly, 2015-02-17 10:17:17]

New patchsets have been uploaded after l-g-t-m from groby@chromium.org

[dconnelly, 2015-02-17 10:17:47]

https://codereview.chromium.org/864963004/diff/1/chrome/browser/ui/cocoa/pass...
File chrome/browser/ui/cocoa/passwords/account_avatar_fetcher_manager.mm
(right):

https://codereview.chromium.org/864963004/diff/1/chrome/browser/ui/cocoa/pass...
chrome/browser/ui/cocoa/passwords/account_avatar_fetcher_manager.mm:80: auto it
= std::find(bridges_.begin(), bridges_.end(), bridge);
On 2015/02/12 01:07:57, groby wrote:
> You could simplify the three lines to
> bridges_.erase(std::remove(bridges_.begin(), bridges_.end(), bridge),
> bridges_.end());
> 
> Your call if you think that's simpler or more complex.

Done.

[dconnelly, 2015-02-17 10:17:51]

The CQ bit was checked by dconnelly@chromium.org

[commit-bot: I haz the power, 2015-02-17 10:18:46]

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

[commit-bot: I haz the power, 2015-02-17 10:23:31]

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

[commit-bot: I haz the power, 2015-02-17 10:23:31]

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

[dconnelly, 2015-02-19 10:32:43]

Patchset #3 (id:40001) has been deleted

[dconnelly, 2015-02-19 10:37:12]

https://codereview.chromium.org/864963004/diff/60001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/passwords/account_avatar_fetcher_manager.mm
(right):

https://codereview.chromium.org/864963004/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/account_avatar_fetcher_manager.mm:80: auto it
= std::find(bridges_.begin(), bridges_.end(), bridge);
It turns out the std::erase(std::remove(bridges_.begin(), bridges_.end(),
bridge), bridges_.end()) can sometimes stomp memory when applied to a
ScopedVector.

std::remove doesn't guarantee that the elements from the new end iterator to the
old end iterator are actually removed elements -- in fact, the elements in that
range are undefined.

http://www.cplusplus.com/reference/algorithm/remove/

So in some cases, after calling std::remove, all the elements in the range
[bridges_.begin(), bridges_.end()) were the same pointer, and since this is a
ScopedVector, calling bridges_.erase() on any of them led to a double-free.

Given this, it doesn't seem like std::remove is safe to use on ScopedVectors.
I've changed the code back.

[groby-ooo-7-16, 2015-02-19 17:07:20]

Still LGTM

https://codereview.chromium.org/864963004/diff/60001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/passwords/account_avatar_fetcher_manager.mm
(right):

https://codereview.chromium.org/864963004/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/passwords/account_avatar_fetcher_manager.mm:80: auto it
= std::find(bridges_.begin(), bridges_.end(), bridge);
On 2015/02/19 10:37:12, dconnelly wrote:
> It turns out the std::erase(std::remove(bridges_.begin(), bridges_.end(),
> bridge), bridges_.end()) can sometimes stomp memory when applied to a
> ScopedVector.
> 
> std::remove doesn't guarantee that the elements from the new end iterator to
the
> old end iterator are actually removed elements -- in fact, the elements in
that
> range are undefined.
> 
> http://www.cplusplus.com/reference/algorithm/remove/
> 
> So in some cases, after calling std::remove, all the elements in the range
> [bridges_.begin(), bridges_.end()) were the same pointer, and since this is a
> ScopedVector, calling bridges_.erase() on any of them led to a double-free.
> 
> Given this, it doesn't seem like std::remove is safe to use on ScopedVectors.
> I've changed the code back.

Sigh. Bitten by both scopedness and STL annoyingness.

My apologies for misleading you.

[chromium-reviews, 2015-02-20 09:27:31]

No problem, I didn't know either.

On Thu Feb 19 2015 at 6:07:21 PM <groby@chromium.org> wrote:

> Still LGTM
>
>
> https://codereview.chromium.org/864963004/diff/60001/
> chrome/browser/ui/cocoa/passwords/account_avatar_fetcher_manager.mm
> File chrome/browser/ui/cocoa/passwords/account_avatar_fetcher_manager.mm
> (right):
>
> https://codereview.chromium.org/864963004/diff/60001/
> chrome/browser/ui/cocoa/passwords/account_avatar_
> fetcher_manager.mm#newcode80
> chrome/browser/ui/cocoa/passwords/account_avatar_fetcher_manager.mm:80
>
<https://codereview.chromium.org/864963004/diff/60001/chrome/browser/ui/cocoa/...>
> :
> auto it = std::find(bridges_.begin(), bridges_.end(), bridge);
> On 2015/02/19 10:37:12, dconnelly wrote:
> > It turns out the std::erase(std::remove(bridges_.begin(),
> bridges_.end(),
> > bridge), bridges_.end()) can sometimes stomp memory when applied to a
> > ScopedVector.
>
> > std::remove doesn't guarantee that the elements from the new end
> iterator to the
> > old end iterator are actually removed elements -- in fact, the
> elements in that
> > range are undefined.
>
> > http://www.cplusplus.com/reference/algorithm/remove/
>
> > So in some cases, after calling std::remove, all the elements in the
> range
> > [bridges_.begin(), bridges_.end()) were the same pointer, and since
> this is a
> > ScopedVector, calling bridges_.erase() on any of them led to a
> double-free.
>
> > Given this, it doesn't seem like std::remove is safe to use on
> ScopedVectors.
> > I've changed the code back.
>
> Sigh. Bitten by both scopedness and STL annoyingness.
>
> My apologies for misleading you.
>
> https://codereview.chromium.org/864963004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dconnelly, 2015-03-05 08:54:54]

The CQ bit was checked by dconnelly@chromium.org

[commit-bot: I haz the power, 2015-03-05 08:55:32]

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

[commit-bot: I haz the power, 2015-03-05 13:16:56]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-03-05 13:17:34]

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