Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2585203003:
Don't try to report metrics when there's no URLRequest (Closed)
DescriptionDon't try to report metrics when there's no URLRequest
This case can happen when UrlRequest.start() throws an exception.
BUG=675629
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
Committed: https://crrev.com/c7432bd7c942317b23e365d0a816891ba3e5e63f
Cr-Commit-Position: refs/heads/master@{#439599}
Patch Set 1 #
Total comments: 4
Patch Set 2 : delete part of test #
Total comments: 4
Patch Set 3 : make executor reject tasks #
Messages
Total messages: 17 (7 generated)
Description was changed from ========== Don't try to report metrics when there's no URLRequest This case can happen when UrlRequest.start() throws an exception. BUG=675629 ========== to ========== Don't try to report metrics when there's no URLRequest This case can happen when UrlRequest.start() throws an exception. BUG=675629 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
mgersh@chromium.org changed reviewers: + xunjieli@chromium.org
PTAL
https://codereview.chromium.org/2585203003/diff/1/components/cronet/android/t... File components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java (right): https://codereview.chromium.org/2585203003/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java:321: callback = new TestUrlRequestCallback(); Does the test not repro the crash if you only make one request? Why is the second request needed?
https://codereview.chromium.org/2585203003/diff/1/components/cronet/android/t... File components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java (right): https://codereview.chromium.org/2585203003/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java:321: callback = new TestUrlRequestCallback(); On 2016/12/19 18:52:09, xunjieli wrote: > Does the test not repro the crash if you only make one request? Why is the > second request needed? It does repro without this part. It's primarily here to test that we don't try to call the listener in this case, by making sure that the listener is first called by the second request. I was also worried about it being flaky rather than always failing because the crash happens on the network thread and might not happen until after the test has finished, and this makes sure that later events on the network thread are still happening. But I'm not sure whether either of those is actually a concern in practice, so if you don't think it is, I could delete this.
https://codereview.chromium.org/2585203003/diff/1/components/cronet/android/t... File components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java (right): https://codereview.chromium.org/2585203003/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java:321: callback = new TestUrlRequestCallback(); On 2016/12/19 19:06:11, mgersh wrote: > On 2016/12/19 18:52:09, xunjieli wrote: > > Does the test not repro the crash if you only make one request? Why is the > > second request needed? > > It does repro without this part. It's primarily here to test that we don't try > to call the listener in this case, by making sure that the listener is first > called by the second request. I was also worried about it being flaky rather > than always failing because the crash happens on the network thread and might > not happen until after the test has finished, and this makes sure that later > events on the network thread are still happening. But I'm not sure whether > either of those is actually a concern in practice, so if you don't think it is, > I could delete this. One thing confuses me is that TestRequestFinishedListener doesn't handle multiple RequestFinishedInfo -- it basically remembers the latest update. If we get updates from both requests, we wouldn't be able to tell from this test case. I'd suggest either get rid of the second request or change TestRequestFinishedListener so it can accumulate all updates that it receives.
https://codereview.chromium.org/2585203003/diff/1/components/cronet/android/t... File components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java (right): https://codereview.chromium.org/2585203003/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java:321: callback = new TestUrlRequestCallback(); On 2016/12/19 19:40:05, xunjieli wrote: > On 2016/12/19 19:06:11, mgersh wrote: > > On 2016/12/19 18:52:09, xunjieli wrote: > > > Does the test not repro the crash if you only make one request? Why is the > > > second request needed? > > > > It does repro without this part. It's primarily here to test that we don't try > > to call the listener in this case, by making sure that the listener is first > > called by the second request. I was also worried about it being flaky rather > > than always failing because the crash happens on the network thread and might > > not happen until after the test has finished, and this makes sure that later > > events on the network thread are still happening. But I'm not sure whether > > either of those is actually a concern in practice, so if you don't think it > is, > > I could delete this. > > One thing confuses me is that TestRequestFinishedListener doesn't handle > multiple RequestFinishedInfo -- it basically remembers the latest update. If we > get updates from both requests, we wouldn't be able to tell from this test case. > > I'd suggest either get rid of the second request or change > TestRequestFinishedListener so it can accumulate all updates that it receives. Done.
lgtm https://codereview.chromium.org/2585203003/diff/20001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java (right): https://codereview.chromium.org/2585203003/diff/20001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java:302: TestRequestFinishedListener requestFinishedListener = new TestRequestFinishedListener(); Can you make sure the executor backing this listener doesn't have any task posted? could have a simple Executor that rejects all tasks. https://codereview.chromium.org/2585203003/diff/20001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java:311: .addRequestAnnotation(this) are these two annotations needed?
https://codereview.chromium.org/2585203003/diff/20001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java (right): https://codereview.chromium.org/2585203003/diff/20001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java:302: TestRequestFinishedListener requestFinishedListener = new TestRequestFinishedListener(); On 2016/12/19 21:18:31, xunjieli wrote: > Can you make sure the executor backing this listener doesn't have any task > posted? could have a simple Executor that rejects all tasks. Oh, that's a better way to do it, thanks. Done. https://codereview.chromium.org/2585203003/diff/20001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/RequestFinishedInfoTest.java:311: .addRequestAnnotation(this) On 2016/12/19 21:18:31, xunjieli wrote: > are these two annotations needed? Nope, just left over from an earlier version. Removed them.
The CQ bit was checked by mgersh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xunjieli@chromium.org Link to the patchset: https://codereview.chromium.org/2585203003/#ps40001 (title: "make executor reject tasks")
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": 40001, "attempt_start_ts": 1482183450648300,
"parent_rev": "52f48f8f9a5edfaf3c2f353c64be385adc6e3223", "commit_rev":
"b7db0fa4f791de585af0098120d6a9bd4f112c71"}
Message was sent while issue was closed.
Description was changed from ========== Don't try to report metrics when there's no URLRequest This case can happen when UrlRequest.start() throws an exception. BUG=675629 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== Don't try to report metrics when there's no URLRequest This case can happen when UrlRequest.start() throws an exception. BUG=675629 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2585203003 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Don't try to report metrics when there's no URLRequest This case can happen when UrlRequest.start() throws an exception. BUG=675629 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2585203003 ========== to ========== Don't try to report metrics when there's no URLRequest This case can happen when UrlRequest.start() throws an exception. BUG=675629 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Committed: https://crrev.com/c7432bd7c942317b23e365d0a816891ba3e5e63f Cr-Commit-Position: refs/heads/master@{#439599} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c7432bd7c942317b23e365d0a816891ba3e5e63f Cr-Commit-Position: refs/heads/master@{#439599} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
