[Presentation API] return valid WebPresentationConnection object in PresentationReceiver

[Presentation API] return valid WebPresentationConnection object in PresentationReceiver

PresentationReceiver::onReceiverConnectionAvailable() returns nullptr if receiver.connectionList property is not accessed, causing PresentationConnectionProxy holding invalid source_connection_ and may crash render process.

BUG=692629
Review-Url: https://codereview.chromium.org/2706453002
Cr-Commit-Position: refs/heads/master@{#452213}
Committed: https://chromium.googlesource.com/chromium/src/+/00db1bcf1a71b903e3a03ce5e277c4d5d1b49a88

[zhaobin, 2017-02-17 00:31:59]

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

[zhaobin, 2017-02-17 00:32:48]

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

[commit-bot: I haz the power, 2017-02-17 00:34:41]

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-17 00:48:18]

Thanks for the quick fix.  Can a layout test be written for providing a receiver
connection, then accessing the property?

[commit-bot: I haz the power, 2017-02-17 02:39:50]

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

[commit-bot: I haz the power, 2017-02-17 02:39:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[zhaobin, 2017-02-17 18:52:17]

On 2017/02/17 00:48:18, mark a. foltz wrote:
> Thanks for the quick fix.  Can a layout test be written for providing a
receiver
> connection, then accessing the property?

Done.

[mark a. foltz, 2017-02-21 17:47:38]

Thanks for adding a test case :)  Slowly improving coverage!

LGTM with comments on layout test addressed.

https://codereview.chromium.org/2706453002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/presentation/presentation-onreceiverconnection.html
(right):

https://codereview.chromium.org/2706453002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/presentation/presentation-onreceiverconnection.html:16:
var receiver = navigator.presentation.receiver;
s/var/const/

https://codereview.chromium.org/2706453002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/presentation/presentation-onreceiverconnection.html:19:
setTimeout(() => {
Does the rest of the test need to be in an event handler?  The line above to get
the receiver object is synchronous.

https://codereview.chromium.org/2706453002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/presentation/presentation-onreceiverconnection.html:27:
list.connections.map(connection => {
Since the length is 1, you could just write

assert_equals(list.connections[0].url, url);
etc.

Not a big deal :)

https://codereview.chromium.org/2706453002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/presentation/resources/presentation-service-mock.js
(right):

https://codereview.chromium.org/2706453002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/presentation/resources/presentation-service-mock.js:49:
var mojo_url = new url.Url();
s/var/const/ here and below.

Also, Javascript variables should be named lowerCamelCase, so:

mojoUrl
controllerConnectionPtr

etc.

https://google.github.io/styleguide/javascriptguide.xml#Naming

[zhaobin, 2017-02-21 19:11:38]

https://codereview.chromium.org/2706453002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/presentation/presentation-onreceiverconnection.html
(right):

https://codereview.chromium.org/2706453002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/presentation/presentation-onreceiverconnection.html:16:
var receiver = navigator.presentation.receiver;
On 2017/02/21 17:47:38, mark a. foltz wrote:
> s/var/const/

Done.

https://codereview.chromium.org/2706453002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/presentation/presentation-onreceiverconnection.html:19:
setTimeout(() => {
On 2017/02/21 17:47:38, mark a. foltz wrote:
> Does the rest of the test need to be in an event handler?  The line above to
get
> the receiver object is synchronous.

Yes, put them in an event handler to make sure this.client_ is set when calling
mockService.onReceiverConnectionAvailable(). navigator.presentation.receiver
invokes mockService.setClient() (mojo API of PSImpl) to set this.client_ of
mockService. It is a mojo call and has some delays. If there is no setTimeout(),
this.client_ will not be set when calling
mockService.onReceiverConnectionAvailable().

https://codereview.chromium.org/2706453002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/presentation/presentation-onreceiverconnection.html:27:
list.connections.map(connection => {
On 2017/02/21 17:47:38, mark a. foltz wrote:
> Since the length is 1, you could just write
> 
> assert_equals(list.connections[0].url, url);
> etc.
> 
> Not a big deal :)

Done.

https://codereview.chromium.org/2706453002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/presentation/resources/presentation-service-mock.js
(right):

https://codereview.chromium.org/2706453002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/presentation/resources/presentation-service-mock.js:49:
var mojo_url = new url.Url();
On 2017/02/21 17:47:38, mark a. foltz wrote:
> s/var/const/ here and below.
> 
> Also, Javascript variables should be named lowerCamelCase, so:
> 
> mojoUrl
> controllerConnectionPtr
> 
> etc.
> 
> https://google.github.io/styleguide/javascriptguide.xml#Naming

Done.

[zhaobin, 2017-02-21 19:11:48]

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

[commit-bot: I haz the power, 2017-02-21 19:13:02]

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 21:16:29]

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

[commit-bot: I haz the power, 2017-02-21 21:16:30]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[imcheng, 2017-02-21 23:53:16]

lgtm

https://codereview.chromium.org/2706453002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/presentation/presentation-onreceiverconnection.html
(right):

https://codereview.chromium.org/2706453002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/presentation/presentation-onreceiverconnection.html:32:
}, "Test presetnation.receiver.connectionList resolves with incoming
connection.");
s/presetnation/presentation

[zhaobin, 2017-02-22 00:42:46]

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

[commit-bot: I haz the power, 2017-02-22 00:44:36]

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

[zhaobin, 2017-02-22 18:30:48]

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

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

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-22 20:25:57]

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

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

Dry run: This issue passed the CQ dry run.

[zhaobin, 2017-02-22 21:13:48]

The CQ bit was checked by zhaobin@chromium.org

[zhaobin, 2017-02-22 21:13:49]

The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org,
imcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2706453002/#ps60001
(title: "resolve code review comments from Derek")

[commit-bot: I haz the power, 2017-02-22 21:14:57]

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 21:36:10]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487798028881920,
"parent_rev": "1a9852b7bbd9c9407d4816f89759d439776b4e31", "commit_rev":
"00db1bcf1a71b903e3a03ce5e277c4d5d1b49a88"}

[commit-bot: I haz the power, 2017-02-22 21:36:45]

Description was changed from

==========
[Presentation API] return valid WebPresentationConnection object in
PresentationReceiver

PresentationReceiver::onReceiverConnectionAvailable() returns nullptr if
receiver.connectionList property is not accessed, causing
PresentationConnectionProxy holding invalid source_connection_ and may crash
render process.

BUG=692629
==========

to

==========
[Presentation API] return valid WebPresentationConnection object in
PresentationReceiver

PresentationReceiver::onReceiverConnectionAvailable() returns nullptr if
receiver.connectionList property is not accessed, causing
PresentationConnectionProxy holding invalid source_connection_ and may crash
render process.

BUG=692629

Review-Url: https://codereview.chromium.org/2706453002
Cr-Commit-Position: refs/heads/master@{#452213}
Committed:
https://chromium.googlesource.com/chromium/src/+/00db1bcf1a71b903e3a03ce5e277...
==========

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

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