[Presentation API] Fix potential callback leaks in PSImpl.

[Presentation API] Fix potential callback leaks in PSImpl.

Background: mojo rolled out a new version that requires callbacks (as
defined by the => syntax in mojom) must be invoked before they go out
of scope, otherwise it indicates a connection error on the callback's
recipient, which will destroy mojom implementations that are strongly
bound to the connection.

Note that a partial fix was added in crrev.com/1019173002. This patch
addresses the remaining points.

BUG=468575
Committed: https://crrev.com/31fe0da9286d9f24b9cd33e6d8c6c55651f5f47e
Cr-Commit-Position: refs/heads/master@{#322299}

[imcheng, 2015-03-24 02:03:25]

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

[imcheng, 2015-03-24 02:03:26]

Anton, mark: PTAL. I feel that the callback style API between
PresentationDispatcher and PSImpl might not necessarily be the ideal model after
the mojo change so I am just putting this out there for consideration. As we add
the few remaining callbacks (messaging APIs, state change listeners) to PSImpl,
I feel the logic to make sure no callback is "leaked" will get even more
complex, if it isn't complex enough already. The Reset() logic also potentially
creates a burst of IPCs which isn't ideal and what we wanted to avoid in the
first place.

While the Client= syntax has been removed from mojo, it's still possible to
write APIs where both sides have references to each other and invoke their
methods explicitly, and I feel like it is the better model here in that it
allows a simpler internal state in PSImpl. Though with that approach it looks
like PSDispatcher will need to maintain some state to correlate requests with
responses. In particular we shouldn't leak the Blink callbacks.

PTAL and let me know your thoughts. We can discuss on Wednesday as well.

[mark a. foltz, 2015-03-24 23:00:25]

lgtm

I agree with your concerns about bookkeeping of callbacks.  However if we
implement the request/response model with ids it seems like we will just end up
needing to track request objects as well?

Let's discuss in person.

[whywhat, 2015-03-25 17:05:54]

lgtm

https://codereview.chromium.org/996173006/diff/1/content/browser/presentation...
File content/browser/presentation/presentation_service_impl.cc (right):

https://codereview.chromium.org/996173006/diff/1/content/browser/presentation...
content/browser/presentation/presentation_service_impl.cc:187:
++next_request_session_id_;
I'd encapsulate this logic into something like:

int request_session_id = RegisterNewSessionCallback(callback);

since it's repeated in two places here.

https://codereview.chromium.org/996173006/diff/1/content/browser/presentation...
File content/browser/presentation/presentation_service_impl.h (right):

https://codereview.chromium.org/996173006/diff/1/content/browser/presentation...
content/browser/presentation/presentation_service_impl.h:187: void
RunAndEraseNewMojoCallback(
s/New/NewSession?

https://codereview.chromium.org/996173006/diff/1/content/browser/presentation...
content/browser/presentation/presentation_service_impl.h:251:
base::hash_map<int, linked_ptr<NewSessionMojoCallback>> pending_session_cbs_;
Is it possible to just keep a set of Callback pointers or something? Then upon
running a callback you would erase it from the set and upon service shutdown,
iterate over the set and fire the remaining callbacks with errors?

[imcheng, 2015-03-25 23:03:43]

https://codereview.chromium.org/996173006/diff/1/content/browser/presentation...
File content/browser/presentation/presentation_service_impl.cc (right):

https://codereview.chromium.org/996173006/diff/1/content/browser/presentation...
content/browser/presentation/presentation_service_impl.cc:187:
++next_request_session_id_;
On 2015/03/25 17:05:54, whywhat wrote:
> I'd encapsulate this logic into something like:
> 
> int request_session_id = RegisterNewSessionCallback(callback);
> 
> since it's repeated in two places here.

Done.

https://codereview.chromium.org/996173006/diff/1/content/browser/presentation...
File content/browser/presentation/presentation_service_impl.h (right):

https://codereview.chromium.org/996173006/diff/1/content/browser/presentation...
content/browser/presentation/presentation_service_impl.h:187: void
RunAndEraseNewMojoCallback(
On 2015/03/25 17:05:54, whywhat wrote:
> s/New/NewSession?

Done.

https://codereview.chromium.org/996173006/diff/1/content/browser/presentation...
content/browser/presentation/presentation_service_impl.h:251:
base::hash_map<int, linked_ptr<NewSessionMojoCallback>> pending_session_cbs_;
On 2015/03/25 17:05:54, whywhat wrote:
> Is it possible to just keep a set of Callback pointers or something? Then upon
> running a callback you would erase it from the set and upon service shutdown,
> iterate over the set and fire the remaining callbacks with errors?

I think that will be quite complicated to implement. If we bind
NewSessionMojoCallback* instead of request ids to OnStartOrJoinSession*() we
would have to somehow make sure the pointer is still valid by the time they are
invoked.

[imcheng, 2015-03-26 01:52:51]

The CQ bit was checked by imcheng@chromium.org

[imcheng, 2015-03-26 01:52:52]

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

[commit-bot: I haz the power, 2015-03-26 01:54:23]

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

[commit-bot: I haz the power, 2015-03-26 02:08:58]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-03-26 02:10:18]

Patchset 2 (id:??) landed as
https://crrev.com/31fe0da9286d9f24b9cd33e6d8c6c55651f5f47e
Cr-Commit-Position: refs/heads/master@{#322299}