Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
581423002:
[aw] Ensure that WebView APIs do not crash if called after destruction. (Closed)
|
Created:
6 years, 3 months ago by Ignacio Solla Modified:
6 years, 3 months ago CC:
chromium-reviews, android-webview-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Description[aw] Ensure that WebView APIs do not crash if called after destruction.
The documentation for WebView#destroy() reads "This method should be called
after this WebView has been removed from the view system. No other methods
may be called on this WebView after destroy". However, some apps do not
respect that restriction so this change ensures that we fail gracefully
in that case and do not crash.
BUG=415666
Committed: https://crrev.com/781fc47c526db3aabbf86dfca15edaaa018fc67d
Cr-Commit-Position: refs/heads/master@{#296029}
Patch Set 1 #Patch Set 2 : Also fix APIs that do not use mContentViewCore, mWebContents or mNavigationController directly. #
Total comments: 4
Patch Set 3 : Log errors if things are / are not null when they shouldn't / should. #
Total comments: 15
Patch Set 4 : Address Bo's comments #Patch Set 5 : Add missing file, AwcontentViewClient #
Total comments: 3
Patch Set 6 : Fix findbugs warning. #
Messages
Total messages: 23 (4 generated)
igsolla@chromium.org changed reviewers: + benm@chromium.org, boliu@chromium.org
benm@chromium.org: Please review changes in boliu@chromium.org: Please review changes in
benm@chromium.org: Please review changes in boliu@chromium.org: Please review changes in
Also need to check where mContentViewCore/mWebContents/mNavigationController is passed into and held by other objects, either it won't be used there or we add checks as well https://codereview.chromium.org/581423002/diff/20001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/581423002/diff/20001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:255: private boolean mIsDestroyed = false; Hmm... I don't know if this is better or worse than just setting things null and then doing null checks instead. I think null check might be slightly better because it's easy to forget to check mIsDestroyed. I think maybe psychologically, null check is harder to forget since you are using the variable right there. We can also add a function that asserts mIsDestroyed if things are null, and then log an error too. Thoughts Ben?
On 2014/09/19 15:47:19, boliu wrote: > Also need to check where mContentViewCore/mWebContents/mNavigationController is > passed into and held by other objects, either it won't be used there or we add > checks as well I'm not sure about this. My intention is to shield us against misbehave apps that call WebView APIs after calling WebView#destroy(). For that, null-checks in the entry points should be enough. chromium should not try to use AwContents after they have been destroyed, and if it does we can probably come up with a better fix.
https://codereview.chromium.org/581423002/diff/20001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/581423002/diff/20001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:255: private boolean mIsDestroyed = false; On 2014/09/19 15:47:19, boliu wrote: > Hmm... > > I don't know if this is better or worse than just setting things null and then > doing null checks instead. > > I think null check might be slightly better because it's easy to forget to check > mIsDestroyed. I think maybe psychologically, null check is harder to forget > since you are using the variable right there. > > We can also add a function that asserts mIsDestroyed if things are null, and > then log an error too. > > Thoughts Ben? Those variables can only be null after the object has been destroyed, so using mIsDestroyed makes that more obvious. If we use the null checks directly we loose that information, and we might actually be hiding some real issues, ie. those variables being null when they shouldn't. In addition, using mIsDestroyed avoids having multiple null checks when more than one variable is used.
On 2014/09/19 17:26:06, Ignacio Solla wrote: > On 2014/09/19 15:47:19, boliu wrote: > > Also need to check where mContentViewCore/mWebContents/mNavigationController > is > > passed into and held by other objects, either it won't be used there or we add > > checks as well > > I'm not sure about this. My intention is to shield us against misbehave apps > that call WebView APIs after calling WebView#destroy(). For that, null-checks in > the entry points should be enough. chromium should not try to use AwContents > after they have been destroyed, and if it does we can probably come up with a > better fix. Just because it's normally in a "callback from chromium" class doesn't mean apps doesn't have ways to call into it. One example I found is this: https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja... There are probably more. https://codereview.chromium.org/581423002/diff/20001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/581423002/diff/20001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:255: private boolean mIsDestroyed = false; On 2014/09/19 17:26:20, Ignacio Solla wrote: > On 2014/09/19 15:47:19, boliu wrote: > > Hmm... > > > > I don't know if this is better or worse than just setting things null and then > > doing null checks instead. > > > > I think null check might be slightly better because it's easy to forget to > check > > mIsDestroyed. I think maybe psychologically, null check is harder to forget > > since you are using the variable right there. > > > > We can also add a function that asserts mIsDestroyed if things are null, and > > then log an error too. > > > > Thoughts Ben? > > Those variables can only be null after the object has been destroyed, so using > mIsDestroyed makes that more obvious. If we use the null checks directly we > loose that information, and we might actually be hiding some real issues, ie. > those variables being null when they shouldn't. That's why I suggested adding assert mIsDestroyed :) > > In addition, using mIsDestroyed avoids having multiple null checks when more > than one variable is used. That's a good point. But which methods uses more than one object?
On 2014/09/19 17:33:40, boliu wrote: > On 2014/09/19 17:26:06, Ignacio Solla wrote: > > On 2014/09/19 15:47:19, boliu wrote: > > > Also need to check where mContentViewCore/mWebContents/mNavigationController > > is > > > passed into and held by other objects, either it won't be used there or we > add > > > checks as well > > > > I'm not sure about this. My intention is to shield us against misbehave apps > > that call WebView APIs after calling WebView#destroy(). For that, null-checks > in > > the entry points should be enough. chromium should not try to use AwContents > > after they have been destroyed, and if it does we can probably come up with a > > better fix. > > Just because it's normally in a "callback from chromium" class doesn't mean apps > doesn't have ways to call into it. One example I found is this: > https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja... > > There are probably more. The scope of what you’re proposing is different to the scope of this patch, so I will work on that in a follow-up change: 1. This patch: ensure that apps will not crash if invoking a WebView API directly after calling destroy 2. Your proposal: ensure that callbacks from chromium do not crash after ContentViewCore and natives have been destroyed Those are logically separate goals and I prefer to keep them separate to ensure that changes are easy to review. I will work on 2 next.
https://codereview.chromium.org/581423002/diff/20001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/581423002/diff/20001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:255: private boolean mIsDestroyed = false; On 2014/09/19 17:33:39, boliu wrote: > On 2014/09/19 17:26:20, Ignacio Solla wrote: > > On 2014/09/19 15:47:19, boliu wrote: > > > Hmm... > > > > > > I don't know if this is better or worse than just setting things null and > then > > > doing null checks instead. > > > > > > I think null check might be slightly better because it's easy to forget to > > check > > > mIsDestroyed. I think maybe psychologically, null check is harder to forget > > > since you are using the variable right there. > > > > > > We can also add a function that asserts mIsDestroyed if things are null, and > > > then log an error too. > > > > > > Thoughts Ben? > > > > Those variables can only be null after the object has been destroyed, so using > > mIsDestroyed makes that more obvious. If we use the null checks directly we > > loose that information, and we might actually be hiding some real issues, ie. > > those variables being null when they shouldn't. > > That's why I suggested adding assert mIsDestroyed :) I have added an isDestroyed() method that logs an error as you suggested. Is that what you had in mind? Not sure how useful that is, given that those states should not be possible. > > > > > In addition, using mIsDestroyed avoids having multiple null checks when more > > than one variable is used. > > That's a good point. But which methods uses more than one object? loadUrl for example. But my main point is that using mIsDestroyed makes the 2 states of operation obvious: normal mode where nothing should be null, and destroyed mode where objects are null. Using the null checks directly obscures that and we may be hiding real problems.
https://codereview.chromium.org/581423002/diff/40001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/581423002/diff/40001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:645: View enterFullScreen() { I think we need to check destroy in enter/exit full screen too? https://codereview.chromium.org/581423002/diff/40001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:755: mIsDestroyed = false; // As we're about to recreate the native counterpart. Can you factor out the common code in destroy into a helper function, call the helper function here, and remove this setting to mIsDestroyed to false here? https://codereview.chromium.org/581423002/diff/40001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:900: private boolean isDestroyed() { Convert these to asserts so they are not expensive in production And check mNativeAwContents too https://codereview.chromium.org/581423002/diff/40001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:1144: if (mNativeAwContents != 0) { this can become an assert now (or just remove it) https://codereview.chromium.org/581423002/diff/40001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:1186: if (mNativeAwContents != 0) nativeSetBackgroundColor(mNativeAwContents, color); forgot one :) https://codereview.chromium.org/581423002/diff/40001/android_webview/javatest... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java (right): https://codereview.chromium.org/581423002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:100: public void testWebViewApisFailGracefullyAfterDestruction() throws Throwable { Make this a UI test (@UiThreadTest), you are calling these functions all on the wrong thread... https://codereview.chromium.org/581423002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:103: destroyAwContentsOnMainSync(awContents); I think we want to check this actually did something. (At some point in the past, destroy would do nothing if webview is still attached). Maybe check getContentViewCore is null?
https://codereview.chromium.org/581423002/diff/40001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/581423002/diff/40001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:645: View enterFullScreen() { On 2014/09/22 16:23:13, boliu wrote: > I think we need to check destroy in enter/exit full screen too? Done. https://codereview.chromium.org/581423002/diff/40001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:755: mIsDestroyed = false; // As we're about to recreate the native counterpart. On 2014/09/22 16:23:13, boliu wrote: > Can you factor out the common code in destroy into a helper function, call the > helper function here, and remove this setting to mIsDestroyed to false here? Done. https://codereview.chromium.org/581423002/diff/40001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:900: private boolean isDestroyed() { On 2014/09/22 16:23:13, boliu wrote: > Convert these to asserts so they are not expensive in production > And check mNativeAwContents too Done, but then I cannot log an error as you requested before. https://codereview.chromium.org/581423002/diff/40001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:1144: if (mNativeAwContents != 0) { On 2014/09/22 16:23:13, boliu wrote: > this can become an assert now (or just remove it) Done. https://codereview.chromium.org/581423002/diff/40001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:1186: if (mNativeAwContents != 0) nativeSetBackgroundColor(mNativeAwContents, color); On 2014/09/22 16:23:13, boliu wrote: > forgot one :) Done. https://codereview.chromium.org/581423002/diff/40001/android_webview/javatest... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java (right): https://codereview.chromium.org/581423002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:100: public void testWebViewApisFailGracefullyAfterDestruction() throws Throwable { On 2014/09/22 16:23:14, boliu wrote: > Make this a UI test (@UiThreadTest), you are calling these functions all on the > wrong thread... Done. https://codereview.chromium.org/581423002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:103: destroyAwContentsOnMainSync(awContents); On 2014/09/22 16:23:14, boliu wrote: > I think we want to check this actually did something. (At some point in the > past, destroy would do nothing if webview is still attached). > > Maybe check getContentViewCore is null? Done.
lgtm https://codereview.chromium.org/581423002/diff/40001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/581423002/diff/40001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:900: private boolean isDestroyed() { On 2014/09/22 17:04:16, Ignacio Solla wrote: > On 2014/09/22 16:23:13, boliu wrote: > > Convert these to asserts so they are not expensive in production > > And check mNativeAwContents too > > Done, but then I cannot log an error as you requested before. I think no log is ok at this point. No one pays attention to it unless we crash hard anyway.
The CQ bit was checked by igsolla@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/581423002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
https://codereview.chromium.org/581423002/diff/80001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/581423002/diff/80001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:1939: return isDestroyed() ? null : mContentViewCore.supportsAccessibilityAction(action); Woh, findbugs is useful sometimes :) This should be false rather than null. What exactly did this compile to?! Did it compile to (isDestroyed() ? null : mContentViewCore).supportsAccessibilityAction(action); That's kinda scary...
The CQ bit was checked by igsolla@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/581423002/100001
https://codereview.chromium.org/581423002/diff/80001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/581423002/diff/80001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:1939: return isDestroyed() ? null : mContentViewCore.supportsAccessibilityAction(action); On 2014/09/22 18:20:09, boliu wrote: > Woh, findbugs is useful sometimes :) This should be false rather than null. > > What exactly did this compile to?! Did it compile to > > (isDestroyed() ? null : mContentViewCore).supportsAccessibilityAction(action); > > That's kinda scary... I'm surprised it compiles, it is scary. This is a good reason to always write "condition ? exp : null" instead of "condition ? null : exp". I will always use the first form from now on.
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as b7afbe833a264b25fca1464a2481d7da1fa6a380
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/781fc47c526db3aabbf86dfca15edaaa018fc67d Cr-Commit-Position: refs/heads/master@{#296029}
Message was sent while issue was closed.
https://codereview.chromium.org/581423002/diff/80001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/581423002/diff/80001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContents.java:1939: return isDestroyed() ? null : mContentViewCore.supportsAccessibilityAction(action); On 2014/09/22 18:38:39, Ignacio Solla wrote: > On 2014/09/22 18:20:09, boliu wrote: > > Woh, findbugs is useful sometimes :) This should be false rather than null. > > > > What exactly did this compile to?! Did it compile to > > > > (isDestroyed() ? null : mContentViewCore).supportsAccessibilityAction(action); > > > > That's kinda scary... > > I'm surprised it compiles, it is scary. This is a good reason to always write > "condition ? exp : null" instead of "condition ? null : exp". I will always use > the first form from now on. Credit goes to newt who spotted this first. So the ternary expression is obtained from the two terms (null, and boolean) with the expected type (boolean). And one type actually works, ie the Boolean class that's auto boxed/unboxed. That's why findbugs is complainig about null, because it's trying to unbox null as a Boolean. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
