Move some calls from the Android UI thread to the Chrome one

Move some calls from the Android UI thread to the Chrome one

Some callbacks (from AsyncTasks, broadcasts reciever and other
sources) are received on the Android UI thread but the code
expects to run on the Chrome UI thread.

BUG=568602
TBR=atwilson@chromium.org

Committed: https://crrev.com/172f720710be2008df620438736d19224f0ebcc4
Cr-Commit-Position: refs/heads/master@{#364476}

[dgn, 2015-12-10 15:49:38]

Description was changed from

==========
Direct the PolicyProvider broadcasts to the Chrome UI thread

Broadcasts recieved on a different thread were causing native
crashes in WebView

BUG=568602
==========

to

==========
Direct the PolicyProvider broadcasts to the Chrome UI thread

Broadcasts recieved on a different thread were causing native
crashes in WebView

BUG=568602
TBR=atwilson@chromium.org
==========

[dgn, 2015-12-10 16:54:47]

Description was changed from

==========
Direct the PolicyProvider broadcasts to the Chrome UI thread

Broadcasts recieved on a different thread were causing native
crashes in WebView

BUG=568602
TBR=atwilson@chromium.org
==========

to

==========
Move some calls from the Android UI thread to the Chrome one

Some callbacks (from AsyncTasks, broadcasts reciever and other
sources) are received on the Android UI thread but the code
expects to run on the Chrome UI thread.

BUG=568602
TBR=atwilson@chromium.org
==========

[dgn, 2015-12-10 16:55:10]

dgn@chromium.org changed reviewers:
+ atwilson@chromium.org, torne@chromium.org

[dgn, 2015-12-10 16:55:10]

PTAL

[Torne, 2015-12-10 17:12:57]

torne@chromium.org changed reviewers:
+ sgurun@chromium.org

[Torne, 2015-12-10 17:12:57]

+sgurun who might be familiar with some of the code being changed here.

[Torne, 2015-12-10 17:55:21]

This looks pretty reasonable for now. Longer-term, we should probably consider
implementing a Chromium-specific form of AsyncTask that works using ThreadUtils
primitives and uses the Chromium UI thread for you (and also probably things
like: creating a chromium-specific background thread pool instead of using the
deafult one, etc).

LGTM, but I want to see if Selim has any thoughts.

[sgurun-gerrit only, 2015-12-10 17:56:06]

On 2015/12/10 17:55:21, Torne wrote:
> This looks pretty reasonable for now. Longer-term, we should probably consider
> implementing a Chromium-specific form of AsyncTask that works using
ThreadUtils
> primitives and uses the Chromium UI thread for you (and also probably things
> like: creating a chromium-specific background thread pool instead of using the
> deafult one, etc).
> 
> LGTM, but I want to see if Selim has any thoughts.

looking...

[sgurun-gerrit only, 2015-12-10 18:05:10]

On 2015/12/10 17:56:06, sgurun wrote:
> On 2015/12/10 17:55:21, Torne wrote:
> > This looks pretty reasonable for now. Longer-term, we should probably
consider
> > implementing a Chromium-specific form of AsyncTask that works using
> ThreadUtils
> > primitives and uses the Chromium UI thread for you (and also probably things
> > like: creating a chromium-specific background thread pool instead of using
the
> > deafult one, etc).
> > 
> > LGTM, but I want to see if Selim has any thoughts.
> 
> looking...

note that you have a clang failure:
Bugs reported the following issues:
URF_UNREAD_FIELD: Unread field
  In class org.chromium.policy.AbstractAppRestrictionsProvider
  Field org.chromium.policy.AbstractAppRestrictionsProvider.mExecutor
  At AbstractAppRestrictionsProvider.java:[line 50]

[sgurun-gerrit only, 2015-12-10 18:09:50]

lgtm

https://codereview.chromium.org/1514903002/diff/40001/components/policy/andro...
File
components/policy/android/java/src/org/chromium/policy/AbstractAppRestrictionsProvider.java
(right):

https://codereview.chromium.org/1514903002/diff/40001/components/policy/andro...
components/policy/android/java/src/org/chromium/policy/AbstractAppRestrictionsProvider.java:83:
new Handler(ThreadUtils.getUiThreadLooper()));
that would probably be the only change to fix the crash, no?

[sgurun-gerrit only, 2015-12-10 18:10:02]

lgtm

[Torne, 2015-12-10 19:00:47]

