Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2180423003:
aw: Fix FactoryProvider threading (Closed)
Descriptionaw: Fix FactoryProvider threading
Access thread-unsafe chromium classes on the UI thread only
BUG=630709
Committed: https://crrev.com/06550e2edacc1a6b1e797f0a96e933f297e3ffbe
Cr-Commit-Position: refs/heads/master@{#408643}
Patch Set 1 #
Total comments: 7
Patch Set 2 : rebase #Patch Set 3 : fix debug build #Patch Set 4 : skip post on ui thread #
Total comments: 2
Messages
Total messages: 31 (12 generated)
boliu@chromium.org changed reviewers: + torne@chromium.org
part 2 https://codereview.chromium.org/2180423003/diff/1/android_webview/glue/BUILD.gn File android_webview/glue/BUILD.gn (right): https://codereview.chromium.org/2180423003/diff/1/android_webview/glue/BUILD.... android_webview/glue/BUILD.gn:46: "//base:base_build_config_gen", for BuildConfig https://codereview.chromium.org/2180423003/diff/1/android_webview/glue/java/s... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2180423003/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:388: if (BuildConfig.IS_DEBUG && !ThreadUtils.runningOnUiThread()) { the new way to do asserts.. https://codereview.chromium.org/2180423003/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:502: mCookieManager = new CookieManagerAdapter(new AwCookieManager()); AwCookieManager is thread safe, right?
ping https://codereview.chromium.org/2180423003/diff/1/android_webview/glue/java/s... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2180423003/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:388: if (BuildConfig.IS_DEBUG && !ThreadUtils.runningOnUiThread()) { On 2016/07/27 05:30:26, boliu wrote: > the new way to do asserts.. I'll rebase this onto https://codereview.chromium.org/2181053002/ tomorrow
LGTM. We will have to keep an eye out for bugs after we land and ship this, though, because if apps are actually initialising webview by calling these methods on non-main threads, then this CL could potentially turn "technically undefined behaviour because lack of thread safety" (which may or may not actually cause a problem at runtime) into "deadlock" (if the app is doing something on the UI thread that won't complete until the other thread is done). https://codereview.chromium.org/2180423003/diff/1/android_webview/glue/java/s... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2180423003/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:388: if (BuildConfig.IS_DEBUG && !ThreadUtils.runningOnUiThread()) { On 2016/07/27 05:30:26, boliu wrote: > the new way to do asserts.. So not objecting to this, but where did this "come from"? Was there a discussion somewhere of the best way to do asserts/checks in java that I missed? There's an Amazon engineer trying to implement a CHECK/DCHECK equivalent for Java over in https://codereview.chromium.org/1918403004/ and it seems like if we already settled on a different approach we should try to coordinate everything :) https://codereview.chromium.org/2180423003/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:502: mCookieManager = new CookieManagerAdapter(new AwCookieManager()); On 2016/07/27 05:30:26, boliu wrote: > AwCookieManager is thread safe, right? Yes, this explicitly works from any thread.
On 2016/07/28 11:47:12, Torne wrote: > LGTM. We will have to keep an eye out for bugs after we land and ship this, > though, because if apps are actually initialising webview by calling these > methods on non-main threads, then this CL could potentially turn "technically > undefined behaviour because lack of thread safety" (which may or may not > actually cause a problem at runtime) into "deadlock" (if the app is doing > something on the UI thread that won't complete until the other thread is done). Yep. See who screams as a result I guess. https://codereview.chromium.org/2180423003/diff/1/android_webview/glue/java/s... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2180423003/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:388: if (BuildConfig.IS_DEBUG && !ThreadUtils.runningOnUiThread()) { On 2016/07/28 11:47:12, Torne wrote: > On 2016/07/27 05:30:26, boliu wrote: > > the new way to do asserts.. > > So not objecting to this, but where did this "come from"? Was there a discussion > somewhere of the best way to do asserts/checks in java that I missed? Oh I just copied Ted's CL here: https://codereview.chromium.org/2177473004/, ie the CL that started this bug in the first place :p > > There's an Amazon engineer trying to implement a CHECK/DCHECK equivalent for > Java over in https://codereview.chromium.org/1918403004/ and it seems like if we > already settled on a different approach we should try to coordinate everything > :) Definitely should use BuildConfig.DCHECK_IS_ON then?
The CQ bit was checked by boliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from torne@chromium.org Link to the patchset: https://codereview.chromium.org/2180423003/#ps20001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
On 2016/07/28 14:25:54, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) hmm, multidex issues..?
The CQ bit was checked by boliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from torne@chromium.org Link to the patchset: https://codereview.chromium.org/2180423003/#ps40001 (title: "fix debug build")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== aw: Fix FactoryProvider threading Access thread-unsafe chromium classes on the UI thread only BUG=630709 ========== to ========== aw: Fix FactoryProvider threading Access thread-unsafe chromium classes on the UI thread only BUG=630709 Committed: https://crrev.com/993fff8648f2701910b9efdbf1d35244fbf8ca73 Cr-Commit-Position: refs/heads/master@{#408432} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/993fff8648f2701910b9efdbf1d35244fbf8ca73 Cr-Commit-Position: refs/heads/master@{#408432}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2197433002/ by boliu@chromium.org. The reason for reverting is: Broke correct usage: crbug.com/632547 BUG=632547.
Message was sent while issue was closed.
Description was changed from ========== aw: Fix FactoryProvider threading Access thread-unsafe chromium classes on the UI thread only BUG=630709 Committed: https://crrev.com/993fff8648f2701910b9efdbf1d35244fbf8ca73 Cr-Commit-Position: refs/heads/master@{#408432} ========== to ========== aw: Fix FactoryProvider threading Access thread-unsafe chromium classes on the UI thread only BUG=630709 Committed: https://crrev.com/993fff8648f2701910b9efdbf1d35244fbf8ca73 Cr-Commit-Position: refs/heads/master@{#408432} ==========
Description was changed from ========== aw: Fix FactoryProvider threading Access thread-unsafe chromium classes on the UI thread only BUG=630709 Committed: https://crrev.com/993fff8648f2701910b9efdbf1d35244fbf8ca73 Cr-Commit-Position: refs/heads/master@{#408432} ========== to ========== aw: Fix FactoryProvider threading Access thread-unsafe chromium classes on the UI thread only BUG=630709 ==========
ptal, need to skip posting if already on ui thread
LGTM, but I'm surprised that runOnUiThreadBlocking doesn't just run its payload directly if already on the UI thread. Any idea why it explicitly checks for that and prevents it? https://codereview.chromium.org/2180423003/diff/60001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2180423003/diff/60001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:487: ? getBrowserContextOnUiThread().getGeolocationPermissions() minor thing: seems like it might be preferable to create the Callable and then just call it immediately, so we don't duplicate the actual expression? not sure that's really much better though.
On 2016/07/29 11:24:26, Torne wrote: > LGTM, but I'm surprised that runOnUiThreadBlocking doesn't just run its payload > directly if already on the UI thread. Thread utils does that already. It's in runBlockingFuture glue code, where it explicitly throws if it's called on the UI thread > Any idea why it explicitly checks for that > and prevents it? I think it's probably a good thing in runBlockingFuture. Don't create more callable/runnable unless necessary, ie optimize for correct use case. > > https://codereview.chromium.org/2180423003/diff/60001/android_webview/glue/ja... > File > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java > (right): > > https://codereview.chromium.org/2180423003/diff/60001/android_webview/glue/ja... > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:487: > ? getBrowserContextOnUiThread().getGeolocationPermissions() > minor thing: seems like it might be preferable to create the Callable and then > just call it immediately, so we don't duplicate the actual expression? not sure > that's really much better though.
https://codereview.chromium.org/2180423003/diff/60001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2180423003/diff/60001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:487: ? getBrowserContextOnUiThread().getGeolocationPermissions() On 2016/07/29 11:24:26, Torne wrote: > minor thing: seems like it might be preferable to create the Callable and then > just call it immediately, so we don't duplicate the actual expression? not sure > that's really much better though. I guess we don't do that in WebVIewChromium because we don't want to be creating lots of callables/runnables if they are not needed. Even though that's less of a problem here since these are singletons, I don't want to start a new pattern.
On 2016/07/29 13:23:56, boliu wrote: > https://codereview.chromium.org/2180423003/diff/60001/android_webview/glue/ja... > File > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java > (right): > > https://codereview.chromium.org/2180423003/diff/60001/android_webview/glue/ja... > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:487: > ? getBrowserContextOnUiThread().getGeolocationPermissions() > On 2016/07/29 11:24:26, Torne wrote: > > minor thing: seems like it might be preferable to create the Callable and then > > just call it immediately, so we don't duplicate the actual expression? not > sure > > that's really much better though. > > I guess we don't do that in WebVIewChromium because we don't want to be creating > lots of callables/runnables if they are not needed. Even though that's less of a > problem here since these are singletons, I don't want to start a new pattern. OK, fair enough.
The CQ bit was checked by boliu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== aw: Fix FactoryProvider threading Access thread-unsafe chromium classes on the UI thread only BUG=630709 ========== to ========== aw: Fix FactoryProvider threading Access thread-unsafe chromium classes on the UI thread only BUG=630709 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== aw: Fix FactoryProvider threading Access thread-unsafe chromium classes on the UI thread only BUG=630709 ========== to ========== aw: Fix FactoryProvider threading Access thread-unsafe chromium classes on the UI thread only BUG=630709 Committed: https://crrev.com/06550e2edacc1a6b1e797f0a96e933f297e3ffbe Cr-Commit-Position: refs/heads/master@{#408643} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/06550e2edacc1a6b1e797f0a96e933f297e3ffbe Cr-Commit-Position: refs/heads/master@{#408643} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
