[Android] Fix GinJavaBridgeMessageFilter registration issue in Java Bridge.

[Android] Fix GinJavaBridgeMessageFilter registration issue in Java Bridge.

With current implementation for Java Bridge, there are 2 user scenarios related
to renderer freeze caused as message filter registration not working as
intended, namely:

A. On web page opened in tab Render process getting exited silently (or manually
killed from adb console) and user opt to reload on same tab so that new sandbox
renderer associates with the opened tab.

B. On web page opened in tab user opens a "chrome://" schema page and navigates
back (this triggers creation of new sandbox renderer for navigation to
"chrome://" and creation of another renderer on navigating back).

This patch addresses both these problems by overriding the RenderProcessGone()
and RenderViewHostChanged() WebContentsObserver interfaces respectively, and
initiates the re-registration and filter object creation if required.

BUG=572053
Committed: https://crrev.com/5678b791281d722542a8a4b64fabe9e97dafff8e
Cr-Commit-Position: refs/heads/master@{#367165}

[Pritam Nikam, 2015-12-24 11:52:26]

Description was changed from

==========
[Android] Fix GinJavaBridgeMessageFilter registration issue in Java Bridge.

With current implementation for Java Bridge, there are 2 user scenarios related
to renderer freeze caused as message filter registration not working as
intended, namely:

A. On web page opened in tab Render process getting exited silently (or manually
killed from adb console) and user opt to reload on same tab so that new sandbox
renderer associates with the opened tab.

B. On web page opened in tab user opens a "chrome://" schema page and navigates
back (this triggers creation of new sandbox renderer for navigation to
"chrome://" and creation of another renderer on navigating back).

This patch addresses both these problems by overriding the RenderProcessGone()
and RenderViewHostChanged() WebContentsObserver interfaces respectively, and
initiates the re-registration and filter object creation if required.

BUG=572053
==========

to

==========
[Android] Fix GinJavaBridgeMessageFilter registration issue in Java Bridge.

With current implementation for Java Bridge, there are 2 user scenarios related
to renderer freeze caused as message filter registration not working as
intended, namely:

A. On web page opened in tab Render process getting exited silently (or manually
killed from adb console) and user opt to reload on same tab so that new sandbox
renderer associates with the opened tab.

B. On web page opened in tab user opens a "chrome://" schema page and navigates
back (this triggers creation of new sandbox renderer for navigation to
"chrome://" and creation of another renderer on navigating back).

This patch addresses both these problems by overriding the RenderProcessGone()
and RenderViewHostChanged() WebContentsObserver interfaces respectively, and
initiates the re-registration and filter object creation if required.

BUG=572053
R=SamsungPeerReview
==========

[Pritam Nikam, 2015-12-24 11:52:26]

pritam.nikam@samsung.com changed reviewers:
+ vivek.vg@samsung.com, vivekg@chromium.org

[Pritam Nikam, 2015-12-24 14:29:17]

Hi Vivek,
Ptal, thanks!

[mnaganov (inactive), 2015-12-28 19:00:17]

mnaganov@chromium.org changed reviewers:
+ mnaganov@chromium.org

[mnaganov (inactive), 2015-12-28 19:00:17]

Ideally, it would also be great if you could come up with unit tests for these
situations.

https://codereview.chromium.org/1547043002/diff/1/content/browser/android/jav...
File content/browser/android/java/gin_java_bridge_dispatcher_host.cc (right):

https://codereview.chromium.org/1547043002/diff/1/content/browser/android/jav...
content/browser/android/java/gin_java_bridge_dispatcher_host.cc:84:
scoped_refptr<GinJavaBridgeMessageFilter> filter =
A simpler approach is to remove the user data from the associated
RenderProcessHost. This way a new filter will be created and added for the new
channel automatically when needed.

[Pritam Nikam, 2015-12-29 11:22:01]

Description was changed from

==========
[Android] Fix GinJavaBridgeMessageFilter registration issue in Java Bridge.

With current implementation for Java Bridge, there are 2 user scenarios related
to renderer freeze caused as message filter registration not working as
intended, namely:

A. On web page opened in tab Render process getting exited silently (or manually
killed from adb console) and user opt to reload on same tab so that new sandbox
renderer associates with the opened tab.

B. On web page opened in tab user opens a "chrome://" schema page and navigates
back (this triggers creation of new sandbox renderer for navigation to
"chrome://" and creation of another renderer on navigating back).

This patch addresses both these problems by overriding the RenderProcessGone()
and RenderViewHostChanged() WebContentsObserver interfaces respectively, and
initiates the re-registration and filter object creation if required.

BUG=572053
R=SamsungPeerReview
==========

to

==========
[Android] Fix GinJavaBridgeMessageFilter registration issue in Java Bridge.

With current implementation for Java Bridge, there are 2 user scenarios related
to renderer freeze caused as message filter registration not working as
intended, namely:

A. On web page opened in tab Render process getting exited silently (or manually
killed from adb console) and user opt to reload on same tab so that new sandbox
renderer associates with the opened tab.

B. On web page opened in tab user opens a "chrome://" schema page and navigates
back (this triggers creation of new sandbox renderer for navigation to
"chrome://" and creation of another renderer on navigating back).

This patch addresses both these problems by overriding the RenderProcessGone()
and RenderViewHostChanged() WebContentsObserver interfaces respectively, and
initiates the re-registration and filter object creation if required.

BUG=572053
==========

[Pritam Nikam, 2015-12-29 11:24:46]

Thanks mnaganov for review inputs. I've addressed it in new patch, PTAL.

I'll try to come up with unit-tests.
Thanks!

https://codereview.chromium.org/1547043002/diff/1/content/browser/android/jav...
File content/browser/android/java/gin_java_bridge_dispatcher_host.cc (right):

https://codereview.chromium.org/1547043002/diff/1/content/browser/android/jav...
content/browser/android/java/gin_java_bridge_dispatcher_host.cc:84:
scoped_refptr<GinJavaBridgeMessageFilter> filter =
On 2015/12/28 19:00:17, mnaganov wrote:
> A simpler approach is to remove the user data from the associated
> RenderProcessHost. This way a new filter will be created and added for the new
> channel automatically when needed.

Done.

[mnaganov (inactive), 2015-12-29 16:55:28]

Great job! Please address the comments for the new code.

https://codereview.chromium.org/1547043002/diff/20001/content/browser/android...
File content/browser/android/java/gin_java_bridge_message_filter.cc (right):

https://codereview.chromium.org/1547043002/diff/20001/content/browser/android...
content/browser/android/java/gin_java_bridge_message_filter.cc:173:
DCHECK(host);
DCHECK is not needed here, filter's only client is GinJavaBridgeDispatcherHost,
and we trust it.

https://codereview.chromium.org/1547043002/diff/20001/content/browser/android...
content/browser/android/java/gin_java_bridge_message_filter.cc:175: DCHECK(rph);
This DCHECK is also not needed because would `rph` happen to be nullptr, it will
be obvious from a crash dump when we try to access it on the next line.

https://codereview.chromium.org/1547043002/diff/20001/content/browser/android...
File content/browser/android/java/gin_java_bridge_message_filter.h (right):

https://codereview.chromium.org/1547043002/diff/20001/content/browser/android...
content/browser/android/java/gin_java_bridge_message_filter.h:48: // Removes the
user data for the |host| from the associated RenderProcessHost.
Please change the comment to: "Removes the filter, which triggers its deletion.
Needs to be called when the corresponding RenderProcessHost cleans itself up,
e.g. on renderer process exit."

Your version contains too many unnecessary details :)

