[Android WebView] Fix thread unsafety in accessing Java side getters

[Android WebView] Fix thread unsafety in accessing Java side getters

Re-route calls to AwSettings::PopulateWebPreferences through Java
to acquire mAwSettingsLock. Add assertions on holding the lock into
getters called from native code.

BUG=337112
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=248279

[mnaganov (inactive), 2014-01-23 10:38:05]

Ben, PTAL!

[benm (inactive), 2014-01-24 02:20:20]

thanks for catching this mikhail!

https://codereview.chromium.org/143803016/diff/180001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwSettings.java
(right):

https://codereview.chromium.org/143803016/diff/180001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwSettings.java:134: //
Message id for running a Runnable
nit: ... with the AwSettingsLock held.

https://codereview.chromium.org/143803016/diff/180001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwSettings.java:174:
mAwSettingsLock.wait();
shouldn't we have this in a while loop and set a bit when the runnable executes
in the handler above?

[mnaganov (inactive), 2014-01-24 10:01:46]

https://codereview.chromium.org/143803016/diff/180001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwSettings.java
(right):

https://codereview.chromium.org/143803016/diff/180001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwSettings.java:134: //
Message id for running a Runnable
On 2014/01/24 02:20:21, benm wrote:
> nit: ... with the AwSettingsLock held.

Done.

https://codereview.chromium.org/143803016/diff/180001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwSettings.java:174:
mAwSettingsLock.wait();
On 2014/01/24 02:20:21, benm wrote:
> shouldn't we have this in a while loop and set a bit when the runnable
executes
> in the handler above?

Ah, of course! Even better -- fixing this makes updateWebkitPreferencesLocked to
be a special case of maybeRunOnUiThreadBlocking. Since we sync to native
synchronously after each preference change, the role of
mIsUpdateWebkitPrefsMessagePending to prevent sending superfluous messages is
obsolete -- it is only used for syncing between threads. So I can throw away
some duplicate code!

[benm (inactive), 2014-01-24 23:11:11]

https://codereview.chromium.org/143803016/diff/270001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwSettings.java
(left):

https://codereview.chromium.org/143803016/diff/270001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwSettings.java:374: if
(mNativeAwSettings != 0) {
why remove this?

https://codereview.chromium.org/143803016/diff/270001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwSettings.java
(right):

https://codereview.chromium.org/143803016/diff/270001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwSettings.java:160: void
maybeRunOnUiThreadBlocking(Runnable r) {
I guess this should now have Locked on the end of the name?

https://codereview.chromium.org/143803016/diff/270001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwSettings.java:186: void
updateWebkitPreferencesLocked() {
assert Thread.holdsLock(mAwSettingsLock);

[mnaganov (inactive), 2014-01-29 09:33:27]

https://codereview.chromium.org/143803016/diff/270001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwSettings.java
(left):

https://codereview.chromium.org/143803016/diff/270001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwSettings.java:374: if
(mNativeAwSettings != 0) {
On 2014/01/24 23:11:11, benm wrote:
> why remove this?

It's now done inside the Handler's loop just before calling "run()" (line 148),
so semantically there is no change.

https://codereview.chromium.org/143803016/diff/270001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwSettings.java
(right):

https://codereview.chromium.org/143803016/diff/270001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwSettings.java:160: void
maybeRunOnUiThreadBlocking(Runnable r) {
On 2014/01/24 23:11:11, benm wrote:
> I guess this should now have Locked on the end of the name?

Right, it should. I added it and dropped "maybe" prefix since it doesn't seem to
add any useful information about the function.

https://codereview.chromium.org/143803016/diff/270001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwSettings.java:186: void
updateWebkitPreferencesLocked() {
On 2014/01/24 23:11:11, benm wrote:
> assert Thread.holdsLock(mAwSettingsLock);

It's done the first thing in maybeRunOnUiThreadBlocking. I think there is no
need for doing it twice, and was hoping this this doesn't cause a confusion
since these two functions are placed close to each other.

[benm (inactive), 2014-01-31 17:57:36]

thanks Mikhail!

lgtm however I still think that the assert should be added to
updateWebKitPreferencesLocked for clarity.

https://codereview.chromium.org/143803016/diff/270001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwSettings.java
(right):

https://codereview.chromium.org/143803016/diff/270001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwSettings.java:186: void
updateWebkitPreferencesLocked() {
In my opinion, it's inconsistent to not do it here, and looks like an accidental
omission as all the other xxxLocked methods have the assert.

[mnaganov (inactive), 2014-01-31 19:51:34]

Thanks, Ben!

https://codereview.chromium.org/143803016/diff/270001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwSettings.java
(right):

https://codereview.chromium.org/143803016/diff/270001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwSettings.java:186: void
updateWebkitPreferencesLocked() {
On 2014/01/31 17:57:37, benm wrote:
> In my opinion, it's inconsistent to not do it here, and looks like an
accidental
> omission as all the other xxxLocked methods have the assert.

OK, but to me, as long as this function is just calling another one (which makes
the check), an assert here looks superfluous.

Anyone, who will fall into thinking that an assert is missing, will look into
maybeRunOnUiThreadBlocking and find it there.

[mnaganov (inactive), 2014-01-31 19:51:47]

The CQ bit was checked by mnaganov@chromium.org

[commit-bot: I haz the power, 2014-01-31 19:53:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnaganov@chromium.org/143803016/410001

[commit-bot: I haz the power, 2014-01-31 21:17:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnaganov@chromium.org/143803016/410001

[commit-bot: I haz the power, 2014-01-31 21:51:29]

Change committed as 248279

[commit-bot: I haz the power, 2014-01-31 21:51:35]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 21:52:25]

CQ bit was unchecked on CL. Ignoring.