[Presentation API] Fix race condition in which state change is lost.

[Presentation API] Fix race condition in which state change is lost.

Currently there is a race condition between the PresentationConnection
being returned to PresentationDispatcher on a successful start() or
reconnect(), and PresentationDispatcher being notified by the browser
that the state change has become connected.

Although PresentationServiceImpl issues the state change strictly after
executing the callback returning the PresentationConnection, it appears
that they are using distinct independent message pipes, which do not
guarantee order of arrival between these two messages. If the state
change message arrives first, then it will be lost due to the
connection not yet registered with Blink at that point in time.

The fix here is to change state to connected in Blink, after the
PresentationConnection is registered.

This fix needs to be merged back to 57.

Additional test assertions will be in a followup patch.

BUG=684664
Review-Url: https://codereview.chromium.org/2653683006
Cr-Commit-Position: refs/heads/master@{#446465}
Committed: https://chromium.googlesource.com/chromium/src/+/1ebdd16432860b90eaaeaba7042c2e9761b17ef7

[imcheng, 2017-01-24 22:26:29]

Description was changed from

==========
[Presentation API] Fix race condition in which state change is lost.

Currently there is a race condition between the PresentationConnection
being returned to PresentationDispatcher on a successful start() or
reconnect(), and PresentationDispatcher being notified by the browser
that the state change has become connected.

Although PresentationServiceImpl issues the state change strictly after
executing the callback returning the PresentationConnection, it appears
that they are using distinct independent message pipes, which do not
guarantee order of arrival between these two events.

The fix here is to change state to connected in Blink, after the
PresentationConnection is registered.

This fix needs to be merged back to 56 and 57.

BUG=684664
==========

to

==========
[Presentation API] Fix race condition in which state change is lost.

Currently there is a race condition between the PresentationConnection
being returned to PresentationDispatcher on a successful start() or
reconnect(), and PresentationDispatcher being notified by the browser
that the state change has become connected.

Although PresentationServiceImpl issues the state change strictly after
executing the callback returning the PresentationConnection, it appears
that they are using distinct independent message pipes, which do not
guarantee order of arrival between these two events.

The fix here is to change state to connected in Blink, after the
PresentationConnection is registered.

This fix needs to be merged back to 56 and 57.

Additional test assertions will be in a followup patch.

BUG=684664
==========

[imcheng, 2017-01-24 22:35:21]

Description was changed from

==========
[Presentation API] Fix race condition in which state change is lost.

Currently there is a race condition between the PresentationConnection
being returned to PresentationDispatcher on a successful start() or
reconnect(), and PresentationDispatcher being notified by the browser
that the state change has become connected.

Although PresentationServiceImpl issues the state change strictly after
executing the callback returning the PresentationConnection, it appears
that they are using distinct independent message pipes, which do not
guarantee order of arrival between these two events.

The fix here is to change state to connected in Blink, after the
PresentationConnection is registered.

This fix needs to be merged back to 56 and 57.

Additional test assertions will be in a followup patch.

BUG=684664
==========

to

==========
[Presentation API] Fix race condition in which state change is lost.

Currently there is a race condition between the PresentationConnection
being returned to PresentationDispatcher on a successful start() or
reconnect(), and PresentationDispatcher being notified by the browser
that the state change has become connected.

Although PresentationServiceImpl issues the state change strictly after
executing the callback returning the PresentationConnection, it appears
that they are using distinct independent message pipes, which do not
guarantee order of arrival between these two messages. If the state
change message arrives first, then it will be lost due to the
connection not yet registered with Blink at that point in time.

The fix here is to change state to connected in Blink, after the
PresentationConnection is registered.

This fix needs to be merged back to 56 and 57.

Additional test assertions will be in a followup patch.

BUG=684664
==========

[imcheng, 2017-01-24 22:35:37]

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

[imcheng, 2017-01-24 22:35:38]

Mark, Bin: PTAL

[imcheng, 2017-01-24 22:50:46]

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

[commit-bot: I haz the power, 2017-01-24 22:51:32]

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

[zhaobin, 2017-01-24 23:06:47]

lgtm. In M57 connection state change event is fired asychronously; while in M56
it is fired sychronously; Double check it works fine in M56 when porting back?

[commit-bot: I haz the power, 2017-01-24 23:31:22]

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

[commit-bot: I haz the power, 2017-01-24 23:31:22]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[imcheng, 2017-01-25 18:55:58]

Description was changed from

==========
[Presentation API] Fix race condition in which state change is lost.

Currently there is a race condition between the PresentationConnection
being returned to PresentationDispatcher on a successful start() or
reconnect(), and PresentationDispatcher being notified by the browser
that the state change has become connected.

Although PresentationServiceImpl issues the state change strictly after
executing the callback returning the PresentationConnection, it appears
that they are using distinct independent message pipes, which do not
guarantee order of arrival between these two messages. If the state
change message arrives first, then it will be lost due to the
connection not yet registered with Blink at that point in time.

The fix here is to change state to connected in Blink, after the
PresentationConnection is registered.

This fix needs to be merged back to 56 and 57.

Additional test assertions will be in a followup patch.

BUG=684664
==========

to

==========
[Presentation API] Fix race condition in which state change is lost.

Currently there is a race condition between the PresentationConnection
being returned to PresentationDispatcher on a successful start() or
reconnect(), and PresentationDispatcher being notified by the browser
that the state change has become connected.

Although PresentationServiceImpl issues the state change strictly after
executing the callback returning the PresentationConnection, it appears
that they are using distinct independent message pipes, which do not
guarantee order of arrival between these two messages. If the state
change message arrives first, then it will be lost due to the
connection not yet registered with Blink at that point in time.

The fix here is to change state to connected in Blink, after the
PresentationConnection is registered.

This fix needs to be merged back to 57.

Additional test assertions will be in a followup patch.

BUG=684664
==========

[mark a. foltz, 2017-01-26 19:22:33]

lgtm

[mark a. foltz, 2017-01-26 19:24:54]

I'm assuming the controller delegate passes the state change onto the MRPM. 
Yes, the component <==> browser pipe is almost certainly distinct from the
renderer <==> browser pipe. They are different processes after all.

[imcheng, 2017-01-26 20:17:08]

On 2017/01/26 19:24:54, mark a. foltz wrote:
> I'm assuming the controller delegate passes the state change onto the MRPM. 

We don't do that currently. In fact the MRPM does not keep track of presentation
connection states for each route. State change only flows from MRPM to MR not
the other way.

> Yes, the component <==> browser pipe is almost certainly distinct from the
> renderer <==> browser pipe. They are different processes after all.

The pipes in question are renderer -> browser (PresentationService) and browser
-> renderer (PresentationServiceClient). The former is used for answering the
callback for start()/reconnect() and latter is used for signaling state change
to "connected".

[imcheng, 2017-01-26 20:17:41]

The CQ bit was checked by imcheng@chromium.org

[commit-bot: I haz the power, 2017-01-26 20:18:14]

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

[imcheng, 2017-01-26 20:22:41]

To expand slightly on this:

On 2017/01/26 20:17:08, imcheng wrote:
> On 2017/01/26 19:24:54, mark a. foltz wrote:
> > I'm assuming the controller delegate passes the state change onto the MRPM. 
> 
> We don't do that currently. In fact the MRPM does not keep track of
presentation
> connection states for each route. State change only flows from MRPM to MR not
> the other way.
> 

We should definitely rethink the state change mechanism to be more consistent
and less confusing. I am still in favor of letting MRPM initiate all the state
changes, but obviously we have to be careful about the ordering of events so
bugs like this doesn't occur again. 

> > Yes, the component <==> browser pipe is almost certainly distinct from the
> > renderer <==> browser pipe. They are different processes after all.
> 
> The pipes in question are renderer -> browser (PresentationService) and
browser
> -> renderer (PresentationServiceClient). The former is used for answering the
> callback for start()/reconnect() and latter is used for signaling state change
> to "connected".

I believe we can use associated interfaces if we want the two interfaces to use
the same pipe:

https://www.chromium.org/developers/design-documents/mojo/associated-interfaces

[commit-bot: I haz the power, 2017-01-26 22:01:56]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1485461861310410, "parent_rev":
"c972f1886fe4ea5a7fa8e467a499aeae2a315db9", "commit_rev":
"1ebdd16432860b90eaaeaba7042c2e9761b17ef7"}