https://codereview.chromium.org/1547043002/diff/20001/content/browser/android...
content/browser/android/java/gin_java_bridge_message_filter.h:54: void
RemoveUserData(const GinJavaBridgeDispatcherHost* host);
I would prefer to rename `RemoveUserData` into `RemoveFilter` and make it
static. This way, you will not need to retrieve the filter first in
`RenderProcessGone`. It's perfectly OK to call
`SupportsUserData::RemoveUserData` even if the map contains no corresponding
key.

https://codereview.chromium.org/1547043002/diff/20001/content/browser/android...
content/browser/android/java/gin_java_bridge_message_filter.h:54: void
RemoveUserData(const GinJavaBridgeDispatcherHost* host);
Please remove `const` for consistency with the rest of the methods.

[Pritam Nikam, 2015-12-30 12:05:36]

Thanks mnaganov!
I've incorporated review inputs in new patch set, ptal.
Thanks!

https://codereview.chromium.org/1547043002/diff/20001/content/browser/android...
File content/browser/android/java/gin_java_bridge_message_filter.cc (right):

https://codereview.chromium.org/1547043002/diff/20001/content/browser/android...
content/browser/android/java/gin_java_bridge_message_filter.cc:173:
DCHECK(host);
On 2015/12/29 16:55:28, mnaganov wrote:
> DCHECK is not needed here, filter's only client is
GinJavaBridgeDispatcherHost,
> and we trust it.

Done.

https://codereview.chromium.org/1547043002/diff/20001/content/browser/android...
content/browser/android/java/gin_java_bridge_message_filter.cc:175: DCHECK(rph);
On 2015/12/29 16:55:28, mnaganov wrote:
> This DCHECK is also not needed because would `rph` happen to be nullptr, it
will
> be obvious from a crash dump when we try to access it on the next line.

Done.

https://codereview.chromium.org/1547043002/diff/20001/content/browser/android...
File content/browser/android/java/gin_java_bridge_message_filter.h (right):

https://codereview.chromium.org/1547043002/diff/20001/content/browser/android...
content/browser/android/java/gin_java_bridge_message_filter.h:48: // Removes the
user data for the |host| from the associated RenderProcessHost.
On 2015/12/29 16:55:28, mnaganov wrote:
> Please change the comment to: "Removes the filter, which triggers its
deletion.
> Needs to be called when the corresponding RenderProcessHost cleans itself up,
> e.g. on renderer process exit."
> 
> Your version contains too many unnecessary details :)

Done.

