Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2330873002:
Use modifed URLRequest API in net/cert_net (Closed)
|
Created:
4 years, 3 months ago by maksims (do not use this acc) Modified:
4 years, 3 months ago Reviewers:
mmenke CC:
chromium-reviews, cbentzel+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse modifed URLRequest API in net/cert_net
This cl splits the big CL with changes to net/ folder and
has changes only to net/cert_net according to the
changes to URLRequest API.
What the big cl does:
It modifies net/ clients that use URLRequest API as long as
URLRequest::Read returns int net errors and
URLRequest::Delegate and NetworkDelegate methods
(for example, OnResponseStarted or OnCompleted) have int
net_error in the arguments now.
The reason behind splitting the CL into small one is that
an android bot started to be unstable and unittests became
flaky. It was not possible to locate the problem and the
decision was to split the CL and upload small parts with a
6+ hours interval in order to make it possible to locate
the problematic code.
The big CL is located here -
https://codereview.chromium.org/2265873002/
BUG=423484
Committed: https://crrev.com/a74d3cbc572fa1c85cc2c0434a9f8e2f9cba0cd9
Committed: https://crrev.com/ea2402df18aee0655b835267feef57557de5b6cd
Cr-Original-Commit-Position: refs/heads/master@{#418014}
Cr-Commit-Position: refs/heads/master@{#418852}
Patch Set 1 #Patch Set 2 : Fixing the problem that made net_unittests flaky #
Total comments: 3
Patch Set 3 : return instead of break #
Messages
Total messages: 30 (18 generated)
The CQ bit was checked by maksim.sisov@intel.com 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.
Description was changed from ========== cert BUG= ========== to ========== Use modifed URLRequest API in net/cert_net This cl splits the big CL with changes to net/ folder and has changes only to net/cert_net according to the changes to URLRequest API. What the big cl does: It modifies net/ clients that use URLRequest API as long as URLRequest::Read returns int net errors and URLRequest::Delegate and NetworkDelegate methods (for example, OnResponseStarted or OnCompleted) have int net_error in the arguments now. The reason behind splitting the CL into small one is that an android bot started to be unstable and unittests became flaky. It was not possible to locate the problem and the decision was to split the CL and upload small parts with a 6+ hours interval in order to make it possible to locate the problematic code. The big CL is located here - https://codereview.chromium.org/2265873002/ BUG=423484 ==========
maksim.sisov@intel.com changed reviewers: + mmenke@chromium.org
ptal
LGTM
The CQ bit was checked by maksim.sisov@intel.com
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 ========== Use modifed URLRequest API in net/cert_net This cl splits the big CL with changes to net/ folder and has changes only to net/cert_net according to the changes to URLRequest API. What the big cl does: It modifies net/ clients that use URLRequest API as long as URLRequest::Read returns int net errors and URLRequest::Delegate and NetworkDelegate methods (for example, OnResponseStarted or OnCompleted) have int net_error in the arguments now. The reason behind splitting the CL into small one is that an android bot started to be unstable and unittests became flaky. It was not possible to locate the problem and the decision was to split the CL and upload small parts with a 6+ hours interval in order to make it possible to locate the problematic code. The big CL is located here - https://codereview.chromium.org/2265873002/ BUG=423484 ========== to ========== Use modifed URLRequest API in net/cert_net This cl splits the big CL with changes to net/ folder and has changes only to net/cert_net according to the changes to URLRequest API. What the big cl does: It modifies net/ clients that use URLRequest API as long as URLRequest::Read returns int net errors and URLRequest::Delegate and NetworkDelegate methods (for example, OnResponseStarted or OnCompleted) have int net_error in the arguments now. The reason behind splitting the CL into small one is that an android bot started to be unstable and unittests became flaky. It was not possible to locate the problem and the decision was to split the CL and upload small parts with a 6+ hours interval in order to make it possible to locate the problematic code. The big CL is located here - https://codereview.chromium.org/2265873002/ BUG=423484 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Use modifed URLRequest API in net/cert_net This cl splits the big CL with changes to net/ folder and has changes only to net/cert_net according to the changes to URLRequest API. What the big cl does: It modifies net/ clients that use URLRequest API as long as URLRequest::Read returns int net errors and URLRequest::Delegate and NetworkDelegate methods (for example, OnResponseStarted or OnCompleted) have int net_error in the arguments now. The reason behind splitting the CL into small one is that an android bot started to be unstable and unittests became flaky. It was not possible to locate the problem and the decision was to split the CL and upload small parts with a 6+ hours interval in order to make it possible to locate the problematic code. The big CL is located here - https://codereview.chromium.org/2265873002/ BUG=423484 ========== to ========== Use modifed URLRequest API in net/cert_net This cl splits the big CL with changes to net/ folder and has changes only to net/cert_net according to the changes to URLRequest API. What the big cl does: It modifies net/ clients that use URLRequest API as long as URLRequest::Read returns int net errors and URLRequest::Delegate and NetworkDelegate methods (for example, OnResponseStarted or OnCompleted) have int net_error in the arguments now. The reason behind splitting the CL into small one is that an android bot started to be unstable and unittests became flaky. It was not possible to locate the problem and the decision was to split the CL and upload small parts with a 6+ hours interval in order to make it possible to locate the problematic code. The big CL is located here - https://codereview.chromium.org/2265873002/ BUG=423484 Committed: https://crrev.com/a74d3cbc572fa1c85cc2c0434a9f8e2f9cba0cd9 Cr-Commit-Position: refs/heads/master@{#418014} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/a74d3cbc572fa1c85cc2c0434a9f8e2f9cba0cd9 Cr-Commit-Position: refs/heads/master@{#418014}
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/2332293002/ by maksim.sisov@intel.com. The reason for reverting is: This part of the big CL makes net unittests run on android bot to be flaky..
Message was sent while issue was closed.
Description was changed from ========== Use modifed URLRequest API in net/cert_net This cl splits the big CL with changes to net/ folder and has changes only to net/cert_net according to the changes to URLRequest API. What the big cl does: It modifies net/ clients that use URLRequest API as long as URLRequest::Read returns int net errors and URLRequest::Delegate and NetworkDelegate methods (for example, OnResponseStarted or OnCompleted) have int net_error in the arguments now. The reason behind splitting the CL into small one is that an android bot started to be unstable and unittests became flaky. It was not possible to locate the problem and the decision was to split the CL and upload small parts with a 6+ hours interval in order to make it possible to locate the problematic code. The big CL is located here - https://codereview.chromium.org/2265873002/ BUG=423484 Committed: https://crrev.com/a74d3cbc572fa1c85cc2c0434a9f8e2f9cba0cd9 Cr-Commit-Position: refs/heads/master@{#418014} ========== to ========== Use modifed URLRequest API in net/cert_net This cl splits the big CL with changes to net/ folder and has changes only to net/cert_net according to the changes to URLRequest API. What the big cl does: It modifies net/ clients that use URLRequest API as long as URLRequest::Read returns int net errors and URLRequest::Delegate and NetworkDelegate methods (for example, OnResponseStarted or OnCompleted) have int net_error in the arguments now. The reason behind splitting the CL into small one is that an android bot started to be unstable and unittests became flaky. It was not possible to locate the problem and the decision was to split the CL and upload small parts with a 6+ hours interval in order to make it possible to locate the problematic code. The big CL is located here - https://codereview.chromium.org/2265873002/ BUG=423484 Committed: https://crrev.com/a74d3cbc572fa1c85cc2c0434a9f8e2f9cba0cd9 Cr-Commit-Position: refs/heads/master@{#418014} ==========
The CQ bit was checked by maksim.sisov@intel.com 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...
https://codereview.chromium.org/2330873002/diff/20001/net/cert_net/cert_net_f... File net/cert_net/cert_net_fetcher_impl.cc (right): https://codereview.chromium.org/2330873002/diff/20001/net/cert_net/cert_net_f... net/cert_net/cert_net_fetcher_impl.cc:359: if (num_bytes == ERR_IO_PENDING) Ok, this one was the problem. I succeeded to reproduce the issue locally and net_unittests failed several times with origin commit. But when I added this check, they became stable. I asked in chromium channel to revert the patch to stabilize the bot and will commit another CL to see whether new issues appear or not.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2330873002/diff/20001/net/cert_net/cert_net_f... File net/cert_net/cert_net_fetcher_impl.cc (right): https://codereview.chromium.org/2330873002/diff/20001/net/cert_net/cert_net_f... net/cert_net/cert_net_fetcher_impl.cc:360: break; I think this makes more sense as a return than a break. Then you can also remove the num_bytes != ERR_IO_PENDING check at the end of this method, and just unconditionally call OnUrlRequestCompleted.
The CQ bit was checked by maksim.sisov@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2330873002/#ps40001 (title: "return instead of break")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I'll commit this and see if it brakes anything. https://codereview.chromium.org/2330873002/diff/20001/net/cert_net/cert_net_f... File net/cert_net/cert_net_fetcher_impl.cc (right): https://codereview.chromium.org/2330873002/diff/20001/net/cert_net/cert_net_f... net/cert_net/cert_net_fetcher_impl.cc:360: break; On 2016/09/13 16:33:11, mmenke wrote: > I think this makes more sense as a return than a break. Then you can also > remove the num_bytes != ERR_IO_PENDING check at the end of this method, and just > unconditionally call OnUrlRequestCompleted. Done.
Message was sent while issue was closed.
Description was changed from ========== Use modifed URLRequest API in net/cert_net This cl splits the big CL with changes to net/ folder and has changes only to net/cert_net according to the changes to URLRequest API. What the big cl does: It modifies net/ clients that use URLRequest API as long as URLRequest::Read returns int net errors and URLRequest::Delegate and NetworkDelegate methods (for example, OnResponseStarted or OnCompleted) have int net_error in the arguments now. The reason behind splitting the CL into small one is that an android bot started to be unstable and unittests became flaky. It was not possible to locate the problem and the decision was to split the CL and upload small parts with a 6+ hours interval in order to make it possible to locate the problematic code. The big CL is located here - https://codereview.chromium.org/2265873002/ BUG=423484 Committed: https://crrev.com/a74d3cbc572fa1c85cc2c0434a9f8e2f9cba0cd9 Cr-Commit-Position: refs/heads/master@{#418014} ========== to ========== Use modifed URLRequest API in net/cert_net This cl splits the big CL with changes to net/ folder and has changes only to net/cert_net according to the changes to URLRequest API. What the big cl does: It modifies net/ clients that use URLRequest API as long as URLRequest::Read returns int net errors and URLRequest::Delegate and NetworkDelegate methods (for example, OnResponseStarted or OnCompleted) have int net_error in the arguments now. The reason behind splitting the CL into small one is that an android bot started to be unstable and unittests became flaky. It was not possible to locate the problem and the decision was to split the CL and upload small parts with a 6+ hours interval in order to make it possible to locate the problematic code. The big CL is located here - https://codereview.chromium.org/2265873002/ BUG=423484 Committed: https://crrev.com/a74d3cbc572fa1c85cc2c0434a9f8e2f9cba0cd9 Cr-Commit-Position: refs/heads/master@{#418014} ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Use modifed URLRequest API in net/cert_net This cl splits the big CL with changes to net/ folder and has changes only to net/cert_net according to the changes to URLRequest API. What the big cl does: It modifies net/ clients that use URLRequest API as long as URLRequest::Read returns int net errors and URLRequest::Delegate and NetworkDelegate methods (for example, OnResponseStarted or OnCompleted) have int net_error in the arguments now. The reason behind splitting the CL into small one is that an android bot started to be unstable and unittests became flaky. It was not possible to locate the problem and the decision was to split the CL and upload small parts with a 6+ hours interval in order to make it possible to locate the problematic code. The big CL is located here - https://codereview.chromium.org/2265873002/ BUG=423484 Committed: https://crrev.com/a74d3cbc572fa1c85cc2c0434a9f8e2f9cba0cd9 Cr-Commit-Position: refs/heads/master@{#418014} ========== to ========== Use modifed URLRequest API in net/cert_net This cl splits the big CL with changes to net/ folder and has changes only to net/cert_net according to the changes to URLRequest API. What the big cl does: It modifies net/ clients that use URLRequest API as long as URLRequest::Read returns int net errors and URLRequest::Delegate and NetworkDelegate methods (for example, OnResponseStarted or OnCompleted) have int net_error in the arguments now. The reason behind splitting the CL into small one is that an android bot started to be unstable and unittests became flaky. It was not possible to locate the problem and the decision was to split the CL and upload small parts with a 6+ hours interval in order to make it possible to locate the problematic code. The big CL is located here - https://codereview.chromium.org/2265873002/ BUG=423484 Committed: https://crrev.com/a74d3cbc572fa1c85cc2c0434a9f8e2f9cba0cd9 Committed: https://crrev.com/ea2402df18aee0655b835267feef57557de5b6cd Cr-Original-Commit-Position: refs/heads/master@{#418014} Cr-Commit-Position: refs/heads/master@{#418852} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/ea2402df18aee0655b835267feef57557de5b6cd Cr-Commit-Position: refs/heads/master@{#418852} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
