[DIAL] Break circular dependency in DialFetchDeviceDescriptionFunction.

[DIAL] Break circular dependency in DialFetchDeviceDescriptionFunction.

DeviceDescriptionFetcher has 2 references (in the 2 calbacks) to the
DialFetchDeviceDescriptionFunction that owns it, but only 1 of the 2
references is removed at the end state. This results in a circular
dependency which prevents the function from being destroyed. This patch
manually destroys the DeviceDescriptionFetcher after the fetching is
done to break the circular dependency.

BUG=688089
Review-Url: https://codereview.chromium.org/2668383004
Cr-Commit-Position: refs/heads/master@{#447857}
Committed: https://chromium.googlesource.com/chromium/src/+/d8033dadec98c7c2f4c407f89c128b166b5922ef

[imcheng, 2017-02-02 20:02:42]

Description was changed from

==========
[DIAL] Break circular dependency in DialFetchDeviceDescriptionFunction.

DeviceDescriptionFetcher has 2 references (in the 2 calbacks) to the
DialFetchDeviceDescriptionFunction that owns it, but only 1 of the 2
references is removed at the end state. This results in a circular
dependency which prevents the function from being destroyed. This patch
manually destroys the DeviceDescriptionFetcher after the fetching is
done to break the circular dependency.

BUG=671932
==========

to

==========
[DIAL] Break circular dependency in DialFetchDeviceDescriptionFunction.

DeviceDescriptionFetcher has 2 references (in the 2 calbacks) to the
DialFetchDeviceDescriptionFunction that owns it, but only 1 of the 2
references is removed at the end state. This results in a circular
dependency which prevents the function from being destroyed. This patch
manually destroys the DeviceDescriptionFetcher after the fetching is
done to break the circular dependency.

BUG=688089
==========

[imcheng, 2017-02-02 20:03:54]

imcheng@chromium.org changed reviewers:
+ mfoltz@chromium.org

[imcheng, 2017-02-02 20:03:54]

mfoltz@: PTAL.

I wonder if it would make sense to make DeviceDescriptionFetcher destroy itself
upon fetch completion. Since it is intended to be used in a one-shot operation.

[mark a. foltz, 2017-02-02 20:24:42]

On 2017/02/02 at 20:03:54, imcheng wrote:
> mfoltz@: PTAL.
> 
> I wonder if it would make sense to make DeviceDescriptionFetcher destroy
itself upon fetch completion. Since it is intended to be used in a one-shot
operation.

Wouldn't that result in a double delete when the unique_ptr holding it was
destroyed?

[mark a. foltz, 2017-02-02 20:27:32]

LGTM but there might be a simpler fix.

https://codereview.chromium.org/2668383004/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/dial/dial_api.cc (right):

https://codereview.chromium.org/2668383004/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/dial/dial_api.cc:241: void
DialFetchDeviceDescriptionFunction::OnFetchComplete(
Can you add a comment in device_description_fetcher.h indicating that it should
be destroyed in the onsuccess/onerror callbacks?

https://codereview.chromium.org/2668383004/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/dial/dial_api.cc:243:
device_description_fetcher_.reset();
Would resetting the non-invoked callback in DeviceDescriptionFetcher also fix
the problem by dropping the additional reference?  That wouldn't require the
client to drop references explicitly.

[imcheng, 2017-02-02 20:35:51]

On 2017/02/02 20:24:42, mark a. foltz wrote:
> On 2017/02/02 at 20:03:54, imcheng wrote:
> > mfoltz@: PTAL.
> > 
> > I wonder if it would make sense to make DeviceDescriptionFetcher destroy
> itself upon fetch completion. Since it is intended to be used in a one-shot
> operation.
> 
> Wouldn't that result in a double delete when the unique_ptr holding it was
> destroyed?

I was thinking we could make it so that the function does not hold on to the
DeviceDescriptionFetcher.

[imcheng, 2017-02-02 20:44:31]

https://codereview.chromium.org/2668383004/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/dial/dial_api.cc (right):

https://codereview.chromium.org/2668383004/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/dial/dial_api.cc:241: void
DialFetchDeviceDescriptionFunction::OnFetchComplete(
On 2017/02/02 20:27:32, mark a. foltz wrote:
> Can you add a comment in device_description_fetcher.h indicating that it
should
> be destroyed in the onsuccess/onerror callbacks?

I added comment here explaining why we have to destroy it here. It's due to the
way we bind the callbacks passed in.

https://codereview.chromium.org/2668383004/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/dial/dial_api.cc:243:
device_description_fetcher_.reset();
On 2017/02/02 20:27:32, mark a. foltz wrote:
> Would resetting the non-invoked callback in DeviceDescriptionFetcher also fix
> the problem by dropping the additional reference?  That wouldn't require the
> client to drop references explicitly.

Yes, that would work too. But I see no reason to hold on to the fetcher since
it's no longer (re)usable.

[imcheng, 2017-02-02 20:46:07]

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

[commit-bot: I haz the power, 2017-02-02 20:46:45]

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

[mark a. foltz, 2017-02-02 21:40:13]

On 2017/02/02 at 20:35:51, imcheng wrote:
> On 2017/02/02 20:24:42, mark a. foltz wrote:
> > On 2017/02/02 at 20:03:54, imcheng wrote:
> > > mfoltz@: PTAL.
> > > 
> > > I wonder if it would make sense to make DeviceDescriptionFetcher destroy
> > itself upon fetch completion. Since it is intended to be used in a one-shot
> > operation.
> > 
> > Wouldn't that result in a double delete when the unique_ptr holding it was
> > destroyed?
> 
> I was thinking we could make it so that the function does not hold on to the
DeviceDescriptionFetcher.

What would keep it alive until the fetch completes?

[commit-bot: I haz the power, 2017-02-02 21:43:24]

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

[commit-bot: I haz the power, 2017-02-02 21:43:25]

Dry run: This issue passed the CQ dry run.

[imcheng, 2017-02-02 21:50:50]

On 2017/02/02 21:40:13, mark a. foltz wrote:
> On 2017/02/02 at 20:35:51, imcheng wrote:
> > On 2017/02/02 20:24:42, mark a. foltz wrote:
> > > On 2017/02/02 at 20:03:54, imcheng wrote:
> > > > mfoltz@: PTAL.
> > > > 
> > > > I wonder if it would make sense to make DeviceDescriptionFetcher destroy
> > > itself upon fetch completion. Since it is intended to be used in a
one-shot
> > > operation.
> > > 
> > > Wouldn't that result in a double delete when the unique_ptr holding it was
> > > destroyed?
> > 
> > I was thinking we could make it so that the function does not hold on to the
> DeviceDescriptionFetcher.
> 
> What would keep it alive until the fetch completes?

We could do something like this in
DialFetchDeviceDescriptionFunction::MaybeStartFetch():

new DeviceDescriptionFetcher(...)->Start();


and in the DeviceDescriptionFetcher implemenetation, it would delete itself
after invoking either callback:

void DeviceDescriptionFetcher::OnURLFetchComplete(...) {
  std::move(success_cb_).Run(...);
  delete this;
}

void DeviceDescriptionFetcher::ReportError(...) {
  std::move(error_cb_).Run(message);
  delete this;
}

[mark a. foltz, 2017-02-02 22:05:02]

On 2017/02/02 at 21:50:50, imcheng wrote:
> On 2017/02/02 21:40:13, mark a. foltz wrote:
> > On 2017/02/02 at 20:35:51, imcheng wrote:
> > > On 2017/02/02 20:24:42, mark a. foltz wrote:
> > > > On 2017/02/02 at 20:03:54, imcheng wrote:
> > > > > mfoltz@: PTAL.
> > > > > 
> > > > > I wonder if it would make sense to make DeviceDescriptionFetcher
destroy
> > > > itself upon fetch completion. Since it is intended to be used in a
one-shot
> > > > operation.
> > > > 
> > > > Wouldn't that result in a double delete when the unique_ptr holding it
was
> > > > destroyed?
> > > 
> > > I was thinking we could make it so that the function does not hold on to
the
> > DeviceDescriptionFetcher.
> > 
> > What would keep it alive until the fetch completes?
> 
> We could do something like this in
DialFetchDeviceDescriptionFunction::MaybeStartFetch():
> 
> new DeviceDescriptionFetcher(...)->Start();
> 
> 
> and in the DeviceDescriptionFetcher implemenetation, it would delete itself
after invoking either callback:
> 
> void DeviceDescriptionFetcher::OnURLFetchComplete(...) {
>   std::move(success_cb_).Run(...);
>   delete this;
> }
> 
> void DeviceDescriptionFetcher::ReportError(...) {
>   std::move(error_cb_).Run(message);
>   delete this;
> }

That could work.   You might run into shutdown order issues if the fetcher
implementation on the IO thread is destroyed before invoking its callback on the
UI thread.  That would leak the fetcher on the UI thread.  I'd have to dig
through the fetcher code a bit to see if that's safe.

[imcheng, 2017-02-02 22:13:10]

Ok. Indeed it can be problematic during shutdown. I will land this for now.

[imcheng, 2017-02-02 22:13:21]

The CQ bit was checked by imcheng@chromium.org

[imcheng, 2017-02-02 22:13:21]

The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org
Link to the patchset: https://codereview.chromium.org/2668383004/#ps20001
(title: "Addressed Mark's comments")

[commit-bot: I haz the power, 2017-02-02 22:13:47]

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-02 22:19:52]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1486073601194410,
"parent_rev": "36075614a07a4242d459810b956073be7428e56a", "commit_rev":
"d8033dadec98c7c2f4c407f89c128b166b5922ef"}

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

Description was changed from

==========
[DIAL] Break circular dependency in DialFetchDeviceDescriptionFunction.

DeviceDescriptionFetcher has 2 references (in the 2 calbacks) to the
DialFetchDeviceDescriptionFunction that owns it, but only 1 of the 2
references is removed at the end state. This results in a circular
dependency which prevents the function from being destroyed. This patch
manually destroys the DeviceDescriptionFetcher after the fetching is
done to break the circular dependency.

BUG=688089
==========

to

==========
[DIAL] Break circular dependency in DialFetchDeviceDescriptionFunction.

DeviceDescriptionFetcher has 2 references (in the 2 calbacks) to the
DialFetchDeviceDescriptionFunction that owns it, but only 1 of the 2
references is removed at the end state. This results in a circular
dependency which prevents the function from being destroyed. This patch
manually destroys the DeviceDescriptionFetcher after the fetching is
done to break the circular dependency.

BUG=688089

Review-Url: https://codereview.chromium.org/2668383004
Cr-Commit-Position: refs/heads/master@{#447857}
Committed:
https://chromium.googlesource.com/chromium/src/+/d8033dadec98c7c2f4c407f89c12...
==========

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

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/d8033dadec98c7c2f4c407f89c12...