Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2298823002:
Resetting the HttpRequestInfo pointers in HttpNetworkTransaction and streams (Closed)
|
Created:
4 years, 3 months ago by shivanisha Modified:
4 years, 2 months ago CC:
Bryan McQuade, cbentzel+watch_chromium.org, chromium-reviews, jkarlin Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionResetting the HttpRequestInfo pointers in HttpNetworkTransaction and the
underlying streams. This is being done to allow sharing of
HttpNetworkTransaction across various HttpCache::Transactions which might
involve the network transaction and streams to outlive the request info's
owner HttpRequestInfo.
This approach is taken instead of making a copy of request info because in addition
to making the memory footprint larger (headers could be couple of thousands
Kilobytes), it does not seem right to have a copy of a request info whose
URLRequestHttpJob does not exist anymore.
Also HttpRequestInfo contains a pointer to the upload data stream which cannot
be copied and having its pointer around after its owner URLRequest has died is
incorrect.
BUG=472740
Committed: https://crrev.com/0b44085daafa86fe2e6cb11b369c3120082af8dc
Cr-Commit-Position: refs/heads/master@{#425974}
Patch Set 1 #Patch Set 2 : Initial patch #Patch Set 3 : Initial patch #Patch Set 4 : Initial patch #Patch Set 5 : Initial patch #Patch Set 6 : Initial patch #
Total comments: 20
Patch Set 7 : (Intermediate patch-Ignore) Removed GetUploadProgress plumbing and incorporated other feedback. #Patch Set 8 : (Intermediate patch-Ignore) Rebased, removed Upload progress plumbing, incorporated other feedback. #Patch Set 9 : (Intermediate patch-Ignore) Rebased, removed Upload progress plumbing, incorporated other feedback. #Patch Set 10 : (Intermediate patch-Ignore) rebased, removed upload progress plumbing, incorporated other feedback. #Patch Set 11 : (Intermediate patch-Ignore) Rebased, Reviewed upload progress plumbing and other feedback. #Patch Set 12 : (Intermediate patch-Ignore) Rebased, Removed upload progress plumbing and other feedback. #Patch Set 13 : (Intermediate patch-Ignore) Rebased, Removed Upload Progress plumbing, feedback #Patch Set 14 : Rebased, removed upload progress plumbing, feedback. (Rebased till refs/heads/master@{#417381}) #
Total comments: 16
Patch Set 15 : Feedback incorporated and Upload progress plumbing removal moved to another CL #Patch Set 16 : Rebased till refs/heads/master@{#425377} #
Messages
Total messages: 92 (68 generated)
The CQ bit was checked by shivanisha@chromium.org 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: 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...)
The CQ bit was checked by shivanisha@chromium.org 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 checked by shivanisha@chromium.org 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: Exceeded global retry quota
The CQ bit was checked by shivanisha@chromium.org 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: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by shivanisha@chromium.org 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...
Description was changed from ========== Resetting the HttpRequestInfo pointers in HttpNetworkTransaction and the underlying streams. This is being done to allow sharing of HttpNetworkTransaction across various HttpCache::Transactions which might involve the network transaction and streams to outlive the request info's owner HttpRequestInfo. BUG=472740 ========== to ========== Resetting the HttpRequestInfo pointers in HttpNetworkTransaction and the underlying streams. This is being done to allow sharing of HttpNetworkTransaction across various HttpCache::Transactions which might involve the network transaction and streams to outlive the request info's owner HttpRequestInfo. This approach is taken instead of making a copy of request info because in addition to making the memory footprint larger (headers could be couple of thousands Kilobytes), it does not seem right to have a copy of a request info whose URLRequestHttpJob does not exist anymore. Also HttpRequestInfo contains a pointer to the upload data stream which cannot be copied and having its pointer around after its owner URLRequest has died is incorrect. BUG=472740 ==========
Description was changed from ========== Resetting the HttpRequestInfo pointers in HttpNetworkTransaction and the underlying streams. This is being done to allow sharing of HttpNetworkTransaction across various HttpCache::Transactions which might involve the network transaction and streams to outlive the request info's owner HttpRequestInfo. This approach is taken instead of making a copy of request info because in addition to making the memory footprint larger (headers could be couple of thousands Kilobytes), it does not seem right to have a copy of a request info whose URLRequestHttpJob does not exist anymore. Also HttpRequestInfo contains a pointer to the upload data stream which cannot be copied and having its pointer around after its owner URLRequest has died is incorrect. BUG=472740 ========== to ========== Resetting the HttpRequestInfo pointers in HttpNetworkTransaction and the underlying streams. This is being done to allow sharing of HttpNetworkTransaction across various HttpCache::Transactions which might involve the network transaction and streams to outlive the request info's owner HttpRequestInfo. This approach is taken instead of making a copy of request info because in addition to making the memory footprint larger (headers could be couple of thousands Kilobytes), it does not seem right to have a copy of a request info whose URLRequestHttpJob does not exist anymore. Also HttpRequestInfo contains a pointer to the upload data stream which cannot be copied and having its pointer around after its owner URLRequest has died is incorrect. BUG=472740 ==========
shivanisha@chromium.org changed reviewers: + mmenke@chromium.org, rdsmith@chromium.org
Hi Matt, Randy, Please take a look at this CL. This is a pre-requisite for the http cache lock fix. This Cl is not meant to be committed in isolation before the complete fix is ready, but I am trying to break the complete fix into smaller sub-parts. Randy, I have tried to explain resetting vs. copying in the description section but we can discuss more, if need be. Thanks.
https://codereview.chromium.org/2298823002/diff/90001/net/http/http_basic_sta... File net/http/http_basic_state.h (right): https://codereview.chromium.org/2298823002/diff/90001/net/http/http_basic_sta... net/http/http_basic_state.h:68: std::string request_method_; Why do we need these? Generating the request line after the HttpRequestInfo is cleared is useless anyways - we no longer have the request body to upload. https://codereview.chromium.org/2298823002/diff/90001/net/quic/chromium/quic_... File net/quic/chromium/quic_http_stream.cc (right): https://codereview.chromium.org/2298823002/diff/90001/net/quic/chromium/quic_... net/quic/chromium/quic_http_stream.cc:852: DCHECK(next_state_ == STATE_OPEN); DCHECK_EQ provides more useful output on failure. https://codereview.chromium.org/2298823002/diff/90001/net/quic/chromium/quic_... net/quic/chromium/quic_http_stream.cc:860: } // namespace net Add blank line before end of namespace. https://codereview.chromium.org/2298823002/diff/90001/net/spdy/spdy_http_stre... File net/spdy/spdy_http_stream.cc (right): https://codereview.chromium.org/2298823002/diff/90001/net/spdy/spdy_http_stre... net/spdy/spdy_http_stream.cc:103: return upload_progress_; Hrm...I wonder about all this UploadProgress plumbing. The URLRequest itself is what actually owns the UploadDataStream. Maybe we should just rip out all of the UploadProgress data sharing, and have it query the stream directly? I suppose with this API, we could report the amount StreamSocket::Send has said we've send, instead of including the amount that's currently in the write buffer, but it seems a minor distinction, and we're not doing that now, anyways. I suppose that would introduce some potential problems: Before we create the stream, and while we're retrying, we have to rewind the stream. While we're doing that, we probably report an empty UploadProgress, rather than calling into the UploadDataStream, as we have no idea what it will return. We could get around it by just report it at the HttpNetworkTransaction layer. That's the layer responsible for rewinding it. If there's no HttpStream, we just report an UploadProgress(). The responsibility for who does what is a little funky here, regardless of where we do this, but as-is, seems like we have the exact same code in all three HttpStream implementations of this method. Anyhow, may be worth spending a little time thinking about what's most sane here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Initial comments. My biggest concern is the possible conflict in the requirements for the interface specification on the abstract base class HttpTransaction. Can we also arrange that HttpCache::Transaction doesn't require the request info to be live after the same point in the state machine? The idea of using a WeakPtr for this is also growing on me--I think I'm going to upgrade it to "suggestion". It means we'll have DCHECK's in the lower levels enforcing the interface contract, but we don't have to do any particular plumbing--the nulling out of the pointer will be automatic and driven by the owner, as it should be. https://codereview.chromium.org/2298823002/diff/90001/net/http/http_network_t... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/2298823002/diff/90001/net/http/http_network_t... net/http/http_network_transaction.cc:324: // the HttpRequestInfo will be invalid if not marked null here. Request: Could you rewrite this paragraph in terms of a generic consumer of HttpNetworkTransaction, as opposed to specifying that URLRequestHttpJob is the owner? Describing it this way creates a dependency (if only conceptual) between this class and the classes that use it; better to just describe this classes behavior. https://codereview.chromium.org/2298823002/diff/90001/net/http/http_network_t... net/http/http_network_transaction.cc:1637: // Reset will ensure that HttpRequestInfo is only used uptill final response nit: up until? https://codereview.chromium.org/2298823002/diff/90001/net/http/http_network_t... File net/http/http_network_transaction.h (right): https://codereview.chromium.org/2298823002/diff/90001/net/http/http_network_t... net/http/http_network_transaction.h:51: : public HttpTransaction, We need to change the documentation on HttpTransaction::Start(), since we are changing that interface contract. (I'm a bit concerned that we need the old interface contract for HttpCache::Transaction and the new one for HttpNetworkTransaction, which is a heck of a violation of interface abstraction :-{.) https://codereview.chromium.org/2298823002/diff/90001/net/http/http_stream_pa... File net/http/http_stream_parser.cc (right): https://codereview.chromium.org/2298823002/diff/90001/net/http/http_stream_pa... net/http/http_stream_parser.cc:385: // stream to outlive the request_'s owner URLRequestHttpJob. As noted elsewhere, I'm really uncomfortable with comments making references to owning code/code further up the stack; that kind of dependency gets in the way of modularity. Can you make these comments just talk about a generic owner of this class and what it allows that owner to do? https://codereview.chromium.org/2298823002/diff/90001/net/http/http_stream_pa... File net/http/http_stream_parser.h (right): https://codereview.chromium.org/2298823002/diff/90001/net/http/http_stream_pa... net/http/http_stream_parser.h:208: const HttpRequestInfo* request_; Though (i.e. not even to the level of suggestion): Would it be cleaner to make all of these references weak pointers, so we don't have to rely on the ResetRequestInfo() being correctly plumbed to cleanup references to to-be-freed memory? We can still have documentation in the constructors/methods from which the various classes get this pointer as to how long the owner must keep the pointer alive for, but with a weak pointer we'll get a clean crash when the plumbing fails as opposed to a use-after-free bug.
https://codereview.chromium.org/2298823002/diff/90001/net/http/http_network_t... File net/http/http_network_transaction.h (right): https://codereview.chromium.org/2298823002/diff/90001/net/http/http_network_t... net/http/http_network_transaction.h:51: : public HttpTransaction, On 2016/09/01 at 20:18:31, Randy Smith - Not in Fridays wrote: > We need to change the documentation on HttpTransaction::Start(), since we are changing that interface contract. (I'm a bit concerned that we need the old interface contract for HttpCache::Transaction and the new one for HttpNetworkTransaction, which is a heck of a violation of interface abstraction :-{.) We are not really changing the contract. In the existing code it is implicit that HttpRequestInfo may be null when reading of response body starts while this change will make it explicit. I came across this test and the accompanying comment which made me think that it was indeed an implicit assumption: https://cs.chromium.org/chromium/src/net/http/http_stream_parser_unittest.cc?... Although this test is on a stream, the assumption that read may be invoked after the request info has been freed, was probably true for HttpTransaction as well as it wasn't being functionally used after read started. https://codereview.chromium.org/2298823002/diff/90001/net/spdy/spdy_http_stre... File net/spdy/spdy_http_stream.cc (right): https://codereview.chromium.org/2298823002/diff/90001/net/spdy/spdy_http_stre... net/spdy/spdy_http_stream.cc:103: return upload_progress_; Why do we even need to go to the URLRequestJob, HttpTransaction and HttpStream layers for getting upload progress. Since UploadDataStream has all the information about its current progress, isn't returning that at the URLRequest layer sufficient? That sounds like the correct approach to me since URLRequest actually owns UploadDataStream. Am I missing something here?
https://codereview.chromium.org/2298823002/diff/90001/net/spdy/spdy_http_stre... File net/spdy/spdy_http_stream.cc (right): https://codereview.chromium.org/2298823002/diff/90001/net/spdy/spdy_http_stre... net/spdy/spdy_http_stream.cc:103: return upload_progress_; On 2016/09/01 at 20:30:59, shivanisha wrote: > Why do we even need to go to the URLRequestJob, HttpTransaction and HttpStream layers for getting upload progress. Since UploadDataStream has all the information about its current progress, isn't returning that at the URLRequest layer sufficient? That sounds like the correct approach to me since URLRequest actually owns UploadDataStream. Am I missing something here? Ah ok, I see your concerns about rewinding the stream.
On 2016/09/01 20:32:12, shivanisha wrote: > https://codereview.chromium.org/2298823002/diff/90001/net/spdy/spdy_http_stre... > File net/spdy/spdy_http_stream.cc (right): > > https://codereview.chromium.org/2298823002/diff/90001/net/spdy/spdy_http_stre... > net/spdy/spdy_http_stream.cc:103: return upload_progress_; > On 2016/09/01 at 20:30:59, shivanisha wrote: > > Why do we even need to go to the URLRequestJob, HttpTransaction and HttpStream > layers for getting upload progress. Since UploadDataStream has all the > information about its current progress, isn't returning that at the URLRequest > layer sufficient? That sounds like the correct approach to me since URLRequest > actually owns UploadDataStream. Am I missing something here? > > Ah ok, I see your concerns about rewinding the stream. Yea, it could be things would just work, but weed need to check that the UploadDataStream would be sane in those two cases (During initialization, and while rewinding), which we currently don't rely on. If they do just work, I'm fine with switching to that.
https://codereview.chromium.org/2298823002/diff/90001/net/http/http_network_t... File net/http/http_network_transaction.h (right): https://codereview.chromium.org/2298823002/diff/90001/net/http/http_network_t... net/http/http_network_transaction.h:51: : public HttpTransaction, On 2016/09/01 20:30:59, shivanisha wrote: > On 2016/09/01 at 20:18:31, Randy Smith - Not in Fridays wrote: > > We need to change the documentation on HttpTransaction::Start(), since we are > changing that interface contract. (I'm a bit concerned that we need the old > interface contract for HttpCache::Transaction and the new one for > HttpNetworkTransaction, which is a heck of a violation of interface abstraction > :-{.) > > We are not really changing the contract. In the existing code it is implicit > that HttpRequestInfo may be null when reading of response body starts while this > change will make it explicit. I came across this test and the accompanying > comment which made me think that it was indeed an implicit assumption: > https://cs.chromium.org/chromium/src/net/http/http_stream_parser_unittest.cc?... > > Although this test is on a stream, the assumption that read may be invoked after > the request info has been freed, was probably true for HttpTransaction as well > as it wasn't being functionally used after read started. Ah. Wonderful :-J. Thank you for the pointer (and for figuring all this out). I'd still like to take this opportunity to update the interface contract comment in HttpTransaction, and if the idea makes sense to you, switch over to a weak pointer as well.
The CQ bit was checked by shivanisha@chromium.org 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: 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-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by shivanisha@chromium.org 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 checked by shivanisha@chromium.org 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 checked by shivanisha@chromium.org 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: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by shivanisha@chromium.org 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: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by shivanisha@chromium.org 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: 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...)
On 2016/09/02 at 16:44:05, rdsmith wrote: > https://codereview.chromium.org/2298823002/diff/90001/net/http/http_network_t... > File net/http/http_network_transaction.h (right): > > https://codereview.chromium.org/2298823002/diff/90001/net/http/http_network_t... > net/http/http_network_transaction.h:51: : public HttpTransaction, > On 2016/09/01 20:30:59, shivanisha wrote: > > On 2016/09/01 at 20:18:31, Randy Smith - Not in Fridays wrote: > > > We need to change the documentation on HttpTransaction::Start(), since we are > > changing that interface contract. (I'm a bit concerned that we need the old > > interface contract for HttpCache::Transaction and the new one for > > HttpNetworkTransaction, which is a heck of a violation of interface abstraction > > :-{.) > > > > We are not really changing the contract. In the existing code it is implicit > > that HttpRequestInfo may be null when reading of response body starts while this > > change will make it explicit. I came across this test and the accompanying > > comment which made me think that it was indeed an implicit assumption: > > https://cs.chromium.org/chromium/src/net/http/http_stream_parser_unittest.cc?... > > > > Although this test is on a stream, the assumption that read may be invoked after > > the request info has been freed, was probably true for HttpTransaction as well > > as it wasn't being functionally used after read started. > > Ah. Wonderful :-J. Thank you for the pointer (and for figuring all this out). > > I'd still like to take this opportunity to update the interface contract comment in HttpTransaction, and if the idea makes sense to you, switch over to a weak pointer as well. Sure. Updated the interface contract in HttpTransaction. Regarding weak pointer, my thoughts are: The idea behind resetting the request info to null explicitly is not just for fixing the lifetime issue but also because httpNetworkTransaction and HttpStream will now be shared across various consumers and request info may have information specific to a consumer, which we do not want, once sharing starts (which aligns nicely to starting to read the response body, which is also the time when request info is no longer needed). Using weak pointer would require the request info to be converted to a shared pointer to be used, implying shared ownership while it is being used, which also has a performance overhead. For request info we do not need to have that performance implication as we do not require shared ownership semantics. Request info is owned by the calling context and used by the lower layers in the calling stack (as read-only) but it never needs to be used in any two contexts simultaneously. Being a const raw pointer here thus seems a better option because of its low cost and constness. The disadvantage obviously is the complexity to reset it at the right time but since this code has been running with the implicit assumption for a long time, I am not too worried about it. Another reason to prefer a raw pointer is that it also contains upload_data_stream which cannot be shared since its ownership is unique. So even if we were to convert the request info to weak ptr , we would still have the upload_data_stream raw pointer.
https://codereview.chromium.org/2298823002/diff/90001/net/http/http_basic_sta... File net/http/http_basic_state.h (right): https://codereview.chromium.org/2298823002/diff/90001/net/http/http_basic_sta... net/http/http_basic_state.h:68: std::string request_method_; On 2016/08/31 at 21:57:59, mmenke wrote: > Why do we need these? Generating the request line after the HttpRequestInfo is cleared is useless anyways - we no longer have the request body to upload. We need these only for generating request line. I have saved these instead of request info pointer as request info is not needed here leading to one less case to worry about for resetting. https://codereview.chromium.org/2298823002/diff/90001/net/http/http_network_t... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/2298823002/diff/90001/net/http/http_network_t... net/http/http_network_transaction.cc:324: // the HttpRequestInfo will be invalid if not marked null here. On 2016/09/01 at 20:18:31, Randy Smith - Not in Fridays wrote: > Request: Could you rewrite this paragraph in terms of a generic consumer of HttpNetworkTransaction, as opposed to specifying that URLRequestHttpJob is the owner? Describing it this way creates a dependency (if only conceptual) between this class and the classes that use it; better to just describe this classes behavior. done. https://codereview.chromium.org/2298823002/diff/90001/net/http/http_network_t... net/http/http_network_transaction.cc:1637: // Reset will ensure that HttpRequestInfo is only used uptill final response On 2016/09/01 at 20:18:31, Randy Smith - Not in Fridays wrote: > nit: up until? done. https://codereview.chromium.org/2298823002/diff/90001/net/http/http_stream_pa... File net/http/http_stream_parser.cc (right): https://codereview.chromium.org/2298823002/diff/90001/net/http/http_stream_pa... net/http/http_stream_parser.cc:385: // stream to outlive the request_'s owner URLRequestHttpJob. On 2016/09/01 at 20:18:31, Randy Smith - Not in Fridays wrote: > As noted elsewhere, I'm really uncomfortable with comments making references to owning code/code further up the stack; that kind of dependency gets in the way of modularity. Can you make these comments just talk about a generic owner of this class and what it allows that owner to do? done. https://codereview.chromium.org/2298823002/diff/90001/net/quic/chromium/quic_... File net/quic/chromium/quic_http_stream.cc (right): https://codereview.chromium.org/2298823002/diff/90001/net/quic/chromium/quic_... net/quic/chromium/quic_http_stream.cc:852: DCHECK(next_state_ == STATE_OPEN); On 2016/08/31 at 21:57:59, mmenke wrote: > DCHECK_EQ provides more useful output on failure. done. https://codereview.chromium.org/2298823002/diff/90001/net/quic/chromium/quic_... net/quic/chromium/quic_http_stream.cc:860: } // namespace net On 2016/08/31 at 21:57:59, mmenke wrote: > Add blank line before end of namespace. done. https://codereview.chromium.org/2298823002/diff/90001/net/spdy/spdy_http_stre... File net/spdy/spdy_http_stream.cc (right): https://codereview.chromium.org/2298823002/diff/90001/net/spdy/spdy_http_stre... net/spdy/spdy_http_stream.cc:103: return upload_progress_; On 2016/09/01 at 20:32:12, shivanisha wrote: > On 2016/09/01 at 20:30:59, shivanisha wrote: > > Why do we even need to go to the URLRequestJob, HttpTransaction and HttpStream layers for getting upload progress. Since UploadDataStream has all the information about its current progress, isn't returning that at the URLRequest layer sufficient? That sounds like the correct approach to me since URLRequest actually owns UploadDataStream. Am I missing something here? > > Ah ok, I see your concerns about rewinding the stream. Removed all the plumbing for getting upload progress. Now URLRequest can invoke GetUploadProgress directly on upload stream. The stream will return empty UploadProgress if it is not yet successfully initialized. Also added a test for the uninitialized condition.
On 2016/09/08 at 20:37:25, shivanisha wrote: > On 2016/09/02 at 16:44:05, rdsmith wrote: > > https://codereview.chromium.org/2298823002/diff/90001/net/http/http_network_t... > > File net/http/http_network_transaction.h (right): > > > > https://codereview.chromium.org/2298823002/diff/90001/net/http/http_network_t... > > net/http/http_network_transaction.h:51: : public HttpTransaction, > > On 2016/09/01 20:30:59, shivanisha wrote: > > > On 2016/09/01 at 20:18:31, Randy Smith - Not in Fridays wrote: > > > > We need to change the documentation on HttpTransaction::Start(), since we are > > > changing that interface contract. (I'm a bit concerned that we need the old > > > interface contract for HttpCache::Transaction and the new one for > > > HttpNetworkTransaction, which is a heck of a violation of interface abstraction > > > :-{.) > > > > > > We are not really changing the contract. In the existing code it is implicit > > > that HttpRequestInfo may be null when reading of response body starts while this > > > change will make it explicit. I came across this test and the accompanying > > > comment which made me think that it was indeed an implicit assumption: > > > https://cs.chromium.org/chromium/src/net/http/http_stream_parser_unittest.cc?... > > > > > > Although this test is on a stream, the assumption that read may be invoked after > > > the request info has been freed, was probably true for HttpTransaction as well > > > as it wasn't being functionally used after read started. > > > > Ah. Wonderful :-J. Thank you for the pointer (and for figuring all this out). > > > > I'd still like to take this opportunity to update the interface contract comment in HttpTransaction, and if the idea makes sense to you, switch over to a weak pointer as well. > > Sure. Updated the interface contract in HttpTransaction. > Regarding weak pointer, my thoughts are: > The idea behind resetting the request info to null explicitly is not just for fixing the lifetime issue but also because httpNetworkTransaction and HttpStream will now be shared across various consumers and request info may have information specific to a consumer, which we do not want, once sharing starts (which aligns nicely to starting to read the response body, which is also the time when request info is no longer needed). > Using weak pointer would require the request info to be converted to a shared pointer to be used, implying shared ownership while it is being used, which also has a performance overhead. For request info we do not need to have that performance implication as we do not require shared ownership semantics. Request info is owned by the calling context and used by the lower layers in the calling stack (as read-only) but it never needs to be used in any two contexts simultaneously. Being a const raw pointer here thus seems a better option because of its low cost and constness. The disadvantage obviously is the complexity to reset it at the right time but since this code has been running with the implicit assumption for a long time, I am not too worried about it. > Another reason to prefer a raw pointer is that it also contains upload_data_stream which cannot be shared since its ownership is unique. So even if we were to convert the request info to weak ptr , we would still have the upload_data_stream raw pointer. As discussed with Randy, my statement above regarding weak ptr being converted to a shared ptr is not true. I was assuming the weak ptr concept similar to the C++ 11 weak_ptr where a weak ptr needs to be converted to a shared ptr when it is used so that it is not deleted simultaneously. That concept is well suited for multi threaded scenarios while chrome's WeakPtr is for single threaded scenarios. Aside from that though, the other reason of resetting the request info to null explicitly not just for fixing the lifetime issue but also to allow sharing still holds.
Any chance we can do the upload progress plumbing in a separate CL?
On 2016/09/09 at 16:51:16, rdsmith wrote: > Any chance we can do the upload progress plumbing in a separate CL? If we don't it together, then we will need extra steps for upload progress plumbing during ResetRequestInfo. Sorry for the large number of files but most of the files that are changed for upload progress plumbing removal just remove the GetUploadProgress function and the header file.
On 2016/09/09 at 16:56:46, shivanisha wrote: > On 2016/09/09 at 16:51:16, rdsmith wrote: > > Any chance we can do the upload progress plumbing in a separate CL? > > If we don't it together, then we will need extra steps for upload progress plumbing during ResetRequestInfo. > > Sorry for the large number of files but most of the files that are changed for upload progress plumbing removal just remove the GetUploadProgress function and the header file. And the "extra steps" were also crashing one test so the changes weren't sufficient before the upload progress plumbing removal.
On 2016/09/09 at 16:58:26, shivanisha wrote: > On 2016/09/09 at 16:56:46, shivanisha wrote: > > On 2016/09/09 at 16:51:16, rdsmith wrote: > > > Any chance we can do the upload progress plumbing in a separate CL? > > > > If we don't it together, then we will need extra steps for upload progress plumbing during ResetRequestInfo. > > > > Sorry for the large number of files but most of the files that are changed for upload progress plumbing removal just remove the GetUploadProgress function and the header file. > > And the "extra steps" were also crashing one test so the changes weren't sufficient before the upload progress plumbing removal. Apologies for multiple patchsets having the same name. Please look at the latest one and the last one which was reviewed was Patch 6. I ended up with so many patchsets with the same name because one or the other test failed in the commit bots leading to iterative development but for the purpose of review the patch sets from 7 to 13 are not needed.
On 2016/09/09 16:56:46, shivanisha wrote: > On 2016/09/09 at 16:51:16, rdsmith wrote: > > Any chance we can do the upload progress plumbing in a separate CL? > > If we don't it together, then we will need extra steps for upload progress > plumbing during ResetRequestInfo. > > Sorry for the large number of files but most of the files that are changed for > upload progress plumbing removal just remove the GetUploadProgress function and > the header file. Ah, my apologies; I had missed that there was a dependency linkage.
On 2016/09/09 at 17:02:30, shivanisha wrote: > On 2016/09/09 at 16:58:26, shivanisha wrote: > > On 2016/09/09 at 16:56:46, shivanisha wrote: > > > On 2016/09/09 at 16:51:16, rdsmith wrote: > > > > Any chance we can do the upload progress plumbing in a separate CL? > > > > > > If we don't it together, then we will need extra steps for upload progress plumbing during ResetRequestInfo. > > > > > > Sorry for the large number of files but most of the files that are changed for upload progress plumbing removal just remove the GetUploadProgress function and the header file. > > > > And the "extra steps" were also crashing one test so the changes weren't sufficient before the upload progress plumbing removal. > > Apologies for multiple patchsets having the same name. Please look at the latest one and the last one which was reviewed was Patch 6. > I ended up with so many patchsets with the same name because one or the other test failed in the commit bots leading to iterative development but for the purpose of review the patch sets from 7 to 13 are not needed. Ah, Josh just pointed that a patch set can be removed :)
I'm happy with the request_info reset logic. I need to grok the upload progress plumbing more before I can comment on it, but I also don't expect my comments on that to be very useful (lack of knowledge) so if needed you're welcome to commit without me. Which means I'll stamp now :-}: LGTM. Please do mention the upload progress plumbing in the CL description; it's as likely as the request info resetting to be the important thing that someone needs to know if they come back to look at this CL in the future. https://codereview.chromium.org/2298823002/diff/250001/net/http/http_network_... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/2298823002/diff/250001/net/http/http_network_... net/http/http_network_transaction.cc:1634: // consumer in cases where it is shared for writing to the cache. Do nit, suggestion: I'd phrase this slightly differently, maybe something like "so that the transaction can be disassociated from its creating consumer in cases where it is shared for writing ...". That covers the sharing-without-creating-consumer-destroyed case as well. https://codereview.chromium.org/2298823002/diff/250001/net/http/http_network_... net/http/http_network_transaction.cc:1638: DCHECK_EQ(STATE_READ_BODY, next_state_); nit, suggestion: It feels weird to have a functionally one line function that's called from a single place and then has a DCHECK that basically means "make sure this is called from the one place from which it's called". My personal preference would be to have the request_ = nullptr assignment inline in HttpNetworkTransacation::Read() with appropriate comments. (I'd even suggest getting rid of the non-null test on request_; I think a unilateral assignment to nullptr is probably cheaper and no more confusing than doing the test for non-null before the assignment). https://codereview.chromium.org/2298823002/diff/250001/net/http/http_network_... File net/http/http_network_transaction.h (right): https://codereview.chromium.org/2298823002/diff/250001/net/http/http_network_... net/http/http_network_transaction.h:321: // Reset to null during the Read state machine. nit, suggestion: "... to null before the Read state ..."? Or maybe "before entering"? "during" makes it sounds like the state machine could have been going on for a while before the reset occurred. https://codereview.chromium.org/2298823002/diff/250001/net/http/http_stream_p... File net/http/http_stream_parser.cc (right): https://codereview.chromium.org/2298823002/diff/250001/net/http/http_stream_p... net/http/http_stream_parser.cc:385: // stream to outlive the consumer that initially created it. nit, suggestion: Similarly to before, emphasize that just the sharing requires disassociation from a particular request? https://codereview.chromium.org/2298823002/diff/250001/net/http/http_stream_p... net/http/http_stream_parser.cc:1239: DCHECK_EQ(STATE_READ_BODY, io_state_); nit, suggestion: Same as before, I'm not sure if this is best done in a separate method. https://codereview.chromium.org/2298823002/diff/250001/net/http/http_transact... File net/http/http_transaction.h (right): https://codereview.chromium.org/2298823002/diff/250001/net/http/http_transact... net/http/http_transaction.h:56: // final response headers are received. nit, suggestion: "; after that point, the HttpTransaction will not access |*request_info| and it may be deleted." https://codereview.chromium.org/2298823002/diff/250001/net/quic/chromium/quic... File net/quic/chromium/quic_http_stream.cc (right): https://codereview.chromium.org/2298823002/diff/250001/net/quic/chromium/quic... net/quic/chromium/quic_http_stream.cc:846: DCHECK_EQ(STATE_OPEN, next_state_); nit, suggestion: Separate routine needed? (Applies to all other instances as well, but still just a suggestion.)
Description was changed from ========== Resetting the HttpRequestInfo pointers in HttpNetworkTransaction and the underlying streams. This is being done to allow sharing of HttpNetworkTransaction across various HttpCache::Transactions which might involve the network transaction and streams to outlive the request info's owner HttpRequestInfo. This approach is taken instead of making a copy of request info because in addition to making the memory footprint larger (headers could be couple of thousands Kilobytes), it does not seem right to have a copy of a request info whose URLRequestHttpJob does not exist anymore. Also HttpRequestInfo contains a pointer to the upload data stream which cannot be copied and having its pointer around after its owner URLRequest has died is incorrect. BUG=472740 ========== to ========== Resetting the HttpRequestInfo pointers in HttpNetworkTransaction and the underlying streams. This is being done to allow sharing of HttpNetworkTransaction across various HttpCache::Transactions which might involve the network transaction and streams to outlive the request info's owner HttpRequestInfo. This approach is taken instead of making a copy of request info because in addition to making the memory footprint larger (headers could be couple of thousands Kilobytes), it does not seem right to have a copy of a request info whose URLRequestHttpJob does not exist anymore. Also HttpRequestInfo contains a pointer to the upload data stream which cannot be copied and having its pointer around after its owner URLRequest has died is incorrect. This CL also removes the plumbing to get upload progress since URLRequest owns the upload data stream and it can directly ask the stream for the progress instead of going to URLRequestJob, HttpTransaction and HttpStream layers. BUG=472740 ==========
Description was changed from ========== Resetting the HttpRequestInfo pointers in HttpNetworkTransaction and the underlying streams. This is being done to allow sharing of HttpNetworkTransaction across various HttpCache::Transactions which might involve the network transaction and streams to outlive the request info's owner HttpRequestInfo. This approach is taken instead of making a copy of request info because in addition to making the memory footprint larger (headers could be couple of thousands Kilobytes), it does not seem right to have a copy of a request info whose URLRequestHttpJob does not exist anymore. Also HttpRequestInfo contains a pointer to the upload data stream which cannot be copied and having its pointer around after its owner URLRequest has died is incorrect. This CL also removes the plumbing to get upload progress since URLRequest owns the upload data stream and it can directly ask the stream for the progress instead of going to URLRequestJob, HttpTransaction and HttpStream layers. BUG=472740 ========== to ========== Resetting the HttpRequestInfo pointers in HttpNetworkTransaction and the underlying streams. This is being done to allow sharing of HttpNetworkTransaction across various HttpCache::Transactions which might involve the network transaction and streams to outlive the request info's owner HttpRequestInfo. This approach is taken instead of making a copy of request info because in addition to making the memory footprint larger (headers could be couple of thousands Kilobytes), it does not seem right to have a copy of a request info whose URLRequestHttpJob does not exist anymore. Also HttpRequestInfo contains a pointer to the upload data stream which cannot be copied and having its pointer around after its owner URLRequest has died is incorrect. This CL also removes the plumbing to get upload progress since URLRequest owns the upload data stream and it can directly ask the stream for the progress instead of going to URLRequestJob, HttpTransaction and HttpStream layers. BUG=472740 ==========
Still LGTM. (Shivani and I had an offline discussion and concluded that the upload progress plumbing could indeed be split into a separate CL, so long as that CL landed before this one. I have a minor preference in that direction, but would be ok with either approach.) https://codereview.chromium.org/2298823002/diff/250001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2298823002/diff/250001/content/browser/loader... content/browser/loader/resource_dispatcher_host_unittest.cc:904: } nit, suggestion: Is there an existing test that makes sure these interfaces get exercised? (I.e. that a URLRequest with this set as it's upload data stream will actually try to upload some data?) My guess is "no" since it doesn't look like the URLRequests used for this test had an UploadDataStream set for them, which makes me worried that some future test writer will use this class, assuming it'll work, and it won't. If I'm right on this (I'm not certain) would you be willing to write a stub test using this class or use NOT_REACHED() inside the bodies of the {Init,Read}Internal functions to signal clearly to future modifiers what this class will and won't do?
I'll defer to Randy here.
Patchset #15 (id:270001) has been deleted
The CQ bit was checked by shivanisha@chromium.org 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...
Description was changed from ========== Resetting the HttpRequestInfo pointers in HttpNetworkTransaction and the underlying streams. This is being done to allow sharing of HttpNetworkTransaction across various HttpCache::Transactions which might involve the network transaction and streams to outlive the request info's owner HttpRequestInfo. This approach is taken instead of making a copy of request info because in addition to making the memory footprint larger (headers could be couple of thousands Kilobytes), it does not seem right to have a copy of a request info whose URLRequestHttpJob does not exist anymore. Also HttpRequestInfo contains a pointer to the upload data stream which cannot be copied and having its pointer around after its owner URLRequest has died is incorrect. This CL also removes the plumbing to get upload progress since URLRequest owns the upload data stream and it can directly ask the stream for the progress instead of going to URLRequestJob, HttpTransaction and HttpStream layers. BUG=472740 ========== to ========== Resetting the HttpRequestInfo pointers in HttpNetworkTransaction and the underlying streams. This is being done to allow sharing of HttpNetworkTransaction across various HttpCache::Transactions which might involve the network transaction and streams to outlive the request info's owner HttpRequestInfo. This approach is taken instead of making a copy of request info because in addition to making the memory footprint larger (headers could be couple of thousands Kilobytes), it does not seem right to have a copy of a request info whose URLRequestHttpJob does not exist anymore. Also HttpRequestInfo contains a pointer to the upload data stream which cannot be copied and having its pointer around after its owner URLRequest has died is incorrect. BUG=472740 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by shivanisha@chromium.org 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 checked by shivanisha@chromium.org to run a CQ dry run
Patchset #16 (id:310001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #15 (id:290001) has been deleted
Thanks for the review! Incorporated the latest feedback. https://codereview.chromium.org/2298823002/diff/250001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/2298823002/diff/250001/content/browser/loader... content/browser/loader/resource_dispatcher_host_unittest.cc:904: } This code is no longer needed as a result of an independent CL which changed the way these tests work. Also, Upload plumbing removal now being reviewed in https://codereview.chromium.org/2298823002/ https://codereview.chromium.org/2298823002/diff/250001/net/http/http_network_... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/2298823002/diff/250001/net/http/http_network_... net/http/http_network_transaction.cc:1634: // consumer in cases where it is shared for writing to the cache. Do On 2016/09/09 at 17:28:24, Randy Smith - Not in Fridays wrote: > nit, suggestion: I'd phrase this slightly differently, maybe something like "so that the transaction can be disassociated from its creating consumer in cases where it is shared for writing ...". That covers the sharing-without-creating-consumer-destroyed case as well. done. https://codereview.chromium.org/2298823002/diff/250001/net/http/http_network_... net/http/http_network_transaction.cc:1638: DCHECK_EQ(STATE_READ_BODY, next_state_); On 2016/09/09 at 17:28:24, Randy Smith - Not in Fridays wrote: > nit, suggestion: It feels weird to have a functionally one line function that's called from a single place and then has a DCHECK that basically means "make sure this is called from the one place from which it's called". My personal preference would be to have the request_ = nullptr assignment inline in HttpNetworkTransacation::Read() with appropriate comments. (I'd even suggest getting rid of the non-null test on request_; I think a unilateral assignment to nullptr is probably cheaper and no more confusing than doing the test for non-null before the assignment). Sounds good. done. https://codereview.chromium.org/2298823002/diff/250001/net/http/http_network_... File net/http/http_network_transaction.h (right): https://codereview.chromium.org/2298823002/diff/250001/net/http/http_network_... net/http/http_network_transaction.h:321: // Reset to null during the Read state machine. On 2016/09/09 at 17:28:24, Randy Smith - Not in Fridays wrote: > nit, suggestion: "... to null before the Read state ..."? Or maybe "before entering"? "during" makes it sounds like the state machine could have been going on for a while before the reset occurred. done. https://codereview.chromium.org/2298823002/diff/250001/net/http/http_stream_p... File net/http/http_stream_parser.cc (right): https://codereview.chromium.org/2298823002/diff/250001/net/http/http_stream_p... net/http/http_stream_parser.cc:385: // stream to outlive the consumer that initially created it. On 2016/09/09 at 17:28:24, Randy Smith - Not in Fridays wrote: > nit, suggestion: Similarly to before, emphasize that just the sharing requires disassociation from a particular request? done. https://codereview.chromium.org/2298823002/diff/250001/net/http/http_stream_p... net/http/http_stream_parser.cc:1239: DCHECK_EQ(STATE_READ_BODY, io_state_); On 2016/09/09 at 17:28:24, Randy Smith - Not in Fridays wrote: > nit, suggestion: Same as before, I'm not sure if this is best done in a separate method. done. https://codereview.chromium.org/2298823002/diff/250001/net/http/http_transact... File net/http/http_transaction.h (right): https://codereview.chromium.org/2298823002/diff/250001/net/http/http_transact... net/http/http_transaction.h:56: // final response headers are received. On 2016/09/09 at 17:28:24, Randy Smith - Not in Fridays wrote: > nit, suggestion: "; after that point, the HttpTransaction will not access |*request_info| and it may be deleted." done. https://codereview.chromium.org/2298823002/diff/250001/net/quic/chromium/quic... File net/quic/chromium/quic_http_stream.cc (right): https://codereview.chromium.org/2298823002/diff/250001/net/quic/chromium/quic... net/quic/chromium/quic_http_stream.cc:846: DCHECK_EQ(STATE_OPEN, next_state_); On 2016/09/09 at 17:28:24, Randy Smith - Not in Fridays wrote: > nit, suggestion: Separate routine needed? (Applies to all other instances as well, but still just a suggestion.) done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Still LGTM. Thanks!
The CQ bit was checked by shivanisha@chromium.org 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: 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-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by shivanisha@chromium.org 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.
The CQ bit was checked by shivanisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org Link to the patchset: https://codereview.chromium.org/2298823002/#ps350001 (title: "Rebased till refs/heads/master@{#425377}")
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 #16 (id:350001)
Message was sent while issue was closed.
Description was changed from ========== Resetting the HttpRequestInfo pointers in HttpNetworkTransaction and the underlying streams. This is being done to allow sharing of HttpNetworkTransaction across various HttpCache::Transactions which might involve the network transaction and streams to outlive the request info's owner HttpRequestInfo. This approach is taken instead of making a copy of request info because in addition to making the memory footprint larger (headers could be couple of thousands Kilobytes), it does not seem right to have a copy of a request info whose URLRequestHttpJob does not exist anymore. Also HttpRequestInfo contains a pointer to the upload data stream which cannot be copied and having its pointer around after its owner URLRequest has died is incorrect. BUG=472740 ========== to ========== Resetting the HttpRequestInfo pointers in HttpNetworkTransaction and the underlying streams. This is being done to allow sharing of HttpNetworkTransaction across various HttpCache::Transactions which might involve the network transaction and streams to outlive the request info's owner HttpRequestInfo. This approach is taken instead of making a copy of request info because in addition to making the memory footprint larger (headers could be couple of thousands Kilobytes), it does not seem right to have a copy of a request info whose URLRequestHttpJob does not exist anymore. Also HttpRequestInfo contains a pointer to the upload data stream which cannot be copied and having its pointer around after its owner URLRequest has died is incorrect. BUG=472740 Committed: https://crrev.com/0b44085daafa86fe2e6cb11b369c3120082af8dc Cr-Commit-Position: refs/heads/master@{#425974} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/0b44085daafa86fe2e6cb11b369c3120082af8dc Cr-Commit-Position: refs/heads/master@{#425974} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
