Mandoline: Move ContentHandlerConnection into separate file

Mandoline: Move ContentHandlerConnection into separate file

ContentHandlerConnection will serve as the equivalent of SiteInstance in Chromium.

In the future, ContentHandlerConnection will represent an Application instance for a particular origin/site.

As the complexity of this object will grow soon, it makes sense to move it out as a first step.

ContentHandlerConnection now also uses the same lifetime management idiom used in ApplicationConnection and elsewhere to help guard against reentrant calls during destruction and double deletes. This also makes it easier to unit test this class in the future as more functionality is added.

BUG=496937
Committed: https://crrev.com/09c726e468cf0e5e0c9a2d39888c63561d5838f0
Cr-Commit-Position: refs/heads/master@{#338173}

[Fady Samuel, 2015-07-08 22:26:27]

fsamuel@chromium.org changed reviewers:
+ jam@chromium.org

[jam, 2015-07-09 17:27:23]

https://codereview.chromium.org/1228743002/diff/20001/mojo/shell/application_...
File mojo/shell/application_manager.h (right):

https://codereview.chromium.org/1228743002/diff/20001/mojo/shell/application_...
mojo/shell/application_manager.h:148: friend class ContentHandlerConnection;
now that it't not an inner class, the friend declaration seems dubious. just
make OnContentHandlerError public

https://codereview.chromium.org/1228743002/diff/20001/mojo/shell/content_hand...
File mojo/shell/content_handler_connection.cc (right):

https://codereview.chromium.org/1228743002/diff/20001/mojo/shell/content_hand...
mojo/shell/content_handler_connection.cc:36: return;
in this case, how does the object get deleted?

https://codereview.chromium.org/1228743002/diff/20001/mojo/shell/content_hand...
File mojo/shell/content_handler_connection.h (right):

https://codereview.chromium.org/1228743002/diff/20001/mojo/shell/content_hand...
mojo/shell/content_handler_connection.h:35: 
nit: no blank line?

[Fady Samuel, 2015-07-09 17:49:22]

PTAL

https://codereview.chromium.org/1228743002/diff/20001/mojo/shell/application_...
File mojo/shell/application_manager.h (right):

https://codereview.chromium.org/1228743002/diff/20001/mojo/shell/application_...
mojo/shell/application_manager.h:148: friend class ContentHandlerConnection;
On 2015/07/09 17:27:23, jam wrote:
> now that it't not an inner class, the friend declaration seems dubious. just
> make OnContentHandlerError public

Done.

https://codereview.chromium.org/1228743002/diff/20001/mojo/shell/content_hand...
File mojo/shell/content_handler_connection.cc (right):

https://codereview.chromium.org/1228743002/diff/20001/mojo/shell/content_hand...
mojo/shell/content_handler_connection.cc:36: return;
On 2015/07/09 17:27:23, jam wrote:
> in this case, how does the object get deleted?

The point of this flag is to ensure that CloseConnection can only delete this
once.

https://codereview.chromium.org/1228743002/diff/20001/mojo/shell/content_hand...
File mojo/shell/content_handler_connection.h (right):

https://codereview.chromium.org/1228743002/diff/20001/mojo/shell/content_hand...
mojo/shell/content_handler_connection.h:35: 
On 2015/07/09 17:27:23, jam wrote:
> nit: no blank line?

Done.

[Fady Samuel, 2015-07-09 17:52:47]

https://codereview.chromium.org/1228743002/diff/40001/mojo/shell/content_hand...
File mojo/shell/content_handler_connection.cc (right):

https://codereview.chromium.org/1228743002/diff/40001/mojo/shell/content_hand...
mojo/shell/content_handler_connection.cc:24:
manager->ConnectToApplicationInternal(
Actually, it needs to be friend for this call.

[jam, 2015-07-09 18:00:18]

https://codereview.chromium.org/1228743002/diff/20001/mojo/shell/content_hand...
File mojo/shell/content_handler_connection.cc (right):

https://codereview.chromium.org/1228743002/diff/20001/mojo/shell/content_hand...
mojo/shell/content_handler_connection.cc:36: return;
On 2015/07/09 17:49:21, Fady Samuel wrote:
> On 2015/07/09 17:27:23, jam wrote:
> > in this case, how does the object get deleted?
> 
> The point of this flag is to ensure that CloseConnection can only delete this
> once.

Sure. But what I mean, if connection_closed_ is true, it means that this object
is deleted. so we have a UAF. or am i missing something?

https://codereview.chromium.org/1228743002/diff/40001/mojo/shell/content_hand...
File mojo/shell/content_handler_connection.cc (right):

https://codereview.chromium.org/1228743002/diff/40001/mojo/shell/content_hand...
mojo/shell/content_handler_connection.cc:24:
manager->ConnectToApplicationInternal(
On 2015/07/09 17:52:47, Fady Samuel wrote:
> Actually, it needs to be friend for this call.

if we don't want to make it public, can we call the public one? what's missing
from that one, just the qualifier? if so can we add that to the public one?

[Fady Samuel, 2015-07-09 18:15:18]

Renamed ConnectToApplicationInternal to ConnectToApplication and updated call
sites. ContentHandlerConnection no longer needs to be a friend of
ApplicationManager.

https://codereview.chromium.org/1228743002/diff/40001/mojo/shell/content_hand...
File mojo/shell/content_handler_connection.cc (right):

https://codereview.chromium.org/1228743002/diff/40001/mojo/shell/content_hand...
mojo/shell/content_handler_connection.cc:24:
manager->ConnectToApplicationInternal(
On 2015/07/09 18:00:18, jam wrote:
> On 2015/07/09 17:52:47, Fady Samuel wrote:
> > Actually, it needs to be friend for this call.
> 
> if we don't want to make it public, can we call the public one? what's missing
> from that one, just the qualifier? if so can we add that to the public one?

Done.

[Fady Samuel, 2015-07-09 18:15:30]

PTAL

[jam, 2015-07-09 19:14:53]

lgtm

[Fady Samuel, 2015-07-09 19:15:09]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2015-07-09 19:16:29]

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

[commit-bot: I haz the power, 2015-07-09 19:30:02]

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

[commit-bot: I haz the power, 2015-07-09 19:30:03]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  cast_shell_android on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_andr...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

(exceeded global retry quota)

[Fady Samuel, 2015-07-09 21:46:12]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2015-07-09 21:46:13]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1228743002/#ps80001
(title: "Fix build")

[commit-bot: I haz the power, 2015-07-09 21:46:36]

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

[commit-bot: I haz the power, 2015-07-09 22:39:55]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-07-09 22:41:50]

Patchset 5 (id:??) landed as
https://crrev.com/09c726e468cf0e5e0c9a2d39888c63561d5838f0
Cr-Commit-Position: refs/heads/master@{#338173}