[commit-bot: I haz the power, 2017-01-26 22:03:52]

Description was changed from

==========
[Presentation API] Fix race condition in which state change is lost.

Currently there is a race condition between the PresentationConnection
being returned to PresentationDispatcher on a successful start() or
reconnect(), and PresentationDispatcher being notified by the browser
that the state change has become connected.

Although PresentationServiceImpl issues the state change strictly after
executing the callback returning the PresentationConnection, it appears
that they are using distinct independent message pipes, which do not
guarantee order of arrival between these two messages. If the state
change message arrives first, then it will be lost due to the
connection not yet registered with Blink at that point in time.

The fix here is to change state to connected in Blink, after the
PresentationConnection is registered.

This fix needs to be merged back to 57.

Additional test assertions will be in a followup patch.

BUG=684664
==========

to

==========
[Presentation API] Fix race condition in which state change is lost.

Currently there is a race condition between the PresentationConnection
being returned to PresentationDispatcher on a successful start() or
reconnect(), and PresentationDispatcher being notified by the browser
that the state change has become connected.

Although PresentationServiceImpl issues the state change strictly after
executing the callback returning the PresentationConnection, it appears
that they are using distinct independent message pipes, which do not
guarantee order of arrival between these two messages. If the state
change message arrives first, then it will be lost due to the
connection not yet registered with Blink at that point in time.

The fix here is to change state to connected in Blink, after the
PresentationConnection is registered.

This fix needs to be merged back to 57.

Additional test assertions will be in a followup patch.

BUG=684664

Review-Url: https://codereview.chromium.org/2653683006
Cr-Commit-Position: refs/heads/master@{#446465}
Committed:
https://chromium.googlesource.com/chromium/src/+/1ebdd16432860b90eaaeaba7042c...
==========

[commit-bot: I haz the power, 2017-01-26 22:03:53]

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