Prevent thread assertion for HTC mail apk

Prevent thread assertion for HTC mail apk

HTC's mail apk calls evaluateJavaScript() inside
InputConnection#setComposingText(). This is problematic with ImeThread
feature since InputConnection methods are now running on its own thread
(IME thread). This causes a thread check assertion in Android's framework
(android.webkit.WebView).

As a workaround, we set sEnforceThreadChecking to false immediately after
it was set in WebView's constructor.

HTC is actively fixing their apk to avoid this crash, but as a last resort,
this workaround will help avoid such a crash.

BUG=636717, 622151
Committed: https://crrev.com/f99c4ade4b10c24a59b7bf34de4d7509e7ca8708
Cr-Commit-Position: refs/heads/master@{#414717}

[Changwan Ryu, 2016-08-12 05:45:04]

The CQ bit was checked by changwan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 05:45:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Changwan Ryu, 2016-08-12 06:24:44]

changwan@chromium.org changed reviewers:
+ aelias@chromium.org, torne@chromium.org

[Changwan Ryu, 2016-08-12 06:24:44]

This is a preliminary version that does not check for version code. Could you
take an initial look and see if this is going in the right direction? Thanks.

[Changwan Ryu, 2016-08-12 06:52:07]

The CQ bit was checked by changwan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 06:52:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Changwan Ryu, 2016-08-12 06:53:24]

On 2016/08/12 06:24:44, Changwan Ryu wrote:
> This is a preliminary version that does not check for version code. Could you
> take an initial look and see if this is going in the right direction? Thanks.

Updated with correct values HTC provided.

[commit-bot: I haz the power, 2016-08-12 07:25:15]

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

[commit-bot: I haz the power, 2016-08-12 07:25:16]

Dry run: This issue passed the CQ dry run.

[aelias_OOO_until_Jul13, 2016-08-12 07:49:57]

https://codereview.chromium.org/2243753002/diff/40001/android_webview/glue/ja...
File
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
(right):

https://codereview.chromium.org/2243753002/diff/40001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:108:
private static volatile AtomicBoolean sShouldAvoidThreadCheckingForHtcMail;
Reading
https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/atomic/package...,
it seems like the recommended way to use atomics is:

private final AtomicBoolean sIsHtcMail = new AtomicBoolean(false);

https://codereview.chromium.org/2243753002/diff/40001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:143:
avoidThreadCheckingForHtcMail(mWebView);
Please restructure this to look like:

sIsHtcMail.set(isHtcMail(mWebView));
if (isHtcMail.get()) disableThreadChecking(mWebView);

