|
|
Created:
6 years, 11 months ago by guangzhen.li Modified:
6 years, 10 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionFix the crash issue caused by vibrate permission
The app will be crashed when using vibrate API, without VIBRATE permission specified.
Catch the security exception instead of crash should be more friendly.
BUG=338151
TEST=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251289
Patch Set 1 #Patch Set 2 : Check permissin instead of try-catch #
Total comments: 3
Patch Set 3 : Change error message, and cleaner condition #Patch Set 4 : Add myself in the AUTHORS file #Messages
Total messages: 31 (0 generated)
This looks pretty reasonable, but I'll defer to mvanouwerkerk@ who originally wrote this. On a related note, mvanouwerkerk@ do you know if we have a standard approach for functionality conditional on permissions? Do we prefer explicit exception handling, or something like Context.checkCallingorSelfPermission() (see this line from AccessibilityInjector.java, https://code.google.com/p/chromium/codesearch#chromium/src/content/public/and...)
On 2014/01/26 15:49:10, jdduke wrote: > This looks pretty reasonable, but I'll defer to mvanouwerkerk@ who originally > wrote this. > > On a related note, mvanouwerkerk@ do you know if we have a standard approach for > functionality conditional on permissions? Do we prefer explicit exception > handling, or something like Context.checkCallingorSelfPermission() (see this > line from AccessibilityInjector.java, > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/and...) When does this occur? If the user has revoked the vibrate permission explicitly? Or for embedders that do not include this permission in their manifest? In any case, it might be worth initializing something like: mHasVibratePermission = context.checkCallingOrSelfPermission( android.Manifest.permission.VIBRATE) == PackageManager.PERMISSION_GRANTED; in the constructor, and then early returning in vibrate()/cancelVibration() to avoid spammy error messages.
On 2014/01/27 01:13:39, jdduke wrote: > On 2014/01/26 15:49:10, jdduke wrote: > > This looks pretty reasonable, but I'll defer to mvanouwerkerk@ who originally > > wrote this. > > > > On a related note, mvanouwerkerk@ do you know if we have a standard approach > for > > functionality conditional on permissions? Do we prefer explicit exception > > handling, or something like Context.checkCallingorSelfPermission() (see this > > line from AccessibilityInjector.java, > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/and...) > > When does this occur? If the user has revoked the vibrate permission explicitly? > Or for embedders that do not include this permission in their manifest? > > In any case, it might be worth initializing something like: > > mHasVibratePermission = context.checkCallingOrSelfPermission( > android.Manifest.permission.VIBRATE) == > PackageManager.PERMISSION_GRANTED; > > in the constructor, and then early returning in vibrate()/cancelVibration() to > avoid spammy error messages. For VibrationProvider.vibrate(), it's from html5 api with navigator.vibrate(1000); There is no permission check through the implementation on Android. The browser has vibrate permission, but for other use case, It's possible to exclude the permission. According to AccessibilityInjector.java, check permission is better than try-catch. :-)
Thanks for tackling this issue, I wasn't aware this was a concern! I like Jared Duke's suggestion is good, i.e. this: mHasVibratePermission = context.checkCallingOrSelfPermission( android.Manifest.permission.VIBRATE) == PackageManager.PERMISSION_GRANTED; Ben: do we need to do more more to handle this consistently for webview? Pavel: how can we log once to the inspector console to tell the developer there's a missing permission? Assuming we'd log this once from the VibrationProvider constructor and not from subsequent calls to #vibrate and #cancelVibration.
On 2014/01/27 16:36:51, Michael van Ouwerkerk wrote: > Thanks for tackling this issue, I wasn't aware this was a concern! > > I like Jared Duke's suggestion is good, i.e. this: > mHasVibratePermission = context.checkCallingOrSelfPermission( > android.Manifest.permission.VIBRATE) == > PackageManager.PERMISSION_GRANTED; > > Ben: do we need to do more more to handle this consistently for webview? > > Pavel: how can we log once to the inspector console to tell the developer > there's a missing permission? Assuming we'd log this once from the > VibrationProvider constructor and not from subsequent calls to #vibrate and > #cancelVibration. Hi Michael, I have investigated to print log to inspector console in java layer, but it seems unavailable. One simple solution should be using the way like ContentViewCore::addJavascriptInterface(), and use js to print permission warning? I have upload another patch which using the same way as AccessibilityInjector.
I discussed the logging options with Pavel, and I summarized his advice in this bug: http://crbug.com/338690 Basically, if we want to log to the devtools console we need to do some more work. I think that can be done independently from this change. https://codereview.chromium.org/143673004/diff/80001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/VibrationProvider.java (right): https://codereview.chromium.org/143673004/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/VibrationProvider.java:35: if (mHasVibratePermission) mVibrator.vibrate(milliseconds); This condition would be cleaner if merged into the other if statement: if (mAudioManager.getRingerMode() != AudioManager.RINGER_MODE_SILENT && mHasVibratePermission) { mVibrator.vibrate(milliseconds); } https://codereview.chromium.org/143673004/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/VibrationProvider.java:50: Log.e(TAG, "Caught security exception, requires VIBRATE permission."); This code did not actually catch an exception, please rephrase this message.
https://codereview.chromium.org/143673004/diff/80001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/VibrationProvider.java (right): https://codereview.chromium.org/143673004/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/VibrationProvider.java:50: Log.e(TAG, "Caught security exception, requires VIBRATE permission."); On 2014/01/28 15:09:04, Michael van Ouwerkerk wrote: > This code did not actually catch an exception, please rephrase this message. Also, could we make this a warning, Log.w, instead of an error?
Thanks for your comments. I'm currently in holiday till Feb. 6. Will fix these when back to office. Br. Guangzhen > 在 2014年1月28日,下午11:09,"mvanouwerkerk@chromium.org" <mvanouwerkerk@chromium.org> 写道: > > I discussed the logging options with Pavel, and I summarized his advice in this > bug: > http://crbug.com/338690 > > Basically, if we want to log to the devtools console we need to do some more > work. I think that can be done independently from this change. > > > > > https://codereview.chromium.org/143673004/diff/80001/content/public/android/j... > File > content/public/android/java/src/org/chromium/content/browser/VibrationProvider.java > (right): > > https://codereview.chromium.org/143673004/diff/80001/content/public/android/j... > content/public/android/java/src/org/chromium/content/browser/VibrationProvider.java:35: > if (mHasVibratePermission) mVibrator.vibrate(milliseconds); > This condition would be cleaner if merged into the other if statement: > > if (mAudioManager.getRingerMode() != AudioManager.RINGER_MODE_SILENT && > mHasVibratePermission) { > mVibrator.vibrate(milliseconds); > } > > https://codereview.chromium.org/143673004/diff/80001/content/public/android/j... > content/public/android/java/src/org/chromium/content/browser/VibrationProvider.java:50: > Log.e(TAG, "Caught security exception, requires VIBRATE permission."); > This code did not actually catch an exception, please rephrase this > message. > > https://codereview.chromium.org/143673004/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thanks; this looks like the right approach for WebView.
On 2014/01/28 15:45:32, jdduke wrote: > https://codereview.chromium.org/143673004/diff/80001/content/public/android/j... > File > content/public/android/java/src/org/chromium/content/browser/VibrationProvider.java > (right): > > https://codereview.chromium.org/143673004/diff/80001/content/public/android/j... > content/public/android/java/src/org/chromium/content/browser/VibrationProvider.java:50: > Log.e(TAG, "Caught security exception, requires VIBRATE permission."); > On 2014/01/28 15:09:04, Michael van Ouwerkerk wrote: > > This code did not actually catch an exception, please rephrase this message. > > Also, could we make this a warning, Log.w, instead of an error? Hi @jdduke, New patch updated, fixed as comments above, Would you please help to review again?
Thanks! lgtm
lgtm
The CQ bit was checked by guangzhen.li@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/guangzhen.li@intel.com/143673004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
Sorry, lgtm!
The CQ bit was checked by jdduke@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/guangzhen.li@intel.com/143673004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
The CQ bit was checked by guangzhen.li@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/guangzhen.li@intel.com/143673004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
I think isn't going so submit until you are a registered author. ** Presubmit Warnings ** guangzhen.li@intel.com is not in AUTHORS file. If you are a new contributor, please visit http://www.chromium.org/developers/contributing-code and read the "Legal" section
The CQ bit was checked by guangzhen.li@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/guangzhen.li@intel.com/143673004/540001
On 2014/02/13 10:36:28, Michael van Ouwerkerk wrote: > I think isn't going so submit until you are a registered author. > > ** Presubmit Warnings ** > mailto:guangzhen.li@intel.com is not in AUTHORS file. If you are a new contributor, > please visit > http://www.chromium.org/developers/contributing-code and read the "Legal" > section Thanks, I have signed the CLA and added myself to AUTHORS file. :-)
Message was sent while issue was closed.
Change committed as 251289 |