Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2440613003:
[Cronet] Enforce implementation does not call through API classes (Closed)
|
Created:
4 years, 2 months ago by pauljensen Modified:
3 years, 11 months ago Reviewers:
kapishnikov CC:
chromium-reviews, cbentzel+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Cronet] Enforce implementation does not call through API classes
Such calls could fail if an older API is used that
does not contain newer methods. Calls should instead call through a
wrapper class from VersionSafeCallbacks.
R=kapishnikov
BUG=629299
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
Committed: https://crrev.com/8c9905454fbeadbf095b2af75a7faaccc1beede2
Cr-Commit-Position: refs/heads/master@{#441186}
Patch Set 1 #Patch Set 2 : finish #
Total comments: 18
Patch Set 3 : add tests #Patch Set 4 : address comments, get test running #
Total comments: 7
Patch Set 5 : actually check unittest result #Patch Set 6 : address comments #
Total comments: 4
Patch Set 7 : address comments #Patch Set 8 : rebase #Patch Set 9 : avoid NetworkExceptionImpl calling through API inadvertently #
Total comments: 3
Patch Set 10 : add exception for Exception.getMessage() call #
Dependent Patchsets: Messages
Total messages: 31 (13 generated)
Description was changed from ========== [Cronet] Enforce API restrictions at build time ========== to ========== [Cronet] Enforce API restrictions at build time CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
Description was changed from ========== [Cronet] Enforce API restrictions at build time CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== [Cronet] Enforce implementation does not call through API classes Such calls could fail if an older API is used that does not contain newer methods. Calls should instead call through a wrapper class from VersionSafeCallbacks. R=kapishnikov BUG=629299 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
pauljensen@chromium.org changed reviewers: + kapishnikov@chromium.org
Andrei, PTAL, thank you!
Paul, the change looks great. My only concern is how safe/stable this approach is. If the output from the javap changes for some reason, the script may stop detecting the problems while returning 'success'. I guess we should add a test with a simple java class that has all elements that we are detecting in the script. Another approach could be using a byte code analysis library. It should be more safe but will require more efforts to implement. https://codereview.chromium.org/2440613003/diff/20001/components/cronet/tools... File components/cronet/tools/api_static_checks.py (right): https://codereview.chromium.org/2440613003/diff/20001/components/cronet/tools... components/cronet/tools/api_static_checks.py:23: # These regular expressions catch the beginning of lines that declar classes and declar -> declare https://codereview.chromium.org/2440613003/diff/20001/components/cronet/tools... components/cronet/tools/api_static_checks.py:25: CLASS_RE = re.compile(r'.*class ([^ ]*) .*{') I think '\' is missing before '{' since we want to literally match one '{' character. Otherwise the regular expression is not completely valid. https://codereview.chromium.org/2440613003/diff/20001/components/cronet/tools... components/cronet/tools/api_static_checks.py:78: if line[8:16] == ': invoke': This looks a little fragile. Can the output format change in the future? Should we have a test to detect such change? https://codereview.chromium.org/2440613003/diff/20001/components/cronet/tools... components/cronet/tools/api_static_checks.py:79: callee = line[54:].split(':')[0] 1. Can we add an assertion that |caller_class| and |caller_method| are not empty at this point? 2. Would it be more safe to get substring between "// Method " and ":"? '54' looks a little fragile. https://codereview.chromium.org/2440613003/diff/20001/components/cronet/tools... components/cronet/tools/api_static_checks.py:84: if callee_method == '"<init>"': Should we also track the usage of constructors, i.e. if we construct an API class from impl, should we do it through the wrappers? E.g., what will happen if we add a non-default constructor to an API class? Btw, are we constructing any API classes from the impl? https://codereview.chromium.org/2440613003/diff/20001/components/cronet/tools... components/cronet/tools/api_static_checks.py:123: for root, _, filenames in os.walk(temp_dir): Should |root| be renamed to |dirpath|? I guess |temp_dir| is the root. https://codereview.chromium.org/2440613003/diff/20001/components/cronet/tools... components/cronet/tools/api_static_checks.py:142: api_calls = [] Should we rename it to |bad_api_calls|? https://codereview.chromium.org/2440613003/diff/20001/components/cronet/tools... components/cronet/tools/api_static_checks.py:151: print 'ERROR: javap failed on ' + filenames I think it is not allowed to concatenate string with a list. Suggestion: print 'ERROR: javap failed on ' + ' '.join(filenames)
I added some tests, PTAL. https://codereview.chromium.org/2440613003/diff/20001/components/cronet/tools... File components/cronet/tools/api_static_checks.py (right): https://codereview.chromium.org/2440613003/diff/20001/components/cronet/tools... components/cronet/tools/api_static_checks.py:23: # These regular expressions catch the beginning of lines that declar classes and On 2016/12/01 20:34:17, kapishnikov wrote: > declar -> declare Done. https://codereview.chromium.org/2440613003/diff/20001/components/cronet/tools... components/cronet/tools/api_static_checks.py:25: CLASS_RE = re.compile(r'.*class ([^ ]*) .*{') On 2016/12/01 20:34:16, kapishnikov wrote: > I think '\' is missing before '{' since we want to literally match one '{' > character. Otherwise the regular expression is not completely valid. Done. https://codereview.chromium.org/2440613003/diff/20001/components/cronet/tools... components/cronet/tools/api_static_checks.py:78: if line[8:16] == ': invoke': On 2016/12/01 20:34:17, kapishnikov wrote: > This looks a little fragile. Can the output format change in the future? Should > we have a test to detect such change? I doubt the format will ever change; for example "man javap" shows this output format. I've added some tests that it keeps working. https://codereview.chromium.org/2440613003/diff/20001/components/cronet/tools... components/cronet/tools/api_static_checks.py:79: callee = line[54:].split(':')[0] On 2016/12/01 20:34:16, kapishnikov wrote: > 1. Can we add an assertion that |caller_class| and |caller_method| are not empty > at this point? > > 2. Would it be more safe to get substring between "// Method " and ":"? '54' > looks a little fragile. Done. https://codereview.chromium.org/2440613003/diff/20001/components/cronet/tools... components/cronet/tools/api_static_checks.py:84: if callee_method == '"<init>"': On 2016/12/01 20:34:17, kapishnikov wrote: > Should we also track the usage of constructors, i.e. if we construct an API > class from impl, should we do it through the wrappers? E.g., what will happen if > we add a non-default constructor to an API class? > > Btw, are we constructing any API classes from the impl? We construct plenty of API classes from the impl. We "extend" many API classes, and those constructors call the "super" constructors. We might want to track the constructor usage, but that is more complicated. I've added a TODO. https://codereview.chromium.org/2440613003/diff/20001/components/cronet/tools... components/cronet/tools/api_static_checks.py:123: for root, _, filenames in os.walk(temp_dir): On 2016/12/01 20:34:17, kapishnikov wrote: > Should |root| be renamed to |dirpath|? I guess |temp_dir| is the root. Done. https://codereview.chromium.org/2440613003/diff/20001/components/cronet/tools... components/cronet/tools/api_static_checks.py:142: api_calls = [] On 2016/12/01 20:34:16, kapishnikov wrote: > Should we rename it to |bad_api_calls|? Done. https://codereview.chromium.org/2440613003/diff/20001/components/cronet/tools... components/cronet/tools/api_static_checks.py:151: print 'ERROR: javap failed on ' + filenames On 2016/12/01 20:34:16, kapishnikov wrote: > I think it is not allowed to concatenate string with a list. Suggestion: > print 'ERROR: javap failed on ' + ' '.join(filenames) Done.
https://codereview.chromium.org/2440613003/diff/60001/components/cronet/tools... File components/cronet/tools/api_static_checks.py (right): https://codereview.chromium.org/2440613003/diff/60001/components/cronet/tools... components/cronet/tools/api_static_checks.py:32: ALLOWED_EXCEPTIONS = [ I think we should add the callee method signatures to the exception list in order to detect the case when an overloaded method is added to the API. https://codereview.chromium.org/2440613003/diff/60001/components/cronet/tools... components/cronet/tools/api_static_checks.py:79: callee = line.split(' // ')[1].split('Method ')[1].split(':')[0] Can we merge two '//' & 'Method' splits into one split? https://codereview.chromium.org/2440613003/diff/60001/components/cronet/tools... components/cronet/tools/api_static_checks.py:87: # TODO(pauljensen): Look into enforcing restricting constructor calls. Let's file a bug for that. I think it is important that we fix it before the integration.
https://codereview.chromium.org/2440613003/diff/60001/components/cronet/tools... File components/cronet/tools/api_static_checks.py (right): https://codereview.chromium.org/2440613003/diff/60001/components/cronet/tools... components/cronet/tools/api_static_checks.py:32: ALLOWED_EXCEPTIONS = [ On 2016/12/02 20:05:03, kapishnikov wrote: > I think we should add the callee method signatures to the exception list in > order to detect the case when an overloaded method is added to the API. Done. https://codereview.chromium.org/2440613003/diff/60001/components/cronet/tools... components/cronet/tools/api_static_checks.py:79: callee = line.split(' // ')[1].split('Method ')[1].split(':')[0] On 2016/12/02 20:05:03, kapishnikov wrote: > Can we merge two '//' & 'Method' splits into one split? No, when calling through an interface it's "InterfaceMethod", otherwise it's just "Method", so the text between "//" and "Method" isn't constant. Having the two splits()s better enforces that the output format isn't changing, i.e. it will fail if either is removed. https://codereview.chromium.org/2440613003/diff/60001/components/cronet/tools... components/cronet/tools/api_static_checks.py:87: # TODO(pauljensen): Look into enforcing restricting constructor calls. On 2016/12/02 20:05:03, kapishnikov wrote: > Let's file a bug for that. I think it is important that we fix it before the > integration. Done, and updated comment.
LGTM https://codereview.chromium.org/2440613003/diff/20001/components/cronet/tools... File components/cronet/tools/api_static_checks.py (right): https://codereview.chromium.org/2440613003/diff/20001/components/cronet/tools... components/cronet/tools/api_static_checks.py:78: if line[8:16] == ': invoke': On 2016/12/02 18:24:14, pauljensen wrote: > On 2016/12/01 20:34:17, kapishnikov wrote: > > This looks a little fragile. Can the output format change in the future? > Should > > we have a test to detect such change? > > I doubt the format will ever change; for example "man javap" shows this output > format. I've added some tests that it keeps working. Acknowledged. https://codereview.chromium.org/2440613003/diff/20001/components/cronet/tools... components/cronet/tools/api_static_checks.py:84: if callee_method == '"<init>"': On 2016/12/02 18:24:14, pauljensen wrote: > On 2016/12/01 20:34:17, kapishnikov wrote: > > Should we also track the usage of constructors, i.e. if we construct an API > > class from impl, should we do it through the wrappers? E.g., what will happen > if > > we add a non-default constructor to an API class? > > > > Btw, are we constructing any API classes from the impl? > > We construct plenty of API classes from the impl. We "extend" many API classes, > and those constructors call the "super" constructors. > We might want to track the constructor usage, but that is more complicated. > I've added a TODO. Acknowledged. https://codereview.chromium.org/2440613003/diff/60001/components/cronet/tools... File components/cronet/tools/api_static_checks.py (right): https://codereview.chromium.org/2440613003/diff/60001/components/cronet/tools... components/cronet/tools/api_static_checks.py:79: callee = line.split(' // ')[1].split('Method ')[1].split(':')[0] On 2016/12/16 18:45:57, pauljensen wrote: > On 2016/12/02 20:05:03, kapishnikov wrote: > > Can we merge two '//' & 'Method' splits into one split? > > No, when calling through an interface it's "InterfaceMethod", otherwise it's > just "Method", so the text between "//" and "Method" isn't constant. Having the > two splits()s better enforces that the output format isn't changing, i.e. it > will fail if either is removed. Acknowledged. https://codereview.chromium.org/2440613003/diff/100001/components/cronet/tool... File components/cronet/tools/api_static_checks.py (right): https://codereview.chromium.org/2440613003/diff/100001/components/cronet/tool... components/cronet/tools/api_static_checks.py:136: package += '/' Same comment as in 2544043002 https://codereview.chromium.org/2440613003/diff/100001/components/cronet/tool... components/cronet/tools/api_static_checks.py:140: api_classes += [package + classname] Same comment as in 2544043002
https://codereview.chromium.org/2440613003/diff/100001/components/cronet/tool... File components/cronet/tools/api_static_checks.py (right): https://codereview.chromium.org/2440613003/diff/100001/components/cronet/tool... components/cronet/tools/api_static_checks.py:136: package += '/' On 2016/12/28 21:54:41, kapishnikov wrote: > Same comment as in 2544043002 Done. https://codereview.chromium.org/2440613003/diff/100001/components/cronet/tool... components/cronet/tools/api_static_checks.py:140: api_classes += [package + classname] On 2016/12/28 21:54:41, kapishnikov wrote: > Same comment as in 2544043002 Done, I had to wrap it in os.path.normpath() because os.path.relpath() returns '.' instead of an empty string.
The CQ bit was checked by pauljensen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kapishnikov@chromium.org Link to the patchset: https://codereview.chromium.org/2440613003/#ps120001 (title: "address comments")
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by pauljensen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kapishnikov@chromium.org Link to the patchset: https://codereview.chromium.org/2440613003/#ps140001 (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_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
https://codereview.chromium.org/2440613003/diff/160001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/impl/NetworkExceptionImpl.java (right): https://codereview.chromium.org/2440613003/diff/160001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/impl/NetworkExceptionImpl.java:66: StringBuilder b = new StringBuilder(((IOException) this).getMessage()); Andrei, it looks like while this CL was in review a call through the API was added. WDYT about my fix to avoid calling through the API here?
https://codereview.chromium.org/2440613003/diff/160001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/impl/NetworkExceptionImpl.java (right): https://codereview.chromium.org/2440613003/diff/160001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/impl/NetworkExceptionImpl.java:66: StringBuilder b = new StringBuilder(((IOException) this).getMessage()); On 2017/01/03 15:54:00, pauljensen wrote: > Andrei, it looks like while this CL was in review a call through the API was > added. WDYT about my fix to avoid calling through the API here? Wouldn't it create an infinite loop? Since the method is virtual, it should be equivalent to this.getMessage()
https://codereview.chromium.org/2440613003/diff/160001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/impl/NetworkExceptionImpl.java (right): https://codereview.chromium.org/2440613003/diff/160001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/impl/NetworkExceptionImpl.java:66: StringBuilder b = new StringBuilder(((IOException) this).getMessage()); On 2017/01/03 17:59:44, kapishnikov wrote: > On 2017/01/03 15:54:00, pauljensen wrote: > > Andrei, it looks like while this CL was in review a call through the API was > > added. WDYT about my fix to avoid calling through the API here? > > Wouldn't it create an infinite loop? Since the method is virtual, it should be > equivalent to this.getMessage() Oops, ya, I fixed it with an exception. PTAL, thank you!
Looks good. LGTM
The CQ bit was checked by pauljensen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1483469379118600,
"parent_rev": "0d4b2d75f5537b20a40650a8bca5a1dca7c29cc4", "commit_rev":
"c0ec6c541774f9e810690541ded8e8033e350a00"}
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Enforce implementation does not call through API classes Such calls could fail if an older API is used that does not contain newer methods. Calls should instead call through a wrapper class from VersionSafeCallbacks. R=kapishnikov BUG=629299 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== [Cronet] Enforce implementation does not call through API classes Such calls could fail if an older API is used that does not contain newer methods. Calls should instead call through a wrapper class from VersionSafeCallbacks. R=kapishnikov BUG=629299 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2440613003 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Enforce implementation does not call through API classes Such calls could fail if an older API is used that does not contain newer methods. Calls should instead call through a wrapper class from VersionSafeCallbacks. R=kapishnikov BUG=629299 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2440613003 ========== to ========== [Cronet] Enforce implementation does not call through API classes Such calls could fail if an older API is used that does not contain newer methods. Calls should instead call through a wrapper class from VersionSafeCallbacks. R=kapishnikov BUG=629299 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Committed: https://crrev.com/8c9905454fbeadbf095b2af75a7faaccc1beede2 Cr-Commit-Position: refs/heads/master@{#441186} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/8c9905454fbeadbf095b2af75a7faaccc1beede2 Cr-Commit-Position: refs/heads/master@{#441186} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