On 2015/12/10 18:09:50, sgurun wrote:
> lgtm
> 
>
https://codereview.chromium.org/1514903002/diff/40001/components/policy/andro...
> File
>
components/policy/android/java/src/org/chromium/policy/AbstractAppRestrictionsProvider.java
> (right):
> 
>
https://codereview.chromium.org/1514903002/diff/40001/components/policy/andro...
>
components/policy/android/java/src/org/chromium/policy/AbstractAppRestrictionsProvider.java:83:
> new Handler(ThreadUtils.getUiThreadLooper()));
> that would probably be the only change to fix the crash, no?

Apparently the other changes are also required as the crash just moves on to
another cause when the first one is fixed. :/

[dgn, 2015-12-10 19:52:49]

https://codereview.chromium.org/1514903002/diff/40001/components/policy/andro...
File
components/policy/android/java/src/org/chromium/policy/AbstractAppRestrictionsProvider.java
(right):

https://codereview.chromium.org/1514903002/diff/40001/components/policy/andro...
components/policy/android/java/src/org/chromium/policy/AbstractAppRestrictionsProvider.java:83:
new Handler(ThreadUtils.getUiThreadLooper()));
On 2015/12/10 18:09:50, sgurun wrote:
> that would probably be the only change to fix the crash, no? 

As torne said, there were other crashes, both on startup and shutdown. Hopefully
I got all of them

[dgn, 2015-12-10 19:52:53]

Thanks

[dgn, 2015-12-10 19:54:02]

The CQ bit was checked by dgn@chromium.org

[dgn, 2015-12-10 19:54:02]

The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org,
torne@chromium.org
Link to the patchset: https://codereview.chromium.org/1514903002/#ps60001
(title: "Pass findbug")

[commit-bot: I haz the power, 2015-12-10 19:56:19]

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

[commit-bot: I haz the power, 2015-12-10 21:10:31]

Description was changed from

==========
Move some calls from the Android UI thread to the Chrome one

Some callbacks (from AsyncTasks, broadcasts reciever and other
sources) are received on the Android UI thread but the code
expects to run on the Chrome UI thread.

BUG=568602
TBR=atwilson@chromium.org
==========

to

==========
Move some calls from the Android UI thread to the Chrome one

Some callbacks (from AsyncTasks, broadcasts reciever and other
sources) are received on the Android UI thread but the code
expects to run on the Chrome UI thread.

BUG=568602
TBR=atwilson@chromium.org
==========

[commit-bot: I haz the power, 2015-12-10 21:10:32]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-10 21:12:20]

Description was changed from

==========
Move some calls from the Android UI thread to the Chrome one

Some callbacks (from AsyncTasks, broadcasts reciever and other
sources) are received on the Android UI thread but the code
expects to run on the Chrome UI thread.

BUG=568602
TBR=atwilson@chromium.org
==========

to

==========
Move some calls from the Android UI thread to the Chrome one

Some callbacks (from AsyncTasks, broadcasts reciever and other
sources) are received on the Android UI thread but the code
expects to run on the Chrome UI thread.

BUG=568602
TBR=atwilson@chromium.org

Committed: https://crrev.com/172f720710be2008df620438736d19224f0ebcc4
Cr-Commit-Position: refs/heads/master@{#364476}
==========

[commit-bot: I haz the power, 2015-12-10 21:12:22]

Patchset 4 (id:??) landed as
https://crrev.com/172f720710be2008df620438736d19224f0ebcc4
Cr-Commit-Position: refs/heads/master@{#364476}

[boliu, 2015-12-10 22:03:14]

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

[boliu, 2015-12-10 22:03:14]

ahh non-main-UI thread fun..

https://codereview.chromium.org/1514903002/diff/60001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/1514903002/diff/60001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwContents.java:643:
ThreadUtils.runOnUiThread(new Runnable() {
I think new code is not wrong, but I kinda want to revert back to old code here:

Webview relies the the trim call to be delivered synchronously. That's why it's
not using MemoryPressureListener here. So new code is not broken since
runOnuiThread is also synchronous if this is the UI thread. But assuming
runOnuiThread behavior is bad imo.

So there are a few cases:
1) all webviews used on a single non-main thread, has activity context, then the
whole activity lives on that thread, and all the trim signals also gets
delivered on that thread, so both new and old code are fine
2) all webview used on a single non-main thread, has global application context,
then I guess new code is more correct then old code, but then every access to
app context is not thread safe already, and this is only one of them, so
probably not worth worrying too much (app context affects case 1 too, but let's
not think too hard on that..)
3) webviews used from multiple threads, and then gets merged to a single
chromium UI thread (context doesn't matter), I don't ever see this scenario
being thread safe, since chromium will be access all the view tree on its UI
thread, so again not worth worrying about this case imo

Maybe we should test 1) and crash hard on 2) and 3)?

Other webview owners have thoughts here?