Check delegate before running Pepper media-device enumeration/monitoring requests.

Check delegate before running Pepper media-device enumeration/monitoring requests.

This is intended to address a crash that appears to occur because
the delegate object that performs enumerations is not available.

BUG=716377
Review-Url: https://codereview.chromium.org/2855373003
Cr-Commit-Position: refs/heads/master@{#469601}
Committed: https://chromium.googlesource.com/chromium/src/+/8dfbdfabb1747778fcaa216f662790dc7bc6ad3c

[Guido Urdaneta, 2017-05-04 12:31:36]

Description was changed from

==========
Check delegate before running Pepper media device/enumeration requests.

This is intended to address a crash that appears to occur because
the delegate object that performs enumerations is not available.

BUG=716377
==========

to

==========
Check delegate before running Pepper media-device enumeration/monitoring
requests.

This is intended to address a crash that appears to occur because
the delegate object that performs enumerations is not available.

BUG=716377
==========

[Guido Urdaneta, 2017-05-04 12:31:48]

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

[commit-bot: I haz the power, 2017-05-04 12:32:03]

Dry run: CQ is trying da patch.

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

[Guido Urdaneta, 2017-05-04 13:09:51]

guidou@chromium.org changed reviewers:
+ bbudge@chromium.org, yzshen@chromium.org

[Guido Urdaneta, 2017-05-04 13:13:58]

Hi, PTAL.

bbudge@: please do OWNER's review.

yzshen@: Years ago, you added the DCHECK I am removing in this CL. Do you have
any thoughts about this?

[commit-bot: I haz the power, 2017-05-04 13:26:44]

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

[commit-bot: I haz the power, 2017-05-04 13:26:45]

Dry run: This issue passed the CQ dry run.

[bbudge, 2017-05-04 16:57:37]

lgtm

[Guido Urdaneta, 2017-05-05 05:29:43]

The CQ bit was checked by guidou@chromium.org

[commit-bot: I haz the power, 2017-05-05 05:29:55]

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-05-05 05:35:01]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1493962183005570, "parent_rev":
"a22d5fb78823f01b93f1d62465fa178bd5e2b143", "commit_rev":
"8dfbdfabb1747778fcaa216f662790dc7bc6ad3c"}

[commit-bot: I haz the power, 2017-05-05 05:35:11]

Description was changed from

==========
Check delegate before running Pepper media-device enumeration/monitoring
requests.

This is intended to address a crash that appears to occur because
the delegate object that performs enumerations is not available.

BUG=716377
==========

to

==========
Check delegate before running Pepper media-device enumeration/monitoring
requests.

This is intended to address a crash that appears to occur because
the delegate object that performs enumerations is not available.

BUG=716377

Review-Url: https://codereview.chromium.org/2855373003
Cr-Commit-Position: refs/heads/master@{#469601}
Committed:
https://chromium.googlesource.com/chromium/src/+/8dfbdfabb1747778fcaa216f6627...
==========

[commit-bot: I haz the power, 2017-05-05 05:35:12]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/8dfbdfabb1747778fcaa216f6627...

[yzshen1, 2017-05-05 17:08:11]

https://codereview.chromium.org/2855373003/diff/1/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_device_enumeration_host_helper.cc (right):

https://codereview.chromium.org/2855373003/diff/1/content/renderer/pepper/pep...
content/renderer/pepper/pepper_device_enumeration_host_helper.cc:38: if
(!owner->delegate_)
Sorry this has been too long ago. I don't remember this code. :)

Do we understand why delegate could be null? Is there a legitimate reason?

If we early return from, the callback will never be called, does the caller
expect that?

[Guido Urdaneta, 2017-05-05 17:18:12]

https://codereview.chromium.org/2855373003/diff/1/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_device_enumeration_host_helper.cc (right):

https://codereview.chromium.org/2855373003/diff/1/content/renderer/pepper/pep...
content/renderer/pepper/pepper_device_enumeration_host_helper.cc:38: if
(!owner->delegate_)
On 2017/05/05 17:08:11, yzshen1 wrote:
> Sorry this has been too long ago. I don't remember this code. :)
> 
> Do we understand why delegate could be null? Is there a legitimate reason?
> 
> If we early return from, the callback will never be called, does the caller
> expect that?
> 

It is also expected if the document_url is not valid a few lines above, so I
thought it was OK for a somewhat similar situation of a null delegate. 
I don't know why the delegate can become null. I am familiar with the device
enumeration code, but not so much with the lifetimes of the Pepper objects. I
glanced through the source code, but did not find an obvious reason why this
could be the case.

An alternative to leaving the callback hanging could be to invoke the callback
with an empty result.

I can prepare a patch that does that if you think that is better.

[yzshen1, 2017-05-05 17:22:31]

https://codereview.chromium.org/2855373003/diff/1/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_device_enumeration_host_helper.cc (right):

https://codereview.chromium.org/2855373003/diff/1/content/renderer/pepper/pep...
content/renderer/pepper/pepper_device_enumeration_host_helper.cc:38: if
(!owner->delegate_)
On 2017/05/05 17:18:12, Guido Urdaneta wrote:
> On 2017/05/05 17:08:11, yzshen1 wrote:
> > Sorry this has been too long ago. I don't remember this code. :)
> > 
> > Do we understand why delegate could be null? Is there a legitimate reason?
> > 
> > If we early return from, the callback will never be called, does the caller
> > expect that?
> > 
> 
> It is also expected if the document_url is not valid a few lines above, so I
> thought it was OK for a somewhat similar situation of a null delegate. 
That makes sense. :)


> I don't know why the delegate can become null. I am familiar with the device
> enumeration code, but not so much with the lifetimes of the Pepper objects. I
> glanced through the source code, but did not find an obvious reason why this
> could be the case.
This is what I am a little concerned about. There may be something wrong
elsewhere leading to an inconsistent state. 


> 
> An alternative to leaving the callback hanging could be to invoke the callback
> with an empty result.
> 
> I can prepare a patch that does that if you think that is better.

You definitely know this code better than I do now. So it is up to you. :)