https://codereview.chromium.org/1547043002/diff/20001/content/browser/android...
content/browser/android/java/gin_java_bridge_message_filter.h:54: void
RemoveUserData(const GinJavaBridgeDispatcherHost* host);
On 2015/12/29 16:55:28, mnaganov wrote:
> Please remove `const` for consistency with the rest of the methods.

Done.

https://codereview.chromium.org/1547043002/diff/20001/content/browser/android...
content/browser/android/java/gin_java_bridge_message_filter.h:54: void
RemoveUserData(const GinJavaBridgeDispatcherHost* host);
On 2015/12/29 16:55:28, mnaganov wrote:
> I would prefer to rename `RemoveUserData` into `RemoveFilter` and make it
> static. This way, you will not need to retrieve the filter first in
> `RenderProcessGone`. It's perfectly OK to call
> `SupportsUserData::RemoveUserData` even if the map contains no corresponding
> key.

Done.

[mnaganov (inactive), 2015-12-30 16:27:48]

LGTM, thanks!

[Pritam Nikam, 2015-12-30 17:32:31]

The CQ bit was checked by pritam.nikam@samsung.com

[commit-bot: I haz the power, 2015-12-30 17:33:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1547043002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1547043002/40001

[Pritam Nikam, 2015-12-30 17:41:09]

On 2015/12/30 16:27:48, mnaganov  wrote:
> LGTM, thanks!

Thanks mnaganov  :-)
I'll go ahead with commit for now. Unit tests will try to address in follow up
cl. 
Cheers!

[commit-bot: I haz the power, 2015-12-30 18:36:25]

Description was changed from

==========
[Android] Fix GinJavaBridgeMessageFilter registration issue in Java Bridge.

With current implementation for Java Bridge, there are 2 user scenarios related
to renderer freeze caused as message filter registration not working as
intended, namely:

A. On web page opened in tab Render process getting exited silently (or manually
killed from adb console) and user opt to reload on same tab so that new sandbox
renderer associates with the opened tab.

B. On web page opened in tab user opens a "chrome://" schema page and navigates
back (this triggers creation of new sandbox renderer for navigation to
"chrome://" and creation of another renderer on navigating back).

This patch addresses both these problems by overriding the RenderProcessGone()
and RenderViewHostChanged() WebContentsObserver interfaces respectively, and
initiates the re-registration and filter object creation if required.

BUG=572053
==========

to

==========
[Android] Fix GinJavaBridgeMessageFilter registration issue in Java Bridge.

With current implementation for Java Bridge, there are 2 user scenarios related
to renderer freeze caused as message filter registration not working as
intended, namely:

A. On web page opened in tab Render process getting exited silently (or manually
killed from adb console) and user opt to reload on same tab so that new sandbox
renderer associates with the opened tab.

B. On web page opened in tab user opens a "chrome://" schema page and navigates
back (this triggers creation of new sandbox renderer for navigation to
"chrome://" and creation of another renderer on navigating back).

This patch addresses both these problems by overriding the RenderProcessGone()
and RenderViewHostChanged() WebContentsObserver interfaces respectively, and
initiates the re-registration and filter object creation if required.

BUG=572053
==========

[commit-bot: I haz the power, 2015-12-30 18:36:26]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-12-30 18:37:17]

Description was changed from

==========
[Android] Fix GinJavaBridgeMessageFilter registration issue in Java Bridge.

With current implementation for Java Bridge, there are 2 user scenarios related
to renderer freeze caused as message filter registration not working as
intended, namely:

A. On web page opened in tab Render process getting exited silently (or manually
killed from adb console) and user opt to reload on same tab so that new sandbox
renderer associates with the opened tab.

B. On web page opened in tab user opens a "chrome://" schema page and navigates
back (this triggers creation of new sandbox renderer for navigation to
"chrome://" and creation of another renderer on navigating back).

This patch addresses both these problems by overriding the RenderProcessGone()
and RenderViewHostChanged() WebContentsObserver interfaces respectively, and
initiates the re-registration and filter object creation if required.

BUG=572053
==========

to

==========
[Android] Fix GinJavaBridgeMessageFilter registration issue in Java Bridge.

With current implementation for Java Bridge, there are 2 user scenarios related
to renderer freeze caused as message filter registration not working as
intended, namely:

A. On web page opened in tab Render process getting exited silently (or manually
killed from adb console) and user opt to reload on same tab so that new sandbox
renderer associates with the opened tab.

B. On web page opened in tab user opens a "chrome://" schema page and navigates
back (this triggers creation of new sandbox renderer for navigation to
"chrome://" and creation of another renderer on navigating back).

This patch addresses both these problems by overriding the RenderProcessGone()
and RenderViewHostChanged() WebContentsObserver interfaces respectively, and
initiates the re-registration and filter object creation if required.

BUG=572053

Committed: https://crrev.com/5678b791281d722542a8a4b64fabe9e97dafff8e
Cr-Commit-Position: refs/heads/master@{#367165}
==========

[commit-bot: I haz the power, 2015-12-30 18:37:18]

Patchset 3 (id:??) landed as
https://crrev.com/5678b791281d722542a8a4b64fabe9e97dafff8e
Cr-Commit-Position: refs/heads/master@{#367165}