https://codereview.chromium.org/2243753002/diff/40001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:207:
private boolean shouldAvoidThreadCheckingForHtcMail(View view) {
Please rename this to isHtcMail and delete all lines in the method that touch
the atomic.

https://codereview.chromium.org/2243753002/diff/40001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:239:
private void avoidThreadCheckingForHtcMail(View view) {
Please rename to disableThreadChecking and remove the first line.

https://codereview.chromium.org/2243753002/diff/40001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:252:
Log.w(TAG, "We are turning off thread check for this apk.");
This log is a bit low on detail, I'd suggest printing out the detected APK name
and version code and link to http://crbug.com/622151.  Maybe you can put the log
next to the place detecting the APK instead (I don't think this reflection
failing is a real concern).

https://codereview.chromium.org/2243753002/diff/40001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:606:
if (shouldAvoidThreadCheckingForHtcMail(mWebView)) {
Please check sIsHtcMail.get() directly here.

[Changwan Ryu, 2016-08-12 08:40:06]

The CQ bit was checked by changwan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 08:40:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Changwan Ryu, 2016-08-12 08:40:30]

Thanks for the careful review. Please take another look.

https://codereview.chromium.org/2243753002/diff/40001/android_webview/glue/ja...
File
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
(right):

https://codereview.chromium.org/2243753002/diff/40001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:108:
private static volatile AtomicBoolean sShouldAvoidThreadCheckingForHtcMail;
On 2016/08/12 07:49:57, aelias wrote:
> Reading
>
https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/atomic/package...,
> it seems like the recommended way to use atomics is:
> 
> private final AtomicBoolean sIsHtcMail = new AtomicBoolean(false);

Done.

https://codereview.chromium.org/2243753002/diff/40001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:143:
avoidThreadCheckingForHtcMail(mWebView);
On 2016/08/12 07:49:57, aelias wrote:
> Please restructure this to look like:
> 
> sIsHtcMail.set(isHtcMail(mWebView));
> if (isHtcMail.get()) disableThreadChecking(mWebView);

Done.

https://codereview.chromium.org/2243753002/diff/40001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:207:
private boolean shouldAvoidThreadCheckingForHtcMail(View view) {
On 2016/08/12 07:49:57, aelias wrote:
> Please rename this to isHtcMail and delete all lines in the method that touch
> the atomic.

Done.

https://codereview.chromium.org/2243753002/diff/40001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:239:
private void avoidThreadCheckingForHtcMail(View view) {
On 2016/08/12 07:49:57, aelias wrote:
> Please rename to disableThreadChecking and remove the first line.

Done.

https://codereview.chromium.org/2243753002/diff/40001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:252:
Log.w(TAG, "We are turning off thread check for this apk.");
On 2016/08/12 07:49:57, aelias wrote:
> This log is a bit low on detail, I'd suggest printing out the detected APK
name
> and version code and link to http://crbug.com/622151.  Maybe you can put the
log
> next to the place detecting the APK instead (I don't think this reflection
> failing is a real concern).

Added log in isHtcMail() and removed this one.

https://codereview.chromium.org/2243753002/diff/40001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:606:
if (shouldAvoidThreadCheckingForHtcMail(mWebView)) {
On 2016/08/12 07:49:57, aelias wrote:
> Please check sIsHtcMail.get() directly here.

Done.

[commit-bot: I haz the power, 2016-08-12 09:12:16]

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

[commit-bot: I haz the power, 2016-08-12 09:12:17]

Dry run: This issue passed the CQ dry run.

[Torne, 2016-08-12 09:24:23]

https://codereview.chromium.org/2243753002/diff/60001/android_webview/glue/ja...
File
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
(right):

https://codereview.chromium.org/2243753002/diff/60001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:108:
private static volatile AtomicBoolean sIsHtcMail = new AtomicBoolean(false);
For future reference, don't use volatile for this - make it final, but per my
comments later there's no need for atomics (or a boolean) at all. :)

https://codereview.chromium.org/2243753002/diff/60001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:143:
sIsHtcMail.set(isHtcMail(mWebView));
There's no need to do this in WebViewChromium and have to keep testing the
package and so on - it's much easier to just do this in
WebViewChromiumFactoryProvider.initialize() which will execute only once.

https://codereview.chromium.org/2243753002/diff/60001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:211:
if (view.getClass().getName() == null
Instead of checking the class name of a view, just call
Context.getPackageName().

https://codereview.chromium.org/2243753002/diff/60001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:229:
e.printStackTrace();
Printing a stack trace and throwing the exception away is never really useful.
Catch more specific exception types here, and just swallow them silently; if the
package manager can't tell us the version of the mail app then we're not in a
state where enabling this workaround is going to help us :)

https://codereview.chromium.org/2243753002/diff/60001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:238:
for (Class<?> webViewClass = view.getClass();
No need to search the class hierarchy here (and this is fragile, since it's only
doing a substring match): just use Class.forName("android.webkit.WebView"). It's
also a static field, so it's sufficient to do this once. So, it can just be done
immediately after the check in WebViewChromiumFactoryProvider.initialize()

https://codereview.chromium.org/2243753002/diff/60001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:250:
e.printStackTrace();
If we failed here, probably just print a single-line error saying we couldn't
turn it off. The exception is unlikely to be interesting.

https://codereview.chromium.org/2243753002/diff/60001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:603:
ThreadUtils.postOnUiThread(new Runnable() {
Use checkNeedsPost() instead of doing it unconditionally, and just call
mAwContents.evaluateJavaScript directly instead of introducing the extra method.

[Changwan Ryu, 2016-08-12 11:14:06]

https://codereview.chromium.org/2243753002/diff/60001/android_webview/glue/ja...
File
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
(right):

https://codereview.chromium.org/2243753002/diff/60001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:108:
private static volatile AtomicBoolean sIsHtcMail = new AtomicBoolean(false);
On 2016/08/12 09:24:23, Torne wrote:
> For future reference, don't use volatile for this - make it final, but per my
> comments later there's no need for atomics (or a boolean) at all. :)

Acknowledged.

https://codereview.chromium.org/2243753002/diff/60001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:143:
sIsHtcMail.set(isHtcMail(mWebView));
On 2016/08/12 09:24:23, Torne wrote:
> There's no need to do this in WebViewChromium and have to keep testing the
> package and so on - it's much easier to just do this in
> WebViewChromiumFactoryProvider.initialize() which will execute only once.

Unfortunately, disableThreadChecking should be called every time
WebViewChromium.init() gets called because sEnforceThreadChecking will be set
every time WebView gets instantiated.

Having this logic spread out between WebViewChromium and WebViewChromiumProvider
is probably not a good idea. Isn't it?

https://codereview.chromium.org/2243753002/diff/60001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:211:
if (view.getClass().getName() == null
On 2016/08/12 09:24:23, Torne wrote:
> Instead of checking the class name of a view, just call
> Context.getPackageName().

Done.

https://codereview.chromium.org/2243753002/diff/60001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:229:
e.printStackTrace();
On 2016/08/12 09:24:23, Torne wrote:
> Printing a stack trace and throwing the exception away is never really useful.
> Catch more specific exception types here, and just swallow them silently; if
the
> package manager can't tell us the version of the mail app then we're not in a
> state where enabling this workaround is going to help us :)

Changed not to print stack trace.

https://codereview.chromium.org/2243753002/diff/60001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:238:
for (Class<?> webViewClass = view.getClass();
On 2016/08/12 09:24:23, Torne wrote:
> No need to search the class hierarchy here (and this is fragile, since it's
only
> doing a substring match): just use Class.forName("android.webkit.WebView").
It's
> also a static field, so it's sufficient to do this once. So, it can just be
done
> immediately after the check in WebViewChromiumFactoryProvider.initialize()

Done. BTW, this should be done for each WebView.init() call.

https://codereview.chromium.org/2243753002/diff/60001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:250:
e.printStackTrace();
On 2016/08/12 09:24:23, Torne wrote:
> If we failed here, probably just print a single-line error saying we couldn't
> turn it off. The exception is unlikely to be interesting.

Done.

https://codereview.chromium.org/2243753002/diff/60001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:603:
ThreadUtils.postOnUiThread(new Runnable() {
On 2016/08/12 09:24:23, Torne wrote:
> Use checkNeedsPost() instead of doing it unconditionally, and just call
> mAwContents.evaluateJavaScript directly instead of introducing the extra
method.

Changed to runOnUiThread and removed extra method.

[Torne, 2016-08-12 13:25:41]

https://codereview.chromium.org/2243753002/diff/60001/android_webview/glue/ja...
File
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
(right):

https://codereview.chromium.org/2243753002/diff/60001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:108:
private static volatile AtomicBoolean sIsHtcMail = new AtomicBoolean(false);
On 2016/08/12 11:14:06, Changwan Ryu wrote:
> On 2016/08/12 09:24:23, Torne wrote:
> > For future reference, don't use volatile for this - make it final, but per
my
> > comments later there's no need for atomics (or a boolean) at all. :)
> 
> Acknowledged.

Your new CL still has this as volatile. If we actually need this and aren't
going to remove it entirely, it should be final.

https://codereview.chromium.org/2243753002/diff/60001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:143:
sIsHtcMail.set(isHtcMail(mWebView));
On 2016/08/12 11:14:06, Changwan Ryu wrote:
> On 2016/08/12 09:24:23, Torne wrote:
> > There's no need to do this in WebViewChromium and have to keep testing the
> > package and so on - it's much easier to just do this in
> > WebViewChromiumFactoryProvider.initialize() which will execute only once.
> 
> Unfortunately, disableThreadChecking should be called every time
> WebViewChromium.init() gets called because sEnforceThreadChecking will be set
> every time WebView gets instantiated.

Ah, I didn't realise it was reset in the constructor. That's terrible, and
actually broken in some vanishingly unlikely cases (multiple apps in the same
process with different target sdk levels) :(

> Having this logic spread out between WebViewChromium and
WebViewChromiumProvider
> is probably not a good idea. Isn't it?

It is if it avoids having to do atomics, or recheck package names over and over
redundantly. However, you don't have to spread it out in this case: you could
check whether it's HTC Mail once in global initialisation, and then set
sEnforceThreadChecking back to the right value in
WebViewChromiumFactoryProvider.createWebView.

https://codereview.chromium.org/2243753002/diff/80001/android_webview/glue/ja...
File
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
(right):

https://codereview.chromium.org/2243753002/diff/80001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:209:
if (mAppTargetSdkVersion > Build.VERSION_CODES.M) return false;
I'm not sure that checking the target SDK version is correct throughout this
function, though it's unclear from the information they have provided. We might
need to be checking the build version of the actual OS instead/as well,
depending exactly how their versioning works.

https://codereview.chromium.org/2243753002/diff/80001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:225:
} catch (Exception e) {
Comment or something here to note that we are intentionally swallowing it, and
ideally catch a more specific exception type (the ones the package manager
actually throws).

https://codereview.chromium.org/2243753002/diff/80001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:592:
ThreadUtils.runOnUiThread(new Runnable() {
Every other function here uses checkNeedsPost and mFactory.runOnUiThreadBlocking
or mFactory.addTask as appropriate. Is there a problem with doing the same here?

[Torne, 2016-08-12 13:26:07]

Also can you drop the "(upstream)" from the commit message?

[Changwan Ryu, 2016-08-16 10:30:24]

https://codereview.chromium.org/2243753002/diff/60001/android_webview/glue/ja...
File
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
(right):

https://codereview.chromium.org/2243753002/diff/60001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:108:
private static volatile AtomicBoolean sIsHtcMail = new AtomicBoolean(false);
On 2016/08/12 13:25:41, Torne wrote:
> On 2016/08/12 11:14:06, Changwan Ryu wrote:
> > On 2016/08/12 09:24:23, Torne wrote:
> > > For future reference, don't use volatile for this - make it final, but per
> my
> > > comments later there's no need for atomics (or a boolean) at all. :)
> > 
> > Acknowledged.
> 
> Your new CL still has this as volatile. If we actually need this and aren't
> going to remove it entirely, it should be final.

My bad. Fixed as final.

https://codereview.chromium.org/2243753002/diff/60001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:143:
sIsHtcMail.set(isHtcMail(mWebView));
On 2016/08/12 13:25:41, Torne wrote:
> On 2016/08/12 11:14:06, Changwan Ryu wrote:
> > On 2016/08/12 09:24:23, Torne wrote:
> > > There's no need to do this in WebViewChromium and have to keep testing the
> > > package and so on - it's much easier to just do this in
> > > WebViewChromiumFactoryProvider.initialize() which will execute only once.
> > 
> > Unfortunately, disableThreadChecking should be called every time
> > WebViewChromium.init() gets called because sEnforceThreadChecking will be
set
> > every time WebView gets instantiated.
> 
> Ah, I didn't realise it was reset in the constructor. That's terrible, and
> actually broken in some vanishingly unlikely cases (multiple apps in the same
> process with different target sdk levels) :(
> 
> > Having this logic spread out between WebViewChromium and
> WebViewChromiumProvider
> > is probably not a good idea. Isn't it?
> 
> It is if it avoids having to do atomics, or recheck package names over and
over
> redundantly. However, you don't have to spread it out in this case: you could
> check whether it's HTC Mail once in global initialisation, and then set
> sEnforceThreadChecking back to the right value in
> WebViewChromiumFactoryProvider.createWebView.

Moved the checking logic to factory and replaced atomicboolean by final.
BTW, I assume that sEnforceThreadChecking in HTC mail would be set to false
while other apps should have the right value. Why do we need to set
sEnforceThreadChecking back to the right value?

https://codereview.chromium.org/2243753002/diff/80001/android_webview/glue/ja...
File
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
(right):

https://codereview.chromium.org/2243753002/diff/80001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:209:
if (mAppTargetSdkVersion > Build.VERSION_CODES.M) return false;
On 2016/08/12 13:25:41, Torne wrote:
> I'm not sure that checking the target SDK version is correct throughout this
> function, though it's unclear from the information they have provided. We
might
> need to be checking the build version of the actual OS instead/as well,
> depending exactly how their versioning works.

You're right. Fixed now.

https://codereview.chromium.org/2243753002/diff/80001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:225:
} catch (Exception e) {
On 2016/08/12 13:25:41, Torne wrote:
> Comment or something here to note that we are intentionally swallowing it, and
> ideally catch a more specific exception type (the ones the package manager
> actually throws).

Done.

https://codereview.chromium.org/2243753002/diff/80001/android_webview/glue/ja...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:592:
ThreadUtils.runOnUiThread(new Runnable() {
On 2016/08/12 13:25:41, Torne wrote:
> Every other function here uses checkNeedsPost and
mFactory.runOnUiThreadBlocking
> or mFactory.addTask as appropriate. Is there a problem with doing the same
here?

Done.

[Changwan Ryu, 2016-08-16 10:30:32]

The CQ bit was checked by changwan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 10:30:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Torne, 2016-08-16 11:43:07]

On 2016/08/16 10:30:24, Changwan Ryu wrote:
>
https://codereview.chromium.org/2243753002/diff/60001/android_webview/glue/ja...
>
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:143:
> sIsHtcMail.set(isHtcMail(mWebView));
> On 2016/08/12 13:25:41, Torne wrote:
> > On 2016/08/12 11:14:06, Changwan Ryu wrote:
> > > On 2016/08/12 09:24:23, Torne wrote:
> > > > There's no need to do this in WebViewChromium and have to keep testing
the
> > > > package and so on - it's much easier to just do this in
> > > > WebViewChromiumFactoryProvider.initialize() which will execute only
once.
> > > 
> > > Unfortunately, disableThreadChecking should be called every time
> > > WebViewChromium.init() gets called because sEnforceThreadChecking will be
> set
> > > every time WebView gets instantiated.
> > 
> > Ah, I didn't realise it was reset in the constructor. That's terrible, and
> > actually broken in some vanishingly unlikely cases (multiple apps in the
same
> > process with different target sdk levels) :(
> > 
> > > Having this logic spread out between WebViewChromium and
> > WebViewChromiumProvider
> > > is probably not a good idea. Isn't it?
> > 
> > It is if it avoids having to do atomics, or recheck package names over and
> over
> > redundantly. However, you don't have to spread it out in this case: you
could
> > check whether it's HTC Mail once in global initialisation, and then set
> > sEnforceThreadChecking back to the right value in
> > WebViewChromiumFactoryProvider.createWebView.
> 
> Moved the checking logic to factory and replaced atomicboolean by final.
> BTW, I assume that sEnforceThreadChecking in HTC mail would be set to false
> while other apps should have the right value. Why do we need to set
> sEnforceThreadChecking back to the right value?

Sorry, I think I didn't explain this very well. What I meant is that you should,
just once in the entire lifetime of the application (i.e. when the
WebViewChromiumFactoryProvider is initialised for the first time) check whether
it's HTC Mail or not (not once per webview creation as you do now), and that
instead of reflecting into WebView in WebViewChromium, you should just do it
here in WebViewChromiumFactoryProvider.createWebView. By "setting it back to the
right value" i mean "turning it back off again if the app is HTC Mail", after it
gets turned back on by the framework code.

However, something I didn't think about enough in the previous comments is that
the fact that the framework turns this back on when each webview is created
means that this workaround is not actually safe, and cannot ever be I suspect.
Every time the app creates a WebView, there will be a brief time window when
thread checking is turned back on, and if the timing is unfortunate the app will
crash anyway, despite this workaround. I guess there's not much we can do about
this :(

[Torne, 2016-08-16 11:45:07]

Also, can you remove the (upstream) from the commit message?

https://codereview.chromium.org/2243753002/diff/140001/android_webview/glue/j...
File
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
(right):

https://codereview.chromium.org/2243753002/diff/140001/android_webview/glue/j...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:116:
WebView.PrivateAccess webViewPrivate, boolean isHtcMail) {
Let's not call this "isHtcMail" - let's name it after what it actually does:
something like enablePostingEvaluateJavascript? (ugh, long name.. something
shorter?)

[commit-bot: I haz the power, 2016-08-16 11:56:36]

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

[commit-bot: I haz the power, 2016-08-16 11:56:37]

Dry run: This issue passed the CQ dry run.

[aelias_OOO_until_Jul13, 2016-08-16 22:25:50]

lgtm as far as I'm concerned.

On 2016/08/16 at 11:43:07, torne wrote:
> However, something I didn't think about enough in the previous comments is
that the fact that the framework turns this back on when each webview is created
means that this workaround is not actually safe, and cannot ever be I suspect.
Every time the app creates a WebView, there will be a brief time window when
thread checking is turned back on, and if the timing is unfortunate the app will
crash anyway, despite this workaround. I guess there's not much we can do about
this :(

It's a unique, very specific known issue on already existing builds we're
working around here.  And we know that it shouldn't ever race with WebView
creation.  It happens when the IME is first interacted with, which means the
WebView is not only created but also loaded enough to know there's a textbox. 
It's also in a context where we know the app shouldn't be creating a second
WebView.  So I think the current way of doing this is safe.

[Changwan Ryu, 2016-08-17 07:51:16]

Description was changed from

==========
Prevent thread assertion for HTC mail apk (upstream)

HTC's mail apk calls evaluateJavaScript() inside
InputConnection#setComposingText(). This is problematic with ImeThread
feature since InputConnection methods are now running on its own thread
(IME thread). This causes a thread check assertion in Android's framework
(android.webkit.WebView).

As a workaround, we set sEnforceThreadChecking to false immediately after
it was set in WebView's constructor.

HTC is actively fixing their apk to avoid this crash, but as a last resort,
this workaround will help avoid such a crash.

BUG=636717, 622151
==========

to

==========
Prevent thread assertion for HTC mail apk

HTC's mail apk calls evaluateJavaScript() inside
InputConnection#setComposingText(). This is problematic with ImeThread
feature since InputConnection methods are now running on its own thread
(IME thread). This causes a thread check assertion in Android's framework
(android.webkit.WebView).

As a workaround, we set sEnforceThreadChecking to false immediately after
it was set in WebView's constructor.

HTC is actively fixing their apk to avoid this crash, but as a last resort,
this workaround will help avoid such a crash.

BUG=636717, 622151
==========

[Changwan Ryu, 2016-08-17 07:53:51]

The CQ bit was checked by changwan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-17 07:54:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Changwan Ryu, 2016-08-17 08:00:13]

Thanks for the explanation.

I've removed upstream from commit message, moved the checking logic to factory
provider's initialize().

I think this should work fine assuming that
1) provider and webview are contained in HTC mail app process.
2) no new webview can be created while typing something.

But please let me know if you find anything problematic.

https://codereview.chromium.org/2243753002/diff/140001/android_webview/glue/j...
File
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
(right):

https://codereview.chromium.org/2243753002/diff/140001/android_webview/glue/j...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:116:
WebView.PrivateAccess webViewPrivate, boolean isHtcMail) {
On 2016/08/16 11:45:07, Torne wrote:
> Let's not call this "isHtcMail" - let's name it after what it actually does:
> something like enablePostingEvaluateJavascript? (ugh, long name.. something
> shorter?)

changed as shouldDisableThreadChecking

[commit-bot: I haz the power, 2016-08-17 08:24:08]

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

[commit-bot: I haz the power, 2016-08-17 08:24:09]

Dry run: This issue passed the CQ dry run.

[Torne, 2016-08-17 10:43:07]

https://codereview.chromium.org/2243753002/diff/160001/android_webview/glue/j...
File
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
(right):

https://codereview.chromium.org/2243753002/diff/160001/android_webview/glue/j...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:203:
// This is called here because otherwise compiler optimization may call
I'm really suspicious of this comment. What do you mean exactly? Called here as
opposed to where else? Are we protected by a lock here that we aren't in the
other location?

https://codereview.chromium.org/2243753002/diff/160001/android_webview/glue/j...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:571:
if (checkNeedsPost()) {
You need to combine this into the previous if condition, otherwise when
mShouldDisableThreadChecking is true but checkNeedsPost returns false, the call
will be entirely dropped.

https://codereview.chromium.org/2243753002/diff/160001/android_webview/glue/j...
File
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java
(right):

https://codereview.chromium.org/2243753002/diff/160001/android_webview/glue/j...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:506:
|| packageInfo.versionCode < 866001861) {
This condition doesn't work as intended; it will always disable thread checks if
the versionCode is less than 866001861 regardless of whether the device is M or
not.

You could do this as:
if (packageInfo.versionCode < (Build.VERSION.SDK_INT < Build.VERSION_CODES.M ?
864021756 : 866001861))

but it may be more clear to just expand it out to a nested if or something?

[Changwan Ryu, 2016-08-17 14:07:29]

The CQ bit was checked by changwan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-17 14:07:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Changwan Ryu, 2016-08-17 14:07:42]

https://codereview.chromium.org/2243753002/diff/160001/android_webview/glue/j...
File
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
(right):

https://codereview.chromium.org/2243753002/diff/160001/android_webview/glue/j...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:203:
// This is called here because otherwise compiler optimization may call
On 2016/08/17 10:43:07, Torne wrote:
> I'm really suspicious of this comment. What do you mean exactly? Called here
as
> opposed to where else? Are we protected by a lock here that we aren't in the
> other location?

As opposed to do it inside WebViewChromiumFactoryProvider::createWebView().
Removed comment to avoid confusion.

https://codereview.chromium.org/2243753002/diff/160001/android_webview/glue/j...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:571:
if (checkNeedsPost()) {
On 2016/08/17 10:43:07, Torne wrote:
> You need to combine this into the previous if condition, otherwise when
> mShouldDisableThreadChecking is true but checkNeedsPost returns false, the
call
> will be entirely dropped.

Good catch! Done.

https://codereview.chromium.org/2243753002/diff/160001/android_webview/glue/j...
File
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java
(right):

https://codereview.chromium.org/2243753002/diff/160001/android_webview/glue/j...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:506:
|| packageInfo.versionCode < 866001861) {
On 2016/08/17 10:43:07, Torne wrote:
> This condition doesn't work as intended; it will always disable thread checks
if
> the versionCode is less than 866001861 regardless of whether the device is M
or
> not.
> 
> You could do this as:
> if (packageInfo.versionCode < (Build.VERSION.SDK_INT < Build.VERSION_CODES.M ?
> 864021756 : 866001861))
> 
> but it may be more clear to just expand it out to a nested if or something?

Good catch! Fixed now.

[Torne, 2016-08-17 14:14:34]

https://codereview.chromium.org/2243753002/diff/160001/android_webview/glue/j...
File
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
(right):

https://codereview.chromium.org/2243753002/diff/160001/android_webview/glue/j...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:203:
// This is called here because otherwise compiler optimization may call
On 2016/08/17 14:07:41, Changwan Ryu wrote:
> On 2016/08/17 10:43:07, Torne wrote:
> > I'm really suspicious of this comment. What do you mean exactly? Called here
> as
> > opposed to where else? Are we protected by a lock here that we aren't in the
> > other location?
> 
> As opposed to do it inside WebViewChromiumFactoryProvider::createWebView().
> Removed comment to avoid confusion.

I'm still confused about what this means, though. Why would it be a problem to
do it in createWebView? The memory model doesn't really work in a way that means
an unsafe ordering can be prevented by just moving code around; you need actual
barriers/locks. So, unless I'm missing something here, either it was safe in
either place, or it's not safe in either place.

[commit-bot: I haz the power, 2016-08-17 14:46:07]

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

[commit-bot: I haz the power, 2016-08-17 14:46:08]

Dry run: This issue passed the CQ dry run.

[Changwan Ryu, 2016-08-18 09:31:02]

https://codereview.chromium.org/2243753002/diff/160001/android_webview/glue/j...
File
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
(right):

https://codereview.chromium.org/2243753002/diff/160001/android_webview/glue/j...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:203:
// This is called here because otherwise compiler optimization may call
On 2016/08/17 14:14:34, Torne wrote:
> On 2016/08/17 14:07:41, Changwan Ryu wrote:
> > On 2016/08/17 10:43:07, Torne wrote:
> > > I'm really suspicious of this comment. What do you mean exactly? Called
here
> > as
> > > opposed to where else? Are we protected by a lock here that we aren't in
the
> > > other location?
> > 
> > As opposed to do it inside WebViewChromiumFactoryProvider::createWebView().
> > Removed comment to avoid confusion.
> 
> I'm still confused about what this means, though. Why would it be a problem to
> do it in createWebView? The memory model doesn't really work in a way that
means
> an unsafe ordering can be prevented by just moving code around; you need
actual
> barriers/locks. So, unless I'm missing something here, either it was safe in
> either place, or it's not safe in either place.

Hmm.. I had to look up a bit.
https://docs.oracle.com/javase/specs/jls/se7/html/jls-17.html
JLS mentions reflection use case in Example 17.5.3-1, and it seems that
reflection does not give ordering guarantee, so there is still room that
reordering can happen between the line in WebView's constructor and the line
here.

The difference when I put the code in createWebView() and when I put it in
WebViewChromium.init() comes from, I think, ART's behavior under normal
optimization settings. I haven't tested it against official build optimization /
HTC's variation of ART, so there is uncertainty that this can actually work or
not.

BTW, I couldn't find a good way to set a memory barrier in this case. Do you
have any suggestion?

[Torne, 2016-08-18 10:14:27]

On 2016/08/18 09:31:02, Changwan Ryu wrote:
> Hmm.. I had to look up a bit.
> https://docs.oracle.com/javase/specs/jls/se7/html/jls-17.html
> JLS mentions reflection use case in Example 17.5.3-1, and it seems that
> reflection does not give ordering guarantee, so there is still room that
> reordering can happen between the line in WebView's constructor and the line
> here.

That's only about using reflection to set final fields, which are permitted a
much wider range of compiler optimisations.

It's still unclear exactly what the issue you're trying to fix here is, so it's
hard for me to suggest how to fix it. Are you worried about reordering solely
within the thread which executes this code (i.e. which sets
sEnforceThreadChecking to true in the WebView initialisation and then
subsequently sets it back to false here), or are you worried about whether the
change is visible to other threads?

> The difference when I put the code in createWebView() and when I put it in
> WebViewChromium.init() comes from, I think, ART's behavior under normal
> optimization settings. I haven't tested it against official build optimization
/
> HTC's variation of ART, so there is uncertainty that this can actually work or
> not.

Have you actually observed the code as you implemented it before being a
problem? I can't tell from this description :)

> BTW, I couldn't find a good way to set a memory barrier in this case. Do you
> have any suggestion?

You can't explicitly insert barriers in java; they occur only where the memory
model says they must.

I can't suggest how to solve this because I really don't understand at all what
problem you're trying to solve.

[Changwan Ryu, 2016-08-19 00:10:24]

The CQ bit was checked by changwan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 00:10:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Changwan Ryu, 2016-08-19 00:11:39]

On 2016/08/18 10:14:27, Torne wrote:
> On 2016/08/18 09:31:02, Changwan Ryu wrote:
> > Hmm.. I had to look up a bit.
> > https://docs.oracle.com/javase/specs/jls/se7/html/jls-17.html
> > JLS mentions reflection use case in Example 17.5.3-1, and it seems that
> > reflection does not give ordering guarantee, so there is still room that
> > reordering can happen between the line in WebView's constructor and the line
> > here.
> 
> That's only about using reflection to set final fields, which are permitted a
> much wider range of compiler optimisations.
> 
> It's still unclear exactly what the issue you're trying to fix here is, so
it's
> hard for me to suggest how to fix it. Are you worried about reordering solely
> within the thread which executes this code (i.e. which sets
> sEnforceThreadChecking to true in the WebView initialisation and then
> subsequently sets it back to false here), or are you worried about whether the
> change is visible to other threads?
> 
> > The difference when I put the code in createWebView() and when I put it in
> > WebViewChromium.init() comes from, I think, ART's behavior under normal
> > optimization settings. I haven't tested it against official build
optimization
> /
> > HTC's variation of ART, so there is uncertainty that this can actually work
or
> > not.
> 
> Have you actually observed the code as you implemented it before being a
> problem? I can't tell from this description :)
> 
> > BTW, I couldn't find a good way to set a memory barrier in this case. Do you
> > have any suggestion?
> 
> You can't explicitly insert barriers in java; they occur only where the memory
> model says they must.
> 
> I can't suggest how to solve this because I really don't understand at all
what
> problem you're trying to solve.

Ok, I just avoided this issue by moving the disabling code into initForReal().
It is guaranteed to be called before InputConnection() activation, thus
evaluateJavaScript()
will be called after the disabling code anyways.

PTAL.

[commit-bot: I haz the power, 2016-08-19 00:50:41]

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

[commit-bot: I haz the power, 2016-08-19 00:50:42]

Dry run: This issue passed the CQ dry run.

[Torne, 2016-08-19 11:45:33]

On 2016/08/19 00:11:39, Changwan Ryu wrote:
> On 2016/08/18 10:14:27, Torne wrote:
> > On 2016/08/18 09:31:02, Changwan Ryu wrote:
> > > Hmm.. I had to look up a bit.
> > > https://docs.oracle.com/javase/specs/jls/se7/html/jls-17.html
> > > JLS mentions reflection use case in Example 17.5.3-1, and it seems that
> > > reflection does not give ordering guarantee, so there is still room that
> > > reordering can happen between the line in WebView's constructor and the
line
> > > here.
> > 
> > That's only about using reflection to set final fields, which are permitted
a
> > much wider range of compiler optimisations.
> > 
> > It's still unclear exactly what the issue you're trying to fix here is, so
> it's
> > hard for me to suggest how to fix it. Are you worried about reordering
solely
> > within the thread which executes this code (i.e. which sets
> > sEnforceThreadChecking to true in the WebView initialisation and then
> > subsequently sets it back to false here), or are you worried about whether
the
> > change is visible to other threads?
> > 
> > > The difference when I put the code in createWebView() and when I put it in
> > > WebViewChromium.init() comes from, I think, ART's behavior under normal
> > > optimization settings. I haven't tested it against official build
> optimization
> > /
> > > HTC's variation of ART, so there is uncertainty that this can actually
work
> or
> > > not.
> > 
> > Have you actually observed the code as you implemented it before being a
> > problem? I can't tell from this description :)
> > 
> > > BTW, I couldn't find a good way to set a memory barrier in this case. Do
you
> > > have any suggestion?
> > 
> > You can't explicitly insert barriers in java; they occur only where the
memory
> > model says they must.
> > 
> > I can't suggest how to solve this because I really don't understand at all
> what
> > problem you're trying to solve.
> 
> Ok, I just avoided this issue by moving the disabling code into initForReal().
> It is guaranteed to be called before InputConnection() activation, thus
> evaluateJavaScript()
> will be called after the disabling code anyways.
> 
> PTAL.

Doing it after a post seems *less* safe, as that delays it further, which makes
the time window where it's set to the wrong value larger - if the app in
question is never using two webviews concurrently then that doesn't really
matter, as we've discussed earlier, but nonetheless making this worse for no
benefit doesn't seem ideal. You should disable it as early as possible: you
could just do it during the WebViewChromium constructor, when you're passed the
value of the boolean, or right back where I originally suggested in the factory
provider.

It sounds like you're worried about the JIT deciding to reorder the reflection
code with respect to the original setting of the static in the WebView
constructor (i.e. things happening on the same thread): this really can't
happen. It's not permitted to reorder your code in a way that means that the
visible effects within a single thread happen out of order with respect to the
source code. The section of the JLS you linked to is about using reflection to
alter *final* variables, which are a special case as the compiler/runtime can
normally assume they are constants, and so changing their value via reflection
is a weird occurrence that has implications for the memory model. This variable
is *not* final, and so the compiler and runtime *must* assume that calling
anything whatsoever might change its value unless it can prove otherwise
(basically, inlining enough code that it can see for itself all memory accesses
that happen), and that's not going to happen for reflection calls.

Did you actually observe this not working correctly when you put the code
somewhere else? If so then I'd like to see exactly where you put it that wasn't
working, because that would be very surprising; but if you are just trying to be
cautious, it's not necessary, and I think it would be better and clearer to do
it as early as possible.

[Changwan Ryu, 2016-08-23 06:32:41]

On 2016/08/19 11:45:33, Torne wrote:
> On 2016/08/19 00:11:39, Changwan Ryu wrote:
> > On 2016/08/18 10:14:27, Torne wrote:
> > > On 2016/08/18 09:31:02, Changwan Ryu wrote:
> > > > Hmm.. I had to look up a bit.
> > > > https://docs.oracle.com/javase/specs/jls/se7/html/jls-17.html
> > > > JLS mentions reflection use case in Example 17.5.3-1, and it seems that
> > > > reflection does not give ordering guarantee, so there is still room that
> > > > reordering can happen between the line in WebView's constructor and the
> line
> > > > here.
> > > 
> > > That's only about using reflection to set final fields, which are
permitted
> a
> > > much wider range of compiler optimisations.
> > > 
> > > It's still unclear exactly what the issue you're trying to fix here is, so
> > it's
> > > hard for me to suggest how to fix it. Are you worried about reordering
> solely
> > > within the thread which executes this code (i.e. which sets
> > > sEnforceThreadChecking to true in the WebView initialisation and then
> > > subsequently sets it back to false here), or are you worried about whether
> the
> > > change is visible to other threads?
> > > 
> > > > The difference when I put the code in createWebView() and when I put it
in
> > > > WebViewChromium.init() comes from, I think, ART's behavior under normal
> > > > optimization settings. I haven't tested it against official build
> > optimization
> > > /
> > > > HTC's variation of ART, so there is uncertainty that this can actually
> work
> > or
> > > > not.
> > > 
> > > Have you actually observed the code as you implemented it before being a
> > > problem? I can't tell from this description :)
> > > 
> > > > BTW, I couldn't find a good way to set a memory barrier in this case. Do
> you
> > > > have any suggestion?
> > > 
> > > You can't explicitly insert barriers in java; they occur only where the
> memory
> > > model says they must.
> > > 
> > > I can't suggest how to solve this because I really don't understand at all
> > what
> > > problem you're trying to solve.
> > 
> > Ok, I just avoided this issue by moving the disabling code into
initForReal().
> > It is guaranteed to be called before InputConnection() activation, thus
> > evaluateJavaScript()
> > will be called after the disabling code anyways.
> > 
> > PTAL.
> 
> Doing it after a post seems *less* safe, as that delays it further, which
makes
> the time window where it's set to the wrong value larger - if the app in
> question is never using two webviews concurrently then that doesn't really
> matter, as we've discussed earlier, but nonetheless making this worse for no
> benefit doesn't seem ideal. You should disable it as early as possible: you
> could just do it during the WebViewChromium constructor, when you're passed
the
> value of the boolean, or right back where I originally suggested in the
factory
> provider.
> 
> It sounds like you're worried about the JIT deciding to reorder the reflection
> code with respect to the original setting of the static in the WebView
> constructor (i.e. things happening on the same thread): this really can't
> happen. It's not permitted to reorder your code in a way that means that the
> visible effects within a single thread happen out of order with respect to the
> source code. The section of the JLS you linked to is about using reflection to
> alter *final* variables, which are a special case as the compiler/runtime can
> normally assume they are constants, and so changing their value via reflection
> is a weird occurrence that has implications for the memory model. This
variable
> is *not* final, and so the compiler and runtime *must* assume that calling
> anything whatsoever might change its value unless it can prove otherwise
> (basically, inlining enough code that it can see for itself all memory
accesses
> that happen), and that's not going to happen for reflection calls.
> 
> Did you actually observe this not working correctly when you put the code
> somewhere else? If so then I'd like to see exactly where you put it that
wasn't
> working, because that would be very surprising; but if you are just trying to
be
> cautious, it's not necessary, and I think it would be better and clearer to do
> it as early as possible.

Sure, I've uploaded a patch that does not work. Please check and advise.

[Changwan Ryu, 2016-08-24 07:38:05]

One thing to note: because of the nature of how ThreadedInputConnectionFactory
works
(https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro...)

delaying this by one UI cycle shouldn't be a problem assuming that we have only
one webview instance in the window.

Also, I have reduced the overall code structure to the following:

public A {
  private static volatile boolean sValue = false;
  private C mC;

  public A(B b) {
    sValue = getTrueExternally();
    mC = b.createC();
  }

  public boolean getValue() {
    return sValue;
  }

  private boolean getTrueExternally() {
    // Check DB to eventually get true.
    ...
  }
}

public B {
   public C createC(A a) {
      C c = new C(this, a);
      changeAValueToFalse();
      return wvc;
   }
   private void changeValueToFalse() {
      // Set A.sValue to false using reflection
      ...
   }
}

int main() {
 B b = new B();
 A a = new A(b);
 assertFalse(a.getValue());  // fails. B#changeValueToFalse() was called before
A#getTrueExternally()
}

Does this make sense? You can try this against HTC mail client apk mentioned in
the crbug.
Basically, changeValueToFalse() does not explicitly refer to A.sValue,
so compiler has no reason to guarantee the execution order.

As you said, moving around the code cannot change the order guarantee
theoretically,
so I think patch set #11 may be safer.

[Torne, 2016-08-24 11:02:59]

OK, so this is my reasoning for why I don't think this is permissible to be
reordered from the perspective of the main thread doing the writes (at any
stage: bytecode compilation, dex translation, AOT/JIT compilation). This will
include some things you already know, because I'm going to link to this CL when
asking other experts about this who aren't already familiar with the WebView
code.

In source order:

1) The android.webkit.WebView constructor sets sEnforceThreadChecking based on
target SDK. This is a regular write to a volatile field. Per the new memory
model's volatile semantics (from JSR-133 in java 1.4) this has the semantics of
a monitor release - which means that it's forbidden to have *any* later write,
even to a non-volatile field, reordered to happen *before* the volatile field
update. I think this should, by itself, already be enough to guarantee the
ordering is correct without any further considerations..

2) Next it calls checkThread(), which internally reads sEnforceThreadChecking.
Again, since the field is volatile, this read has the semantics of a monitor
acquire, which means that any later *read* cannot be reordered to happen before
this either.

3) Then it calls ensureProviderCreated(), which calls checkThread() again
(whoops, guess that previous one was redundant, but nevermind), sees that the
provider is null, and calls getFactory().createWebView(this, new
PrivateAccess()) to create it. This is the point where we leave the framework
code and jump into the WebView APK. So at this point, the code is all (so far)
compiled into the framework, and at the time of compilation the definition of
createWebView is not available (since it's loaded dynamically at runtime) - this
means the compiler here has no idea what the function it's calling is going to
do, and therefore must pessimistically assume that the entire world changes
during this call. This means that if there's actually a reordering here, it must
be happening later, in AOT/JIT compilation.

4) getFactory() resolves at runtime to an instance of
WebViewChromiumFactoryProvider and so we end up in the createWebView
implementation you are updating here in your CL. We construct a WebViewChromium
(which doesn't do anything relevant) and then if mShouldDisableThreadChecking is
set, we call disableThreadChecking(). mShouldDisableThreadChecking is set during
construction of WebViewChromiumFactoryProvider if the app is the one where we
need to do this workaround. It's not final because of the way the constructors
are set up here (both calling an initialize() method) - we could and probably
should fix this, though, by moving the body of initialize into the 1-arg
constructor and having the 0-arg constructor delegate to that directly. However,
even though it's not currently final, getFactory() uses a regular lock to
protect the factory instance and so the initialisation of this field shouldn't
be a problem either (and presumably when you tested this with logging you
actually saw it call disableThreadChecking?)

5) disableThreadChecking uses reflection to reach into android.webkit.WebView
and set sEnforceThreadChecking back to false. I'm not certain what the memory
model semantics of an access via reflection are - it's possible this doesn't
really have volatile semantics even though the field is volatile, which could
have implications for ordering *between* threads? However, this doesn't seem
sufficient to permit a reordering on this same thread..

6) You checked the value after this point, on the same thread, and it's set back
to true somehow.

Does that sound like an accurate description? Because... I'm pretty sure that
unless I've made a big mistake here, there's several different reasons why we
can't end up observing the value of that field being true on this thread.

If it's only observable as the wrong value on a *different* thread, which is
where we actually care about it in the real code, then there's slightly more
scope for things to be messed up here due to memory reordering between CPUs, but
even then we have a bunch of happens-before relationships established and I
would be surprised if there is actually a "gap" where this can be observed to
break.

If there's actually more than one WebView being created then this could be
broken, as the creation of the second webview could be racing with the IME
thread processing an event for the first webview, but you said that's not the
case - and also for this to be observable as a problem there'd have to be two
webviews being *created* at once on different threads, which would have been
broken already before the IME thread was introduced, we don't allow that kind of
usage.

So I'm pretty stumped here. If we can create a minimal repro for this (ideally a
non-android-dependent one that could be tested on a regular JVM to compare with
ART) then that would make it much easier to reason about whether we're just
doing something not permitted, or whether this is an actual bug in the runtime,
but I have a nasty suspicion that taking too much away from the real-world case
is going to change the behaviour *anyway* and may just conceal, not fix, the
actual problem - so worth a try, but the failure to create a minimal repro
doesn't necessarily mean there's no issue :(

[aelias_OOO_until_Jul13, 2016-08-24 18:32:36]

I'm not following all the arcane details, but I'd like to take a step back here
and focus on the goal here.  We need to fix a few known HTC mail APKs that we
can individually test and that furthermore only ship on particular fixed Android
releases.  It doesn't matter whether or not the workaround is correct or
trustworthy in general principle as long as we test it works on those particular
targets, we believe it cannot affect any other APK, and we believe future
WebView changes from our end are also unlikely to regress the workaround in the
medium term.  Don't all those things hold true for at least one of the proposed
patchsets, and if so can we just go with that one?

[Torne, 2016-08-24 18:44:25]

No. There is unfortunately no reason to believe that any of these patchsets
work reliably even for the very specific cases we care about. The previous
one happened to work for changwan in whatever specific configuration he
tested, but that doesn't mean it will work for users. The differences
between the version that appears to work and the one that appears to be
broken is not a difference that is expected to have any effect, and so
without knowing why it appears to make a difference it's a fair assumption
that it's a fluke and may not be deterministic.

On Wed, 24 Aug 2016, 7:32 pm , <aelias@chromium.org> wrote:

> I'm not following all the arcane details, but I'd like to take a step back
> here
> and focus on the goal here. We need to fix a few known HTC mail APKs that
> we
> can individually test and that furthermore only ship on particular fixed
> Android
> releases. It doesn't matter whether or not the workaround is correct or
> trustworthy in general principle as long as we test it works on those
> particular
> targets, we believe it cannot affect any other APK, and we believe future
> WebView changes from our end are also unlikely to regress the workaround
> in the
> medium term. Don't all those things hold true for at least one of the
> proposed
> patchsets, and if so can we just go with that one?
>
> https://codereview.chromium.org/2243753002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Torne, 2016-08-25 12:48:47]

Sorry, that was a bit abrupt :/

I'm just worried that this workaround won't work reliably for users, which would
defeat the point of introducing it.

[aelias_OOO_until_Jul13, 2016-08-25 21:42:03]

No problem, I agree landing something we don't understand is generally a bad
practice.  I just wanted to make sure the bar wasn't set unnecessarily high as
I'm eager to move on to the next yak to shave to get IME thread finally
launched, but I appreciate that even a minimum degree of confidence hasn't been
attained yet in your judgement.

[sgurun-gerrit only, 2016-08-25 23:41:01]

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

[sgurun-gerrit only, 2016-08-25 23:41:02]

https://codereview.chromium.org/2243753002/diff/220001/android_webview/glue/j...
File
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java
(right):

https://codereview.chromium.org/2243753002/diff/220001/android_webview/glue/j...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:502:
final String htcMailPackageId = "com.htc.android.mail";
Sorry but I don't think we should create a precedent in our code by adding some
specialization for one APK. We should first make sure this is the right fix by
addressing Torne's comments and even then, I am not happy about it.

is this change temporary? (I very much hope so).

If not, can we do it in a different way. even a hidden api that can be called
via reflection is better, IMO.

[aelias_OOO_until_Jul13, 2016-08-26 00:16:40]

On 2016/08/25 at 23:41:02, sgurun wrote:
>
https://codereview.chromium.org/2243753002/diff/220001/android_webview/glue/j...
> File
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java
(right):
> 
>
https://codereview.chromium.org/2243753002/diff/220001/android_webview/glue/j...
>
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:502:
final String htcMailPackageId = "com.htc.android.mail";
> Sorry but I don't think we should create a precedent in our code by adding
some specialization for one APK. We should first make sure this is the right fix
by addressing Torne's comments and even then, I am not happy about it.
> 
> is this change temporary? (I very much hope so).
> 
> If not, can we do it in a different way. even a hidden api that can be called
via reflection is better, IMO.

The idea is that HTC is pushing a proper fix in new APKs, but this
specialization is to workaround the existing buggy APKs that they are unable to
deploy updates to.  Nobody likes it but the alternative is to crash that (small)
subset of HTC mail apps.  We've crashed them before and had a fair bit of
complaining come out of it, so we're trying to do something more painless.

If you have a concrete different way to propose we are open to it, but because
so much of the code involved in the crasher is set in stone, this is the only
working method we've come up with.

[aelias_OOO_until_Jul13, 2016-08-26 00:18:37]

It's temporary in the sense that it's unnecessary the more HTC rolls out
updates.  It's not temporary in that we would probably be afraid to remove the
workaround for a couple of years until we believe all affected users have
replaced their devices.

[Changwan Ryu, 2016-08-26 02:26:03]

The CQ bit was checked by changwan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 02:26:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Changwan Ryu, 2016-08-26 02:35:20]

Ok, as torne@ questioned, alternative approach of patch set #12 did not work for
a reason other than memory model.

I had incorrectly assumed that WebView constructor creates WebViewProvider
(WebViewChromium) by reading the code, and that’s why I suspected that it might
have to do with memory model. Several Java experts (including torne@) confirmed
that even reflection should not affect the program order in a single thread in
this case.

After adding lots of logs in the framework, I realized that WebViewChromium was
being constructed somewhere else.

View#<init>
→ WebView#setOverScrollMode()
→ WebView#ensureProviderCreated()
→ WebViewChromiumFactoryProvider#createWebView()

Then
WebView#<init>
→ WebViewChromium#init()

Therefore, createWebView() is called before sEnforceThreadChecking is set. If we
set the value inside WebViewChromium#init(), then there shouldn’t be any
problem. In this sense, I think by moving back to patch 10 and by removing
misleading comment, we should be good.

Also to answer sgurun@'s question of possibility of calling a hidden API,
it's the framework code that causes the crash and APK is out in the wild, the
best option for us is to
reset sEnforceThreadChecking as soon as it's set, so that's why we are changing
WebViewChromium code.
I could not think of any other good workaround so far.

[commit-bot: I haz the power, 2016-08-26 03:08:24]

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

[commit-bot: I haz the power, 2016-08-26 03:08:25]

Dry run: This issue passed the CQ dry run.

[sgurun-gerrit only, 2016-08-26 04:00:04]

On 2016/08/26 02:35:20, Changwan Ryu wrote:
> Ok, as torne@ questioned, alternative approach of patch set #12 did not work
for
> a reason other than memory model.
> 
> I had incorrectly assumed that WebView constructor creates WebViewProvider
> (WebViewChromium) by reading the code, and that’s why I suspected that it
might
> have to do with memory model. Several Java experts (including torne@)
confirmed
> that even reflection should not affect the program order in a single thread in
> this case.
> 
> After adding lots of logs in the framework, I realized that WebViewChromium
was
> being constructed somewhere else.
> 
> View#<init>
> → WebView#setOverScrollMode()
> → WebView#ensureProviderCreated()
> → WebViewChromiumFactoryProvider#createWebView()
> 
> Then
> WebView#<init>
> → WebViewChromium#init()
> 
> Therefore, createWebView() is called before sEnforceThreadChecking is set. If
we
> set the value inside WebViewChromium#init(), then there shouldn’t be any
> problem. In this sense, I think by moving back to patch 10 and by removing
> misleading comment, we should be good.
> 
> Also to answer sgurun@'s question of possibility of calling a hidden API,
> it's the framework code that causes the crash and APK is out in the wild, the
> best option for us is to
> reset sEnforceThreadChecking as soon as it's set, so that's why we are
changing
> WebViewChromium code.
> I could not think of any other good workaround so far.

well, that is sad :(

[Torne, 2016-08-26 11:25:01]

Selim, yes, the workaround here is unfortunate but because it's limited to a
fixed range of app versions that already shipped, I think we can live with it
for a while to avoid making a lot of HTC users justifiably upset.

One trivial comment to address, and then this LGTM.

https://codereview.chromium.org/2243753002/diff/240001/android_webview/glue/j...
File
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
(right):

https://codereview.chromium.org/2243753002/diff/240001/android_webview/glue/j...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:204:
if (mShouldDisableThreadChecking) disableThreadChecking();
Can you do this *before* posting the task to the other thread? (i.e on line 189)

[Changwan Ryu, 2016-08-26 11:35:03]

The CQ bit was checked by changwan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 11:35:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Changwan Ryu, 2016-08-26 11:36:31]

https://codereview.chromium.org/2243753002/diff/240001/android_webview/glue/j...
File
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
(right):

https://codereview.chromium.org/2243753002/diff/240001/android_webview/glue/j...
android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:204:
if (mShouldDisableThreadChecking) disableThreadChecking();
On 2016/08/26 11:25:01, Torne wrote:
> Can you do this *before* posting the task to the other thread? (i.e on line
189)

Done.

[Torne, 2016-08-26 12:03:06]

Thanks; LGTM :)

[commit-bot: I haz the power, 2016-08-26 12:42:50]

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

[commit-bot: I haz the power, 2016-08-26 12:42:51]

Dry run: This issue passed the CQ dry run.

[Changwan Ryu, 2016-08-26 15:15:33]

The CQ bit was checked by changwan@chromium.org

[Changwan Ryu, 2016-08-26 15:15:34]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org
Link to the patchset: https://codereview.chromium.org/2243753002/#ps260001
(title: "move disableThreadChecking before addTask")

[commit-bot: I haz the power, 2016-08-26 15:16:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 15:19:54]

Description was changed from

==========
Prevent thread assertion for HTC mail apk

HTC's mail apk calls evaluateJavaScript() inside
InputConnection#setComposingText(). This is problematic with ImeThread
feature since InputConnection methods are now running on its own thread
(IME thread). This causes a thread check assertion in Android's framework
(android.webkit.WebView).

As a workaround, we set sEnforceThreadChecking to false immediately after
it was set in WebView's constructor.

HTC is actively fixing their apk to avoid this crash, but as a last resort,
this workaround will help avoid such a crash.

BUG=636717, 622151
==========

to

==========
Prevent thread assertion for HTC mail apk

HTC's mail apk calls evaluateJavaScript() inside
InputConnection#setComposingText(). This is problematic with ImeThread
feature since InputConnection methods are now running on its own thread
(IME thread). This causes a thread check assertion in Android's framework
(android.webkit.WebView).

As a workaround, we set sEnforceThreadChecking to false immediately after
it was set in WebView's constructor.

HTC is actively fixing their apk to avoid this crash, but as a last resort,
this workaround will help avoid such a crash.

BUG=636717, 622151
==========

[commit-bot: I haz the power, 2016-08-26 15:19:56]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-08-26 15:21:37]

Description was changed from

==========
Prevent thread assertion for HTC mail apk

HTC's mail apk calls evaluateJavaScript() inside
InputConnection#setComposingText(). This is problematic with ImeThread
feature since InputConnection methods are now running on its own thread
(IME thread). This causes a thread check assertion in Android's framework
(android.webkit.WebView).

As a workaround, we set sEnforceThreadChecking to false immediately after
it was set in WebView's constructor.

HTC is actively fixing their apk to avoid this crash, but as a last resort,
this workaround will help avoid such a crash.

BUG=636717, 622151
==========

to

==========
Prevent thread assertion for HTC mail apk

HTC's mail apk calls evaluateJavaScript() inside
InputConnection#setComposingText(). This is problematic with ImeThread
feature since InputConnection methods are now running on its own thread
(IME thread). This causes a thread check assertion in Android's framework
(android.webkit.WebView).

As a workaround, we set sEnforceThreadChecking to false immediately after
it was set in WebView's constructor.

HTC is actively fixing their apk to avoid this crash, but as a last resort,
this workaround will help avoid such a crash.

BUG=636717, 622151

Committed: https://crrev.com/f99c4ade4b10c24a59b7bf34de4d7509e7ca8708
Cr-Commit-Position: refs/heads/master@{#414717}
==========

[commit-bot: I haz the power, 2016-08-26 15:21:38]

Patchset 14 (id:??) landed as
https://crrev.com/f99c4ade4b10c24a59b7bf34de4d7509e7ca8708
Cr-Commit-Position: refs/heads/master@{#414717}