Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2147543003:
Check if there is a url before checking to launch activity inside CCT (Closed)
DescriptionCheck if there is a url before checking to launch activity inside CCT
In some cases, on a user click in a page, we might get an intent with
a null data string. In these cases we are crashing while checking
whether the uri has an accepted scheme.
This adds an early return to avoid the crash.
BUG=627503
Committed: https://crrev.com/4128ae6b96c70dc721b4ae5ce9d7707169e3b348
Cr-Commit-Position: refs/heads/master@{#410769}
Patch Set 1 #
Total comments: 2
Patch Set 2 : ianwen@'s comments #Patch Set 3 : mariakhomenko@'s comment #Messages
Total messages: 17 (5 generated)
yusufo@chromium.org changed reviewers: + ianwen@chromium.org
One thing I am not sure about: Should we check for empty string as well? I would imagine there are intents we get that are valid outside with an empty string and I wanted to avoid early returning for those here. That was why I didn't use TextUtils.isEmpty Ian, WDYT?
https://codereview.chromium.org/2147543003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabDelegateFactory.java (right): https://codereview.chromium.org/2147543003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabDelegateFactory.java:58: if (intent.getDataString() == null) return false; Shall we change shouldOverrideUrlLoading() in ExternalNavigationHandler, so that if the data string will be null after Intent.parseUri(), we directly return "NO_OVERRIDE"?
yusufo@chromium.org changed reviewers: + nyquist@chromium.org
nyquist@ for externalnav/ https://codereview.chromium.org/2147543003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabDelegateFactory.java (right): https://codereview.chromium.org/2147543003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabDelegateFactory.java:58: if (intent.getDataString() == null) return false; On 2016/07/14 17:31:33, Ian Wen wrote: > Shall we change shouldOverrideUrlLoading() in ExternalNavigationHandler, so that > if the data string will be null after Intent.parseUri(), we directly return > "NO_OVERRIDE"? Done.
ianwen@chromium.org changed reviewers: + mariakhomenko@chromium.org
Add Maria to the CL. I wonder what it means that data string of an intent is null. Could it be that ExternalNavigationHandler #100, the exception check is not enough, and we need to do a null check as well?
Add Maria to the CL. I wonder what it means that data string of an intent is null. Could it be that ExternalNavigationHandler #100, the exception check is not enough, and we need to do a null check as well?
Happy to wait for Maria on this one and rubberstamp at the end. Please ping me if you want me to do something else.
On 2016/07/15 21:19:15, nyquist wrote: > Happy to wait for Maria on this one and rubberstamp at the end. Please ping me > if you want me to do something else. At the beginning of ExternalNavigationHandler we do this: intent = Intent.parseUri(params.getUrl(), Intent.URI_INTENT_SCHEME); This function throws if params.getUrl() is null, so we know if we get to startActivityIfNeeded, then it wasn't null. However, the parse function doesn't guarantee to set DataString on the intent it creates (this is dependent on the URL type we were parsing). So I don't think we should be assuming DataString is not null (and checking for it there). I think this can happen on some non http(s) URIs. You should probably be making the scheme check on the value stored in params.getUrl() instead. I think you can get that from the intent object, by calling intent.toUri(0) and then doing the checks on that in the CustomTabs code.
On 2016/07/18 17:40:21, Maria wrote: > On 2016/07/15 21:19:15, nyquist wrote: > > Happy to wait for Maria on this one and rubberstamp at the end. Please ping me > > if you want me to do something else. > > At the beginning of ExternalNavigationHandler we do this: > > intent = Intent.parseUri(params.getUrl(), Intent.URI_INTENT_SCHEME); > > This function throws if params.getUrl() is null, so we know if we get to > startActivityIfNeeded, then it wasn't null. > > However, the parse function doesn't guarantee to set DataString on the intent it > creates (this is dependent on the URL type we were parsing). So I don't think we > should be assuming DataString is not null (and checking for it there). I think > this can happen on some non http(s) URIs. > > You should probably be making the scheme check on the value stored in > params.getUrl() instead. I think you can get that from the intent object, by > calling intent.toUri(0) and then doing the checks on that in the CustomTabs > code. Done
lgtm
The CQ bit was checked by yusufo@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.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Check if there is a url before checking to launch activity inside CCT In some cases, on a user click in a page, we might get an intent with a null data string. In these cases we are crashing while checking whether the uri has an accepted scheme. This adds an early return to avoid the crash. BUG=627503 ========== to ========== Check if there is a url before checking to launch activity inside CCT In some cases, on a user click in a page, we might get an intent with a null data string. In these cases we are crashing while checking whether the uri has an accepted scheme. This adds an early return to avoid the crash. BUG=627503 Committed: https://crrev.com/4128ae6b96c70dc721b4ae5ce9d7707169e3b348 Cr-Commit-Position: refs/heads/master@{#410769} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/4128ae6b96c70dc721b4ae5ce9d7707169e3b348 Cr-Commit-Position: refs/heads/master@{#410769} |
