Do not start RunLoop of MessagePumpForUI in NestedMessagePumpAndroid

Do not start RunLoop of MessagePumpForUI in NestedMessagePumpAndroid

MessagePumpForUI starts one RunLoop in Start(), and so all other RunLoops
attached to NestedMessagePumpAndroid become nested to the first one.
This CL modifies NestedMessagePumpAndroid not to call methods of
MessagePumpForUI including Start().
And a quit request to finish the first RunLoop of MessagePumpForUI
will be ignored when it wasn't started.

BUG=432737
Committed: https://crrev.com/617edc52980c48dc46673feda23a6a40401e5dda
Cr-Commit-Position: refs/heads/master@{#304709}

[Jaekyun Seok (inactive), 2014-11-17 04:21:14]

jaekyun@chromium.org changed reviewers:
+ thakis@chromium.org, thestig@chromium.org, tommi@chromium.org,
xhwang@chromium.org

[Jaekyun Seok (inactive), 2014-11-17 04:21:15]

Please review this CL to fix a bug in content_browsertests on android platform.

[ncarter (slow), 2014-11-17 18:47:00]

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

[ncarter (slow), 2014-11-17 18:47:01]

https://codereview.chromium.org/733533002/diff/40001/content/browser/media/cd...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/733533002/diff/40001/content/browser/media/cd...
content/browser/media/cdm/browser_cdm_manager.cc:85: // the entry. For example,
see FrameTreeBrowserTest.FrameTreeAfterCrash test.
Link to http://crbug.com/430251 here, seems to be the same issue.

[ncarter (slow), 2014-11-17 19:08:03]

Could you add the browsertest I posted in the bug report http://crbug.com/432737
? Does it pass now?

A good place for it might be in ContentBrowserTestSanityTest -- and you would
probably want to convert the globals to member variables.
https://code.google.com/p/chromium/codesearch#chromium/src/content/test/conte...

[Lei Zhang, 2014-11-17 21:57:38]

thestig@chromium.org changed reviewers:
+ bulach@chromium.org

[Lei Zhang, 2014-11-17 21:57:38]

+bulach who has actually touched message_pump_android.cc.

[bulach, 2014-11-17 22:08:42]

bulach@chromium.org changed reviewers:
+ skyostil@chromium.org

[bulach, 2014-11-17 22:08:42]

sorry, I'm no longer actively working in chromium, adding skyostil and starodub
as my replacements in this area..

regardless, one comment below :)

https://codereview.chromium.org/733533002/diff/40001/content/public/test/nest...
File content/public/test/nested_message_pump_android.cc (right):

https://codereview.chromium.org/733533002/diff/40001/content/public/test/nest...
content/public/test/nested_message_pump_android.cc:129: if (state_) {
it looks like redundant_quit_executed_ could be an internal detail here rather
than an API, no?

[Jaekyun Seok (inactive), 2014-11-18 03:10:07]

PTAL.

I added ncarter's test and modified codes to ignore the quit request in
browser_main_runner instead of adding a new
API.

https://codereview.chromium.org/733533002/diff/40001/content/browser/media/cd...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/733533002/diff/40001/content/browser/media/cd...
content/browser/media/cdm/browser_cdm_manager.cc:85: // the entry. For example,
see FrameTreeBrowserTest.FrameTreeAfterCrash test.
On 2014/11/17 18:47:01, ncarter wrote:
> Link to http://crbug.com/430251 here, seems to be the same issue.

Done.

https://codereview.chromium.org/733533002/diff/40001/content/public/test/nest...
File content/public/test/nested_message_pump_android.cc (right):

https://codereview.chromium.org/733533002/diff/40001/content/public/test/nest...
content/public/test/nested_message_pump_android.cc:129: if (state_) {
On 2014/11/17 22:08:42, bulach wrote:
> it looks like redundant_quit_executed_ could be an internal detail here rather
> than an API, no?

I will ignore the quit request in browser_main_runner instead of adding this new
API.

[Jaekyun Seok (inactive), 2014-11-18 03:13:38]

jaekyun@chromium.org changed reviewers:
+ creis@chromium.org

[Jaekyun Seok (inactive), 2014-11-18 03:13:39]

Hi Charlie,

Please review this CL to fix a bug in content_browsertests on android platform;
especially changes on content/browser/browser_main_runner.cc .

Thanks,

[Sami, 2014-11-18 12:02:25]

https://codereview.chromium.org/733533002/diff/60001/content/browser/media/cd...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/733533002/diff/60001/content/browser/media/cd...
content/browser/media/cdm/browser_cdm_manager.cc:87: if
(g_browser_cdm_manager_map.Get()[render_process_id_] == this)
Should this change be in a different patch? It seems unrelated.

[ncarter (slow), 2014-11-18 18:02:53]

This new version looks much better. lgtm

https://codereview.chromium.org/733533002/diff/60001/content/browser/media/cd...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/733533002/diff/60001/content/browser/media/cd...
content/browser/media/cdm/browser_cdm_manager.cc:87: if
(g_browser_cdm_manager_map.Get()[render_process_id_] == this)
On 2014/11/18 12:02:25, Sami wrote:
> Should this change be in a different patch? It seems unrelated.

I believe the fix (which runs some DeleteSoons that were previously dropped)
exposes a new case of bug 430251, hence this workaround.

https://codereview.chromium.org/733533002/diff/60001/content/test/content_bro...
File content/test/content_browser_test_test.cc (right):

https://codereview.chromium.org/733533002/diff/60001/content/test/content_bro...
content/test/content_browser_test_test.cc:51: void CallbackChecker(bool*
non_nested_task_ran) {
This should be in an anonymous namespace {}, to prevent link-time collisions
with a similarly named function in some other compilation unit.

[Charlie Reis, 2014-11-18 18:23:30]

LGTM with Nick's suggestions.

[Jaekyun Seok (inactive), 2014-11-18 22:38:23]

https://codereview.chromium.org/733533002/diff/60001/content/browser/media/cd...
File content/browser/media/cdm/browser_cdm_manager.cc (right):

https://codereview.chromium.org/733533002/diff/60001/content/browser/media/cd...
content/browser/media/cdm/browser_cdm_manager.cc:87: if
(g_browser_cdm_manager_map.Get()[render_process_id_] == this)
On 2014/11/18 18:02:53, ncarter wrote:
> On 2014/11/18 12:02:25, Sami wrote:
> > Should this change be in a different patch? It seems unrelated.
> 
> I believe the fix (which runs some DeleteSoons that were previously dropped)
> exposes a new case of bug 430251, hence this workaround.

Right, this fix exposes the same issue to bug 430251. So I made this change.

https://codereview.chromium.org/733533002/diff/60001/content/test/content_bro...
File content/test/content_browser_test_test.cc (right):

https://codereview.chromium.org/733533002/diff/60001/content/test/content_bro...
content/test/content_browser_test_test.cc:51: void CallbackChecker(bool*
non_nested_task_ran) {
On 2014/11/18 18:02:53, ncarter wrote:
> This should be in an anonymous namespace {}, to prevent link-time collisions
> with a similarly named function in some other compilation unit.

Done.

[Jaekyun Seok (inactive), 2014-11-18 22:38:52]

The CQ bit was checked by jaekyun@chromium.org

[commit-bot: I haz the power, 2014-11-18 22:40:55]

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

[commit-bot: I haz the power, 2014-11-19 00:11:15]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-11-19 00:12:49]

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