Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2339343002:
Android webview tries to switch to CE context (Closed)
|
Created:
4 years, 3 months ago by Nate Fischer Modified:
4 years, 3 months ago CC:
android-webview-reviews_chromium.org, chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAndroid webview tries to switch to CE context
If Webview is started in an application using
defaultToDeviceProtectedStorage="true", it will first try to switch to a CE
context. If it's unable to switch contexts, it throws an exception.
BUG=645921
Committed: https://crrev.com/18dad59572f1ed74069538bde20f25a4a8b3e586
Cr-Commit-Position: refs/heads/master@{#420730}
Patch Set 1 #
Total comments: 2
Patch Set 2 : rebase #Patch Set 3 : Addressed concerns from PS1 #
Total comments: 5
Patch Set 4 : Removed redundancy #
Total comments: 5
Patch Set 5 : Reverted WebViewChromium, refactored WebViewChromiumFactoryProvider #Patch Set 6 : Minor formatting changes #Messages
Total messages: 30 (10 generated)
Description was changed from ========== Android webview tries to switch to CE context If Webview is started in an application using defaultToDeviceProtectedStorage="true", it will first try to switch to a CE context. If it's unable to switch contexts, it throws an exception. BUG=645921 ========== to ========== Android webview tries to switch to CE context If Webview is started in an application using defaultToDeviceProtectedStorage="true", it will first try to switch to a CE context. If it's unable to switch contexts, it throws an exception. BUG=645921 ==========
ntfschr@chromium.org changed reviewers: + hush@chromium.org, sgurun@chromium.org
I again couldn't find the location for the unit tests, and I wasn't sure how to test the API for switching context, but I can write the tests if I'm pointed in the right direction. Thanks.
On 2016/09/15 01:52:43, Nate Fischer wrote: > I again couldn't find the location for the unit tests, and I wasn't sure how to > test the API for switching context, but I can write the tests if I'm pointed in > the right direction. > > Thanks. before looking at it: have you tested it manually, and if so how? let's first discuss it.
torne@chromium.org changed reviewers: + torne@chromium.org
https://codereview.chromium.org/2339343002/diff/1/android_webview/glue/java/s... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java (right): https://codereview.chromium.org/2339343002/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:117: WebViewChromiumFactoryProvider.switchStorageContextIfDeviceProtectedStorage( We explicitly *don't* want to allow a WebView to be constructed with a device-protected context. Converting this here will remove the point of having the assertion in the first place. The bug is that an app with the default-to-device-proteted flag set which *does* pass a credential-protected context consistently *still* crashes because of the application context. https://codereview.chromium.org/2339343002/diff/1/android_webview/glue/java/s... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2339343002/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:261: context.createCredentialProtectedStorageContext(); This doesn't actually switch anything - you aren't using the return value, so the credential-protected context is never used.
stopped by to say please don't break the caching in ResourcesContextWrapperFactory, ie the sCtxToWrapper map
PTAL
https://codereview.chromium.org/2339343002/diff/40001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java (right): https://codereview.chromium.org/2339343002/diff/40001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:117: Context ctx = WebViewChromiumFactoryProvider.switchToCredentialProtectedStorage( You're still allowing a WebView to be constructed with a device-protected context. We don't want this to be permitted. This file should just be left as-is. https://codereview.chromium.org/2339343002/diff/40001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2339343002/diff/40001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:265: // Check the manifest flag android:defaultToDeviceProtectedStorage I'm not sure there's any point in actually checking this. The only time the application context is going to be device-protected storage is when it's defaulting to it.
I believe this should address all concerns, unless I'm misunderstanding the entry points for creating WebViews. This also removes a check (that was previously there) which I believe was redundant. https://codereview.chromium.org/2339343002/diff/40001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java (right): https://codereview.chromium.org/2339343002/diff/40001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:117: Context ctx = WebViewChromiumFactoryProvider.switchToCredentialProtectedStorage( I'm a bit confused about what you mean here. Digging through the code, it seems like WebViewChromiumFactoryProvider.initialize() is being called before this point is ever reached. I believe WebViewChromiumFactoryProvider.initialize() successfully validates that it is given a CE context (or at least it will once I fix the concerns raised for that file). So I agree that I should revert this diff, since the check does appear to be redundant. However, I don't see how a DE context could still slip through, since the check happens even earlier than this point. Please correct me if I'm mistaken and I'm overlooking an entry point for creating WebViews. https://codereview.chromium.org/2339343002/diff/40001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2339343002/diff/40001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:265: // Check the manifest flag android:defaultToDeviceProtectedStorage On 2016/09/21 at 12:04:18, Torne wrote: > I'm not sure there's any point in actually checking this. The only time the application context is going to be device-protected storage is when it's defaulting to it. Ok, I misunderstood the bug report. I've removed the check.
https://codereview.chromium.org/2339343002/diff/40001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java (right): https://codereview.chromium.org/2339343002/diff/40001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:117: Context ctx = WebViewChromiumFactoryProvider.switchToCredentialProtectedStorage( On 2016/09/22 04:21:12, Nate Fischer wrote: > I'm a bit confused about what you mean here. > > Digging through the code, it seems like > WebViewChromiumFactoryProvider.initialize() is being called before this point is > ever reached. I believe WebViewChromiumFactoryProvider.initialize() successfully > validates that it is given a CE context (or at least it will once I fix the > concerns raised for that file). These are two totally unrelated things that use different contexts. The very first time a process calls any function relating to the webview implementation, we go through WebViewChromiumFactoryProvider.initialize(), which does *not* receive any context whatsoever, and therefore has to retrieve one for itself by using a @SystemApi framework method that gives us the Application object for the application in this process. In all normal apps, the Application object is a CE context, but in system apps that default to DE, the Application is also DE, which is the problem we're trying to work around. This code is only called once in the entire lifetime of the process, and happens even if the app does something unrelated to constructing an instance of WebView, such as interacting with CookieManager via its global singleton. WebViewChromium's constructor here is called when the app actually does "new WebView(context)" and so at this point we have the specific context provided to us by the application for the current activity. We want to make sure that this context is always a CE context, in all apps, no matter whether they default to DE or not, and the current code already does that correctly - you shouldn't remove the check, it's not redundant. > So I agree that I should revert this diff, since the check does appear to be > redundant. However, I don't see how a DE context could still slip through, since > the check happens even earlier than this point. > > Please correct me if I'm mistaken and I'm overlooking an entry point for > creating WebViews. https://codereview.chromium.org/2339343002/diff/60001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java (left): https://codereview.chromium.org/2339343002/diff/60001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:117: WebViewChromiumFactoryProvider.checkStorageIsNotDeviceProtected(webView.getContext()); Leave this as-is, it's correct. https://codereview.chromium.org/2339343002/diff/60001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2339343002/diff/60001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:264: if (!context.getSystemService(UserManager.class).isUserUnlocked()) { I think you should be getting a lint warning about this line: the version of getSystemService that takes a Class and returns the correct type (instead of the old sucky version that takes a String and returns Object) is new in API23 and so WebView can't expect it to be there in general. In this specific case, it's fine because the code is guarded by a version check, but the linter is supposed to point these out and require that you annotate the method to suppress it - is that not happening for you or am I mistaken?
On 2016/09/22 09:59:54, Torne wrote: > https://codereview.chromium.org/2339343002/diff/40001/android_webview/glue/ja... > File > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java > (right): > > https://codereview.chromium.org/2339343002/diff/40001/android_webview/glue/ja... > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:117: > Context ctx = WebViewChromiumFactoryProvider.switchToCredentialProtectedStorage( > On 2016/09/22 04:21:12, Nate Fischer wrote: > > I'm a bit confused about what you mean here. > > > > Digging through the code, it seems like > > WebViewChromiumFactoryProvider.initialize() is being called before this point > is > > ever reached. I believe WebViewChromiumFactoryProvider.initialize() > successfully > > validates that it is given a CE context (or at least it will once I fix the > > concerns raised for that file). > > These are two totally unrelated things that use different contexts. The very > first time a process calls any function relating to the webview implementation, > we go through WebViewChromiumFactoryProvider.initialize(), which does *not* > receive any context whatsoever, and therefore has to retrieve one for itself by > using a @SystemApi framework method that gives us the Application object for the > application in this process. In all normal apps, the Application object is a CE ^^^^^^^^^^^^^^^^ directbootaware is an application attribute, so I think even in non-system apps, the application context can be DE if they add this attribute? > context, but in system apps that default to DE, the Application is also DE, > which is the problem we're trying to work around. This code is only called once > in the entire lifetime of the process, and happens even if the app does > something unrelated to constructing an instance of WebView, such as interacting > with CookieManager via its global singleton. > > WebViewChromium's constructor here is called when the app actually does "new > WebView(context)" and so at this point we have the specific context provided to > us by the application for the current activity. We want to make sure that this > context is always a CE context, in all apps, no matter whether they default to > DE or not, and the current code already does that correctly - you shouldn't > remove the check, it's not redundant. > > > So I agree that I should revert this diff, since the check does appear to be > > redundant. However, I don't see how a DE context could still slip through, > since > > the check happens even earlier than this point. > > > > Please correct me if I'm mistaken and I'm overlooking an entry point for > > creating WebViews. > > https://codereview.chromium.org/2339343002/diff/60001/android_webview/glue/ja... > File > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java > (left): > > https://codereview.chromium.org/2339343002/diff/60001/android_webview/glue/ja... > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:117: > WebViewChromiumFactoryProvider.checkStorageIsNotDeviceProtected(webView.getContext()); > Leave this as-is, it's correct. > > https://codereview.chromium.org/2339343002/diff/60001/android_webview/glue/ja... > File > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java > (right): > > https://codereview.chromium.org/2339343002/diff/60001/android_webview/glue/ja... > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:264: > if (!context.getSystemService(UserManager.class).isUserUnlocked()) { > I think you should be getting a lint warning about this line: the version of > getSystemService that takes a Class and returns the correct type (instead of the > old sucky version that takes a String and returns Object) is new in API23 and so > WebView can't expect it to be there in general. In this specific case, it's fine > because the code is guarded by a version check, but the linter is supposed to > point these out and require that you annotate the method to suppress it - is > that not happening for you or am I mistaken?
On 2016/09/22 17:19:45, sgurun wrote: > On 2016/09/22 09:59:54, Torne wrote: > > > https://codereview.chromium.org/2339343002/diff/40001/android_webview/glue/ja... > > File > > > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java > > (right): > > > > > https://codereview.chromium.org/2339343002/diff/40001/android_webview/glue/ja... > > > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:117: > > Context ctx = > WebViewChromiumFactoryProvider.switchToCredentialProtectedStorage( > > On 2016/09/22 04:21:12, Nate Fischer wrote: > > > I'm a bit confused about what you mean here. > > > > > > Digging through the code, it seems like > > > WebViewChromiumFactoryProvider.initialize() is being called before this > point > > is > > > ever reached. I believe WebViewChromiumFactoryProvider.initialize() > > successfully > > > validates that it is given a CE context (or at least it will once I fix the > > > concerns raised for that file). > > > > These are two totally unrelated things that use different contexts. The very > > first time a process calls any function relating to the webview > implementation, > > we go through WebViewChromiumFactoryProvider.initialize(), which does *not* > > receive any context whatsoever, and therefore has to retrieve one for itself > by > > using a @SystemApi framework method that gives us the Application object for > the > > application in this process. In all normal apps, the Application object is a > CE > ^^^^^^^^^^^^^^^^ > directbootaware is an application attribute, so I think even in non-system apps, > the application context can be DE if they add this attribute? In a normal app *running after unlock* the app context will always be CE (whether the app is direct boot aware or not). But.. I'm actually not sure what it is if it's running before unlock? I think just refusing to convert the context unless the device is already unlocked will cover this in either case, though. > > > context, but in system apps that default to DE, the Application is also DE, > > which is the problem we're trying to work around. This code is only called > once > > in the entire lifetime of the process, and happens even if the app does > > something unrelated to constructing an instance of WebView, such as > interacting > > with CookieManager via its global singleton. > > > > WebViewChromium's constructor here is called when the app actually does "new > > WebView(context)" and so at this point we have the specific context provided > to > > us by the application for the current activity. We want to make sure that this > > context is always a CE context, in all apps, no matter whether they default to > > DE or not, and the current code already does that correctly - you shouldn't > > remove the check, it's not redundant. > > > > > So I agree that I should revert this diff, since the check does appear to be > > > redundant. However, I don't see how a DE context could still slip through, > > since > > > the check happens even earlier than this point. > > > > > > Please correct me if I'm mistaken and I'm overlooking an entry point for > > > creating WebViews. > > > > > https://codereview.chromium.org/2339343002/diff/60001/android_webview/glue/ja... > > File > > > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java > > (left): > > > > > https://codereview.chromium.org/2339343002/diff/60001/android_webview/glue/ja... > > > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:117: > > > WebViewChromiumFactoryProvider.checkStorageIsNotDeviceProtected(webView.getContext()); > > Leave this as-is, it's correct. > > > > > https://codereview.chromium.org/2339343002/diff/60001/android_webview/glue/ja... > > File > > > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java > > (right): > > > > > https://codereview.chromium.org/2339343002/diff/60001/android_webview/glue/ja... > > > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:264: > > if (!context.getSystemService(UserManager.class).isUserUnlocked()) { > > I think you should be getting a lint warning about this line: the version of > > getSystemService that takes a Class and returns the correct type (instead of > the > > old sucky version that takes a String and returns Object) is new in API23 and > so > > WebView can't expect it to be there in general. In this specific case, it's > fine > > because the code is guarded by a version check, but the linter is supposed to > > point these out and require that you annotate the method to suppress it - is > > that not happening for you or am I mistaken?
agreed On Thu, Sep 22, 2016 at 10:34 AM, <torne@chromium.org> wrote: > On 2016/09/22 17:19:45, sgurun wrote: > > On 2016/09/22 09:59:54, Torne wrote: > > > > > > https://codereview.chromium.org/2339343002/diff/40001/ > android_webview/glue/java/src/com/android/webview/chromium/ > WebViewChromium.java > > > File > > > > > > android_webview/glue/java/src/com/android/webview/chromium/ > WebViewChromium.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2339343002/diff/40001/ > android_webview/glue/java/src/com/android/webview/chromium/ > WebViewChromium.java#newcode117 > > > > > > android_webview/glue/java/src/com/android/webview/chromium/ > WebViewChromium.java:117: > > > Context ctx = > > WebViewChromiumFactoryProvider.switchToCredentialProtectedStorage( > > > On 2016/09/22 04:21:12, Nate Fischer wrote: > > > > I'm a bit confused about what you mean here. > > > > > > > > Digging through the code, it seems like > > > > WebViewChromiumFactoryProvider.initialize() is being called before > this > > point > > > is > > > > ever reached. I believe WebViewChromiumFactoryProvider.initialize() > > > successfully > > > > validates that it is given a CE context (or at least it will once I > fix > the > > > > concerns raised for that file). > > > > > > These are two totally unrelated things that use different contexts. > The very > > > first time a process calls any function relating to the webview > > implementation, > > > we go through WebViewChromiumFactoryProvider.initialize(), which does > *not* > > > receive any context whatsoever, and therefore has to retrieve one for > itself > > by > > > using a @SystemApi framework method that gives us the Application > object for > > the > > > application in this process. In all normal apps, the Application > object is a > > CE > > ^^^^^^^^^^^^^^^^ > > directbootaware is an application attribute, so I think even in > non-system > apps, > > the application context can be DE if they add this attribute? > > In a normal app *running after unlock* the app context will always be CE > (whether the app is direct boot aware or not). But.. I'm actually not sure > what > it is if it's running before unlock? > > I think just refusing to convert the context unless the device is already > unlocked will cover this in either case, though. > > > > > > > context, but in system apps that default to DE, the Application is > also DE, > > > which is the problem we're trying to work around. This code is only > called > > once > > > in the entire lifetime of the process, and happens even if the app does > > > something unrelated to constructing an instance of WebView, such as > > interacting > > > with CookieManager via its global singleton. > > > > > > WebViewChromium's constructor here is called when the app actually > does "new > > > WebView(context)" and so at this point we have the specific context > provided > > to > > > us by the application for the current activity. We want to make sure > that > this > > > context is always a CE context, in all apps, no matter whether they > default > to > > > DE or not, and the current code already does that correctly - you > shouldn't > > > remove the check, it's not redundant. > > > > > > > So I agree that I should revert this diff, since the check does > appear to > be > > > > redundant. However, I don't see how a DE context could still slip > through, > > > since > > > > the check happens even earlier than this point. > > > > > > > > Please correct me if I'm mistaken and I'm overlooking an entry point > for > > > > creating WebViews. > > > > > > > > > https://codereview.chromium.org/2339343002/diff/60001/ > android_webview/glue/java/src/com/android/webview/chromium/ > WebViewChromium.java > > > File > > > > > > android_webview/glue/java/src/com/android/webview/chromium/ > WebViewChromium.java > > > (left): > > > > > > > > > https://codereview.chromium.org/2339343002/diff/60001/ > android_webview/glue/java/src/com/android/webview/chromium/ > WebViewChromium.java#oldcode117 > > > > > > android_webview/glue/java/src/com/android/webview/chromium/ > WebViewChromium.java:117: > > > > > > WebViewChromiumFactoryProvider.checkStorageIsNotDeviceProtect > ed(webView.getContext()); > > > Leave this as-is, it's correct. > > > > > > > > > https://codereview.chromium.org/2339343002/diff/60001/ > android_webview/glue/java/src/com/android/webview/chromium/ > WebViewChromiumFactoryProvider.java > > > File > > > > > > android_webview/glue/java/src/com/android/webview/chromium/ > WebViewChromiumFactoryProvider.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2339343002/diff/60001/ > android_webview/glue/java/src/com/android/webview/chromium/ > WebViewChromiumFactoryProvider.java#newcode264 > > > > > > android_webview/glue/java/src/com/android/webview/chromium/ > WebViewChromiumFactoryProvider.java:264: > > > if (!context.getSystemService(UserManager.class).isUserUnlocked()) { > > > I think you should be getting a lint warning about this line: the > version of > > > getSystemService that takes a Class and returns the correct type > (instead of > > the > > > old sucky version that takes a String and returns Object) is new in > API23 > and > > so > > > WebView can't expect it to be there in general. In this specific case, > it's > > fine > > > because the code is guarded by a version check, but the linter is > supposed > to > > > point these out and require that you annotate the method to suppress > it - is > > > that not happening for you or am I mistaken? > > > > https://codereview.chromium.org/2339343002/ > -- 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.
On 2016/09/22 17:47:48, chromium-reviews wrote: > agreed Well the message above was me. Sent from google account by mistake :( > > On Thu, Sep 22, 2016 at 10:34 AM, <mailto:torne@chromium.org> wrote: > > > On 2016/09/22 17:19:45, sgurun wrote: > > > On 2016/09/22 09:59:54, Torne wrote: > > > > > > > > > https://codereview.chromium.org/2339343002/diff/40001/ > > android_webview/glue/java/src/com/android/webview/chromium/ > > WebViewChromium.java > > > > File > > > > > > > > > android_webview/glue/java/src/com/android/webview/chromium/ > > WebViewChromium.java > > > > (right): > > > > > > > > > > > > > https://codereview.chromium.org/2339343002/diff/40001/ > > android_webview/glue/java/src/com/android/webview/chromium/ > > WebViewChromium.java#newcode117 > > > > > > > > > android_webview/glue/java/src/com/android/webview/chromium/ > > WebViewChromium.java:117: > > > > Context ctx = > > > WebViewChromiumFactoryProvider.switchToCredentialProtectedStorage( > > > > On 2016/09/22 04:21:12, Nate Fischer wrote: > > > > > I'm a bit confused about what you mean here. > > > > > > > > > > Digging through the code, it seems like > > > > > WebViewChromiumFactoryProvider.initialize() is being called before > > this > > > point > > > > is > > > > > ever reached. I believe WebViewChromiumFactoryProvider.initialize() > > > > successfully > > > > > validates that it is given a CE context (or at least it will once I > > fix > > the > > > > > concerns raised for that file). > > > > > > > > These are two totally unrelated things that use different contexts. > > The very > > > > first time a process calls any function relating to the webview > > > implementation, > > > > we go through WebViewChromiumFactoryProvider.initialize(), which does > > *not* > > > > receive any context whatsoever, and therefore has to retrieve one for > > itself > > > by > > > > using a @SystemApi framework method that gives us the Application > > object for > > > the > > > > application in this process. In all normal apps, the Application > > object is a > > > CE > > > ^^^^^^^^^^^^^^^^ > > > directbootaware is an application attribute, so I think even in > > non-system > > apps, > > > the application context can be DE if they add this attribute? > > > > In a normal app *running after unlock* the app context will always be CE > > (whether the app is direct boot aware or not). But.. I'm actually not sure > > what > > it is if it's running before unlock? > > > > I think just refusing to convert the context unless the device is already > > unlocked will cover this in either case, though. > > > > > > > > > > > context, but in system apps that default to DE, the Application is > > also DE, > > > > which is the problem we're trying to work around. This code is only > > called > > > once > > > > in the entire lifetime of the process, and happens even if the app does > > > > something unrelated to constructing an instance of WebView, such as > > > interacting > > > > with CookieManager via its global singleton. > > > > > > > > WebViewChromium's constructor here is called when the app actually > > does "new > > > > WebView(context)" and so at this point we have the specific context > > provided > > > to > > > > us by the application for the current activity. We want to make sure > > that > > this > > > > context is always a CE context, in all apps, no matter whether they > > default > > to > > > > DE or not, and the current code already does that correctly - you > > shouldn't > > > > remove the check, it's not redundant. > > > > > > > > > So I agree that I should revert this diff, since the check does > > appear to > > be > > > > > redundant. However, I don't see how a DE context could still slip > > through, > > > > since > > > > > the check happens even earlier than this point. > > > > > > > > > > Please correct me if I'm mistaken and I'm overlooking an entry point > > for > > > > > creating WebViews. > > > > > > > > > > > > > https://codereview.chromium.org/2339343002/diff/60001/ > > android_webview/glue/java/src/com/android/webview/chromium/ > > WebViewChromium.java > > > > File > > > > > > > > > android_webview/glue/java/src/com/android/webview/chromium/ > > WebViewChromium.java > > > > (left): > > > > > > > > > > > > > https://codereview.chromium.org/2339343002/diff/60001/ > > android_webview/glue/java/src/com/android/webview/chromium/ > > WebViewChromium.java#oldcode117 > > > > > > > > > android_webview/glue/java/src/com/android/webview/chromium/ > > WebViewChromium.java:117: > > > > > > > > > WebViewChromiumFactoryProvider.checkStorageIsNotDeviceProtect > > ed(webView.getContext()); > > > > Leave this as-is, it's correct. > > > > > > > > > > > > > https://codereview.chromium.org/2339343002/diff/60001/ > > android_webview/glue/java/src/com/android/webview/chromium/ > > WebViewChromiumFactoryProvider.java > > > > File > > > > > > > > > android_webview/glue/java/src/com/android/webview/chromium/ > > WebViewChromiumFactoryProvider.java > > > > (right): > > > > > > > > > > > > > https://codereview.chromium.org/2339343002/diff/60001/ > > android_webview/glue/java/src/com/android/webview/chromium/ > > WebViewChromiumFactoryProvider.java#newcode264 > > > > > > > > > android_webview/glue/java/src/com/android/webview/chromium/ > > WebViewChromiumFactoryProvider.java:264: > > > > if (!context.getSystemService(UserManager.class).isUserUnlocked()) { > > > > I think you should be getting a lint warning about this line: the > > version of > > > > getSystemService that takes a Class and returns the correct type > > (instead of > > > the > > > > old sucky version that takes a String and returns Object) is new in > > API23 > > and > > > so > > > > WebView can't expect it to be there in general. In this specific case, > > it's > > > fine > > > > because the code is guarded by a version check, but the linter is > > supposed > > to > > > > point these out and require that you annotate the method to suppress > > it - is > > > > that not happening for you or am I mistaken? > > > > > > > > https://codereview.chromium.org/2339343002/ > > > > -- > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
On 2016/09/22 at 17:49:30, sgurun wrote: > On 2016/09/22 17:47:48, chromium-reviews wrote: > > agreed > > Well the message above was me. Sent from google account by mistake :( > > > > > > On Thu, Sep 22, 2016 at 10:34 AM, <mailto:torne@chromium.org> wrote: > > > > > On 2016/09/22 17:19:45, sgurun wrote: > > > > On 2016/09/22 09:59:54, Torne wrote: > > > > > > > > > > > > https://codereview.chromium.org/2339343002/diff/40001/ > > > android_webview/glue/java/src/com/android/webview/chromium/ > > > WebViewChromium.java > > > > > File > > > > > > > > > > > > android_webview/glue/java/src/com/android/webview/chromium/ > > > WebViewChromium.java > > > > > (right): > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2339343002/diff/40001/ > > > android_webview/glue/java/src/com/android/webview/chromium/ > > > WebViewChromium.java#newcode117 > > > > > > > > > > > > android_webview/glue/java/src/com/android/webview/chromium/ > > > WebViewChromium.java:117: > > > > > Context ctx = > > > > WebViewChromiumFactoryProvider.switchToCredentialProtectedStorage( > > > > > On 2016/09/22 04:21:12, Nate Fischer wrote: > > > > > > I'm a bit confused about what you mean here. > > > > > > > > > > > > Digging through the code, it seems like > > > > > > WebViewChromiumFactoryProvider.initialize() is being called before > > > this > > > > point > > > > > is > > > > > > ever reached. I believe WebViewChromiumFactoryProvider.initialize() > > > > > successfully > > > > > > validates that it is given a CE context (or at least it will once I > > > fix > > > the > > > > > > concerns raised for that file). > > > > > > > > > > These are two totally unrelated things that use different contexts. > > > The very > > > > > first time a process calls any function relating to the webview > > > > implementation, > > > > > we go through WebViewChromiumFactoryProvider.initialize(), which does > > > *not* > > > > > receive any context whatsoever, and therefore has to retrieve one for > > > itself > > > > by > > > > > using a @SystemApi framework method that gives us the Application > > > object for > > > > the > > > > > application in this process. In all normal apps, the Application > > > object is a > > > > CE > > > > ^^^^^^^^^^^^^^^^ > > > > directbootaware is an application attribute, so I think even in > > > non-system > > > apps, > > > > the application context can be DE if they add this attribute? > > > > > > In a normal app *running after unlock* the app context will always be CE > > > (whether the app is direct boot aware or not). But.. I'm actually not sure > > > what > > > it is if it's running before unlock? > > > > > > I think just refusing to convert the context unless the device is already > > > unlocked will cover this in either case, though. > > > > > > > > > > > > > > > context, but in system apps that default to DE, the Application is > > > also DE, > > > > > which is the problem we're trying to work around. This code is only > > > called > > > > once > > > > > in the entire lifetime of the process, and happens even if the app does > > > > > something unrelated to constructing an instance of WebView, such as > > > > interacting > > > > > with CookieManager via its global singleton. > > > > > > > > > > WebViewChromium's constructor here is called when the app actually > > > does "new > > > > > WebView(context)" and so at this point we have the specific context > > > provided > > > > to > > > > > us by the application for the current activity. We want to make sure > > > that > > > this > > > > > context is always a CE context, in all apps, no matter whether they > > > default > > > to > > > > > DE or not, and the current code already does that correctly - you > > > shouldn't > > > > > remove the check, it's not redundant. Thanks for the explanation. This makes much more sense now. I believe I understand how to proceed correctly. > > > > > > > > > > > So I agree that I should revert this diff, since the check does > > > appear to > > > be > > > > > > redundant. However, I don't see how a DE context could still slip > > > through, > > > > > since > > > > > > the check happens even earlier than this point. > > > > > > > > > > > > Please correct me if I'm mistaken and I'm overlooking an entry point > > > for > > > > > > creating WebViews. > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2339343002/diff/60001/ > > > android_webview/glue/java/src/com/android/webview/chromium/ > > > WebViewChromium.java > > > > > File > > > > > > > > > > > > android_webview/glue/java/src/com/android/webview/chromium/ > > > WebViewChromium.java > > > > > (left): > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2339343002/diff/60001/ > > > android_webview/glue/java/src/com/android/webview/chromium/ > > > WebViewChromium.java#oldcode117 > > > > > > > > > > > > android_webview/glue/java/src/com/android/webview/chromium/ > > > WebViewChromium.java:117: > > > > > > > > > > > > WebViewChromiumFactoryProvider.checkStorageIsNotDeviceProtect > > > ed(webView.getContext()); > > > > > Leave this as-is, it's correct. > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2339343002/diff/60001/ > > > android_webview/glue/java/src/com/android/webview/chromium/ > > > WebViewChromiumFactoryProvider.java > > > > > File > > > > > > > > > > > > android_webview/glue/java/src/com/android/webview/chromium/ > > > WebViewChromiumFactoryProvider.java > > > > > (right): > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2339343002/diff/60001/ > > > android_webview/glue/java/src/com/android/webview/chromium/ > > > WebViewChromiumFactoryProvider.java#newcode264 > > > > > > > > > > > > android_webview/glue/java/src/com/android/webview/chromium/ > > > WebViewChromiumFactoryProvider.java:264: > > > > > if (!context.getSystemService(UserManager.class).isUserUnlocked()) { > > > > > I think you should be getting a lint warning about this line: the > > > version of > > > > > getSystemService that takes a Class and returns the correct type > > > (instead of > > > > the > > > > > old sucky version that takes a String and returns Object) is new in > > > API23 > > > and > > > > so > > > > > WebView can't expect it to be there in general. In this specific case, > > > it's > > > > fine > > > > > because the code is guarded by a version check, but the linter is > > > supposed > > > to > > > > > point these out and require that you annotate the method to suppress > > > it - is > > > > > that not happening for you or am I mistaken? > > > > > > > > > > > > https://codereview.chromium.org/2339343002/ > > > > > > > -- > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2339343002/diff/60001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java (left): https://codereview.chromium.org/2339343002/diff/60001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java:117: WebViewChromiumFactoryProvider.checkStorageIsNotDeviceProtected(webView.getContext()); On 2016/09/22 at 09:59:54, Torne wrote: > Leave this as-is, it's correct. Ok. I think I understand you clearly now. I think I'll refactor this into two functions: one that checks if the context is DE (just like before) and another function which switches to a CE context if it's available. Then we can keep the check here, just as before, and I'll put the switching logic in WebViewChromiumFactoryProvider. https://codereview.chromium.org/2339343002/diff/60001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2339343002/diff/60001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:264: if (!context.getSystemService(UserManager.class).isUserUnlocked()) { On 2016/09/22 at 09:59:54, Torne wrote: > I think you should be getting a lint warning about this line: the version of getSystemService that takes a Class and returns the correct type (instead of the old sucky version that takes a String and returns Object) is new in API23 and so WebView can't expect it to be there in general. In this specific case, it's fine because the code is guarded by a version check, but the linter is supposed to point these out and require that you annotate the method to suppress it - is that not happening for you or am I mistaken? I don't think I received a lint warning. I was able to run `git cl format` and `git cl upload` without receiving any warning. Should I make any change here (such as an annotation) to mark this as the new version? Or will the lint warnings pop up as the result of some other command? Thanks.
PTAL
LGTM assuming we aren't going to trigger the warnings about incompatible method calls.. https://codereview.chromium.org/2339343002/diff/60001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/2339343002/diff/60001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:264: if (!context.getSystemService(UserManager.class).isUserUnlocked()) { On 2016/09/22 19:47:49, Nate Fischer wrote: > On 2016/09/22 at 09:59:54, Torne wrote: > > I think you should be getting a lint warning about this line: the version of > getSystemService that takes a Class and returns the correct type (instead of the > old sucky version that takes a String and returns Object) is new in API23 and so > WebView can't expect it to be there in general. In this specific case, it's fine > because the code is guarded by a version check, but the linter is supposed to > point these out and require that you annotate the method to suppress it - is > that not happening for you or am I mistaken? > > I don't think I received a lint warning. I was able to run `git cl format` and > `git cl upload` without receiving any warning. Should I make any change here > (such as an annotation) to mark this as the new version? Or will the lint > warnings pop up as the result of some other command? Thanks. I think they happen during the build, not in presubmit? I'm really not sure, though. It's possible it doesn't detect this case for some reason, or that I'm mistaken about how it works, I don't write java code that calls android APIs that frequently. There's a method annotation to indicate that this method uses a certain minimum API version and this is okay..
The CQ bit was checked by ntfschr@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
On 2016/09/23 at 11:13:15, torne wrote: > LGTM assuming we aren't going to trigger the warnings about incompatible method calls.. > > https://codereview.chromium.org/2339343002/diff/60001/android_webview/glue/ja... > File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): > > https://codereview.chromium.org/2339343002/diff/60001/android_webview/glue/ja... > android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:264: if (!context.getSystemService(UserManager.class).isUserUnlocked()) { > On 2016/09/22 19:47:49, Nate Fischer wrote: > > On 2016/09/22 at 09:59:54, Torne wrote: > > > I think you should be getting a lint warning about this line: the version of > > getSystemService that takes a Class and returns the correct type (instead of the > > old sucky version that takes a String and returns Object) is new in API23 and so > > WebView can't expect it to be there in general. In this specific case, it's fine > > because the code is guarded by a version check, but the linter is supposed to > > point these out and require that you annotate the method to suppress it - is > > that not happening for you or am I mistaken? > > > > I don't think I received a lint warning. I was able to run `git cl format` and > > `git cl upload` without receiving any warning. Should I make any change here > > (such as an annotation) to mark this as the new version? Or will the lint > > warnings pop up as the result of some other command? Thanks. > > I think they happen during the build, not in presubmit? I'm really not sure, though. It's possible it doesn't detect this case for some reason, or that I'm mistaken about how it works, I don't write java code that calls android APIs that frequently. > > There's a method annotation to indicate that this method uses a certain minimum API version and this is okay.. Just built again to verify. I can't produce any lint warnings that weren't already there (WebIconDatabaseAdapter has 4 lint warnings, but I didn't touch that file and the warnings appear regardless of this change).
The CQ bit was checked by ntfschr@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 ========== Android webview tries to switch to CE context If Webview is started in an application using defaultToDeviceProtectedStorage="true", it will first try to switch to a CE context. If it's unable to switch contexts, it throws an exception. BUG=645921 ========== to ========== Android webview tries to switch to CE context If Webview is started in an application using defaultToDeviceProtectedStorage="true", it will first try to switch to a CE context. If it's unable to switch contexts, it throws an exception. BUG=645921 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Android webview tries to switch to CE context If Webview is started in an application using defaultToDeviceProtectedStorage="true", it will first try to switch to a CE context. If it's unable to switch contexts, it throws an exception. BUG=645921 ========== to ========== Android webview tries to switch to CE context If Webview is started in an application using defaultToDeviceProtectedStorage="true", it will first try to switch to a CE context. If it's unable to switch contexts, it throws an exception. BUG=645921 Committed: https://crrev.com/18dad59572f1ed74069538bde20f25a4a8b3e586 Cr-Commit-Position: refs/heads/master@{#420730} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/18dad59572f1ed74069538bde20f25a4a8b3e586 Cr-Commit-Position: refs/heads/master@{#420730} |
