Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1732493002:
Prevent URLFetcher::AppendChunkedData from dereferencing NULL pointers. (Closed)
DescriptionPrevent URLFetcher::AppendChunkedData from dereferencing NULL pointers.
Previously, this would happen if it was called after a request failed,
while a request was being throttled, or if a request was redirected and
lost its body without having sent the entire upload body (Either
redirected by stuff outside net with never uploading the body, or
redirected by stuff outside net after a network error).
BUG=568091
Committed: https://crrev.com/56b0cbb91b8ae09e2304eb8e6503ebd729332c35
Cr-Commit-Position: refs/heads/master@{#383571}
Patch Set 1 #Patch Set 2 : fix #Patch Set 3 : Fix stuff #Patch Set 4 : Fix Cronet #
Total comments: 14
Patch Set 5 : Merge #
Total comments: 4
Patch Set 6 : Response to comments #Patch Set 7 : Missed one #Patch Set 8 : Merge #Patch Set 9 : Merge #
Total comments: 17
Patch Set 10 : Resposne to comments #Patch Set 11 : Remove bonus close paren #Patch Set 12 : merge #
Total comments: 12
Patch Set 13 : Response to comments #Patch Set 14 : Remove unused variable #
Messages
Total messages: 36 (14 generated)
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:60001) has been deleted
mmenke@chromium.org changed reviewers: + mef@chromium.org
Misha: Mind reviewing? Sending this to you because you did my upload refactoring reviews, and this touches Cronet, though you presumably aren't too familiar with URLFetcher. https://codereview.chromium.org/1732493002/diff/120001/net/url_request/url_fe... File net/url_request/url_fetcher_core.cc (right): https://codereview.chromium.org/1732493002/diff/120001/net/url_request/url_fe... net/url_request/url_fetcher_core.cc:484: ReleaseRequest(); Bug 2: On error, we release the request without setting cancelled_ to true, so the embedder may continue appending data before it knows about the failure, and since cancelled_ isn't set to true, URLFetcherCore::CompleteAddingUploadDataChunk will try to pass the data to the deleted URLRequest. https://codereview.chromium.org/1732493002/diff/120001/net/url_request/url_fe... net/url_request/url_fetcher_core.cc:534: DidInitializeWriter(result); Bug 1: Note that this can call DidInitializeWriter asynchronously, which creates the URLRequest. If that happens, appending data before that method runs will result in a crash.
Sorry about delay, I was doing bug triage. I'm indeed not very familiar with URLFetcher, so please pardon my ignorant questions. https://codereview.chromium.org/1732493002/diff/120001/components/cronet/andr... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/1732493002/diff/120001/components/cronet/andr... components/cronet/android/url_request_adapter.cc:134: if (!chunked_upload_writer_->AppendData(bytes.get(), bytes_len, Why we don't need !url_request_ check anymore? https://codereview.chromium.org/1732493002/diff/120001/components/cronet/andr... File components/cronet/android/url_request_adapter.h (right): https://codereview.chromium.org/1732493002/diff/120001/components/cronet/andr... components/cronet/android/url_request_adapter.h:34: class URLRequestAdapter : public net::URLRequest::Delegate { This is old API, do we need similar change for new API? https://codereview.chromium.org/1732493002/diff/120001/net/base/chunked_uploa... File net/base/chunked_upload_data_stream.cc (right): https://codereview.chromium.org/1732493002/diff/120001/net/base/chunked_uploa... net/base/chunked_upload_data_stream.cc:18: if (!upload_data_stream_) Could this be racy if upload_data_stream_ is deleted on the network thread while AppendData is running on ChunkedUpload thread? https://codereview.chromium.org/1732493002/diff/120001/net/url_request/url_fe... File net/url_request/url_fetcher_core.h (right): https://codereview.chromium.org/1732493002/diff/120001/net/url_request/url_fe... net/url_request/url_fetcher_core.h:275: bool is_chunked_upload_; // True if using chunked transfer encoding do we need this bool or could it be replaced by chunked_stream_writer_?
On 2016/03/03 16:49:33, mef wrote: > Sorry about delay, I was doing bug triage. I'm indeed not very familiar with > URLFetcher, so please pardon my ignorant questions. Nothing to apologize for, this is a low priority fix, and given the amount of code it touches, your response was much faster than I anticipated. And happy to answer questions - no one knows this code that well. Much happier answering them than getting a rubberstamp, even if it ends up no changes are needed. https://codereview.chromium.org/1732493002/diff/120001/components/cronet/andr... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/1732493002/diff/120001/components/cronet/andr... components/cronet/android/url_request_adapter.cc:134: if (!chunked_upload_writer_->AppendData(bytes.get(), bytes_len, On 2016/03/03 16:49:32, mef wrote: > Why we don't need !url_request_ check anymore? Because chunked_upload_writer_ wraps a WeakPtr. If the request has been deleted, it will notice the WeakPtr is NULL. Keeping it would be redundant. https://codereview.chromium.org/1732493002/diff/120001/components/cronet/andr... File components/cronet/android/url_request_adapter.h (right): https://codereview.chromium.org/1732493002/diff/120001/components/cronet/andr... components/cronet/android/url_request_adapter.h:34: class URLRequestAdapter : public net::URLRequest::Delegate { On 2016/03/03 16:49:32, mef wrote: > This is old API, do we need similar change for new API? The new API doesn't use URLRequest::AppendData, so it's fine. https://codereview.chromium.org/1732493002/diff/120001/net/base/chunked_uploa... File net/base/chunked_upload_data_stream.cc (right): https://codereview.chromium.org/1732493002/diff/120001/net/base/chunked_uploa... net/base/chunked_upload_data_stream.cc:18: if (!upload_data_stream_) On 2016/03/03 16:49:32, mef wrote: > Could this be racy if upload_data_stream_ is deleted on the network thread while > AppendData is running on ChunkedUpload thread? Neither of these classes is threadsafe - this is all happening on the network thread. https://codereview.chromium.org/1732493002/diff/120001/net/url_request/url_fe... File net/url_request/url_fetcher_core.h (right): https://codereview.chromium.org/1732493002/diff/120001/net/url_request/url_fe... net/url_request/url_fetcher_core.h:275: bool is_chunked_upload_; // True if using chunked transfer encoding On 2016/03/03 16:49:32, mef wrote: > do we need this bool or could it be replaced by chunked_stream_writer_? ChunkedUploadDataStreams are non-thread-safe, and have to be used on the thread they're created on. These values are all initialized on the embedder thread, not the network thread. We could post a task over to the network thread to set the chunked_stream_writer_, but then we couldn't DCHECK on it... Also, unfortunately (Very unfortunately), URLFetchers can be reused, and they keep this value between uses, but can't keep chunked_stream_writer_, and I'm reluctant to change the API. I do want to get rid of the ability for these to be reused, but that would probably be a major undertaking. :(
https://codereview.chromium.org/1732493002/diff/120001/net/base/chunked_uploa... File net/base/chunked_upload_data_stream.cc (right): https://codereview.chromium.org/1732493002/diff/120001/net/base/chunked_uploa... net/base/chunked_upload_data_stream.cc:18: if (!upload_data_stream_) On 2016/03/03 17:04:45, mmenke wrote: > On 2016/03/03 16:49:32, mef wrote: > > Could this be racy if upload_data_stream_ is deleted on the network thread > while > > AppendData is running on ChunkedUpload thread? > > Neither of these classes is threadsafe - this is all happening on the network > thread. Ack, for some reason I thought that these are called on different thread. Thanks for bearing with me! https://codereview.chromium.org/1732493002/diff/120001/net/url_request/url_fe... File net/url_request/url_fetcher_core.h (right): https://codereview.chromium.org/1732493002/diff/120001/net/url_request/url_fe... net/url_request/url_fetcher_core.h:275: bool is_chunked_upload_; // True if using chunked transfer encoding On 2016/03/03 17:04:45, mmenke wrote: > On 2016/03/03 16:49:32, mef wrote: > > do we need this bool or could it be replaced by chunked_stream_writer_? > > ChunkedUploadDataStreams are non-thread-safe, and have to be used on the thread > they're created on. These values are all initialized on the embedder thread, > not the network thread. > > We could post a task over to the network thread to set the > chunked_stream_writer_, but then we couldn't DCHECK on it... Also, > unfortunately (Very unfortunately), URLFetchers can be reused, and they keep > this value between uses, but can't keep chunked_stream_writer_, and I'm > reluctant to change the API. > > I do want to get rid of the ability for these to be reused, but that would > probably be a major undertaking. :( So, if URLFetcher is re-used, would we need to create another chunked_stream_ to go with new UrlRequest? https://codereview.chromium.org/1732493002/diff/140001/net/url_request/url_fe... File net/url_request/url_fetcher_core.cc (right): https://codereview.chromium.org/1732493002/diff/140001/net/url_request/url_fe... net/url_request/url_fetcher_core.cc:565: request_->set_upload(make_scoped_ptr(chunked_stream_.release())); can this use std::move? https://codereview.chromium.org/1732493002/diff/140001/net/url_request/url_re... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/1732493002/diff/140001/net/url_request/url_re... net/url_request/url_request_unittest.cc:5792: r->set_upload(make_scoped_ptr(upload_data_stream.release())); would std::move work here?
https://codereview.chromium.org/1732493002/diff/120001/net/base/chunked_uploa... File net/base/chunked_upload_data_stream.cc (right): https://codereview.chromium.org/1732493002/diff/120001/net/base/chunked_uploa... net/base/chunked_upload_data_stream.cc:18: if (!upload_data_stream_) On 2016/03/08 17:19:22, mef wrote: > On 2016/03/03 17:04:45, mmenke wrote: > > On 2016/03/03 16:49:32, mef wrote: > > > Could this be racy if upload_data_stream_ is deleted on the network thread > > while > > > AppendData is running on ChunkedUpload thread? > > > > Neither of these classes is threadsafe - this is all happening on the network > > thread. > > Ack, for some reason I thought that these are called on different thread. Thanks > for bearing with me! The URLFetcherCore runs on two threads, but all it does on the non-network-stack thread is set state before the request starts. Its methods that are meant to be called after the request starts just post messages over to the network thread. Everything else lives only on the network thread, with the exception of the URLFetcher, which is used strictly on the consumers thread (Which may be the network thread, or may not). https://codereview.chromium.org/1732493002/diff/140001/net/url_request/url_fe... File net/url_request/url_fetcher_core.cc (right): https://codereview.chromium.org/1732493002/diff/140001/net/url_request/url_fe... net/url_request/url_fetcher_core.cc:565: request_->set_upload(make_scoped_ptr(chunked_stream_.release())); On 2016/03/08 17:19:22, mef wrote: > can this use std::move? Done. For some reason, I was thinking std::move had trouble when it needs to upcast. MSVC underlines it in red, but compiles it fine, maybe that was it. https://codereview.chromium.org/1732493002/diff/140001/net/url_request/url_re... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/1732493002/diff/140001/net/url_request/url_re... net/url_request/url_request_unittest.cc:5792: r->set_upload(make_scoped_ptr(upload_data_stream.release())); On 2016/03/08 17:19:22, mef wrote: > would std::move work here? Done.
https://codereview.chromium.org/1732493002/diff/120001/net/url_request/url_fe... File net/url_request/url_fetcher_core.h (right): https://codereview.chromium.org/1732493002/diff/120001/net/url_request/url_fe... net/url_request/url_fetcher_core.h:275: bool is_chunked_upload_; // True if using chunked transfer encoding On 2016/03/08 17:19:22, mef wrote: > On 2016/03/03 17:04:45, mmenke wrote: > > On 2016/03/03 16:49:32, mef wrote: > > > do we need this bool or could it be replaced by chunked_stream_writer_? > > > > ChunkedUploadDataStreams are non-thread-safe, and have to be used on the > thread > > they're created on. These values are all initialized on the embedder thread, > > not the network thread. > > > > We could post a task over to the network thread to set the > > chunked_stream_writer_, but then we couldn't DCHECK on it... Also, > > unfortunately (Very unfortunately), URLFetchers can be reused, and they keep > > this value between uses, but can't keep chunked_stream_writer_, and I'm > > reluctant to change the API. > > > > I do want to get rid of the ability for these to be reused, but that would > > probably be a major undertaking. :( > > So, if URLFetcher is re-used, would we need to create another chunked_stream_ to > go with new UrlRequest? Oops...Missed this. Yes, we would.
Misha: Ping? No rush here, just want to make sure it's still on your radar.
On 2016/03/15 15:58:38, mmenke wrote: > Misha: Ping? No rush here, just want to make sure it's still on your radar. Misha: I'll go ahead and find someone else for the review.
On 2016/03/22 15:08:03, mmenke wrote: > On 2016/03/15 15:58:38, mmenke wrote: > > Misha: Ping? No rush here, just want to make sure it's still on your radar. > > Misha: I'll go ahead and find someone else for the review. Oh, sorry about that, I got swamped by perf and ios stuff.
On 2016/03/22 15:13:14, mef wrote: > On 2016/03/22 15:08:03, mmenke wrote: > > On 2016/03/15 15:58:38, mmenke wrote: > > > Misha: Ping? No rush here, just want to make sure it's still on your > radar. > > > > Misha: I'll go ahead and find someone else for the review. > > Oh, sorry about that, I got swamped by perf and ios stuff. No problem. I looked at your calendar. Hadn't realized how busy you were with manager-y stuff.
Description was changed from ========== Prevent URLFetcher::AppendChunkedData from dereferencing NULL pointers. Previously, this would happen if it was called after a request failed, while a request was being throttled, or if a request was redirected and lost its body without having sent the entire upload body (Either redirected by stuff outside net with never uploading the body, or redirected by stuff outside net after a network error). BUG=568091 ========== to ========== Prevent URLFetcher::AppendChunkedData from dereferencing NULL pointers. Previously, this would happen if it was called after a request failed, while a request was being throttled, or if a request was redirected and lost its body without having sent the entire upload body (Either redirected by stuff outside net with never uploading the body, or redirected by stuff outside net after a network error). BUG=568091 ==========
mmenke@chromium.org changed reviewers: - mef@chromium.org
mmenke@chromium.org changed reviewers: + eroman@chromium.org
[+eroman]: Eric, mind reviewing? This is a fix for a pretty obscure crash, and I took the opportunity to remove a method from URLRequest's interface as well. No real experts here, and people who reviewed my last two CLs in this area (David for URLFetcher and Misha for UploadDataStream) are currently overloaded.
sure, will take a look later today
https://codereview.chromium.org/1732493002/diff/220001/components/cronet/andr... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/1732493002/diff/220001/components/cronet/andr... components/cronet/android/url_request_adapter.cc:133: // while appendChunk was posting the task from an application thread. This comment and subsequent VLOG() don't seem congruent with the new condition. https://codereview.chromium.org/1732493002/diff/220001/components/cronet/andr... components/cronet/android/url_request_adapter.cc:172: url_request_->set_upload(std::move(chunked_upload_data_stream)); This looked wrong when first reading it because of the apparent reference cycle. I figured the writer would be holding a weak ptr, but worth adding a comment IMO. https://codereview.chromium.org/1732493002/diff/220001/net/base/chunked_uploa... File net/base/chunked_upload_data_stream.h (right): https://codereview.chromium.org/1732493002/diff/220001/net/base/chunked_uploa... net/base/chunked_upload_data_stream.h:43: // Once called with |is_done| being true, must never be called again. This description of is_done is different than that for the underlying ChunkedUploadDataStream. Any reason for that? https://codereview.chromium.org/1732493002/diff/220001/net/base/chunked_uploa... net/base/chunked_upload_data_stream.h:44: // Returns true if write succeeded, false if it failed (Generally because Might be worth clarifying that true doesn't mean the write truly succeeded (in the sense of being received by server), just that it passed it on successfully to next layer. https://codereview.chromium.org/1732493002/diff/220001/net/base/chunked_uploa... net/base/chunked_upload_data_stream.h:46: bool AppendData(const char* data, int data_len, bool is_done); Rather than having this is_done boolean, have you considered having a Close() method instead? I guess this is to mirror ChunkedUploadDataStream having a similar signature, but that one also looks bizarro to my eyes. https://codereview.chromium.org/1732493002/diff/220001/net/base/chunked_uploa... net/base/chunked_upload_data_stream.h:63: scoped_ptr<Writer> CreateWriter(); What is the effect of creating multiple writers? Is the intent to only ever have one outstanding writer per ChunkedUploadDataStream? Are we going to run into problems if multiple writers are created, and one of them closes (is_done=true) while the other keeps writing. Will this end up in returning false, or will it violate the invariant below that ChunkedUploadDataStream::AppendData() never be called after appending is_done = true? Can you add some guidelines on use in the function's comments? https://codereview.chromium.org/1732493002/diff/220001/net/base/chunked_uploa... net/base/chunked_upload_data_stream.h:95: base::WeakPtrFactory<ChunkedUploadDataStream> weak_factory_; Generally use of weak-pointers feels like we failed in our ownership model somewhere, and is papering over a design problem. I haven't thought about this hard enough to suggest alternatives, and this approach does seems reasonable as described, so I am not against it. But I would be interested in what other approaches you considered, as I am still kind of paging in this code :) https://codereview.chromium.org/1732493002/diff/220001/net/url_request/url_fe... File net/url_request/url_fetcher_core.cc (right): https://codereview.chromium.org/1732493002/diff/220001/net/url_request/url_fe... net/url_request/url_fetcher_core.cc:523: if (is_chunked_upload_) { What is the gap between when this runs, and when StartURLRequest() runs? Does this actually need to be set up here, will users be able to append data to chunked_stream_ before it is moved to the URLRequest ? This seems weird in that ChunkedUploadDataStream() is itself just a buffer that queues up the data. So we have a writer interface to ChunkedUploadDataStream, and then URLRequest pulls data from that. I will have to dig deeper into how we do the upload abstractions to comment better on this, although your feedback will speed that up :)
Thanks for the thought out comments! https://codereview.chromium.org/1732493002/diff/220001/components/cronet/andr... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/1732493002/diff/220001/components/cronet/andr... components/cronet/android/url_request_adapter.cc:133: // while appendChunk was posting the task from an application thread. On 2016/03/22 19:05:37, eroman wrote: > This comment and subsequent VLOG() don't seem congruent with the new condition. Comment seems correct - you can actually succeed without completing the upload (With, for instance, a 413 - we check for a response after the connection has been closed for just that case), but it's uncommon. I've restated the comment, and removed the VLOG - I don't think it's that useful. https://codereview.chromium.org/1732493002/diff/220001/components/cronet/andr... components/cronet/android/url_request_adapter.cc:172: url_request_->set_upload(std::move(chunked_upload_data_stream)); On 2016/03/22 19:05:37, eroman wrote: > This looked wrong when first reading it because of the apparent reference cycle. > > I figured the writer would be holding a weak ptr, but worth adding a comment > IMO. Done. https://codereview.chromium.org/1732493002/diff/220001/net/base/chunked_uploa... File net/base/chunked_upload_data_stream.h (right): https://codereview.chromium.org/1732493002/diff/220001/net/base/chunked_uploa... net/base/chunked_upload_data_stream.h:43: // Once called with |is_done| being true, must never be called again. On 2016/03/22 19:05:37, eroman wrote: > This description of is_done is different than that for the underlying > ChunkedUploadDataStream. Any reason for that? It's the same, word-for-word, as the comment before ChunkedUploadDataStream::AppendData. Or am I missing something? https://codereview.chromium.org/1732493002/diff/220001/net/base/chunked_uploa... net/base/chunked_upload_data_stream.h:44: // Returns true if write succeeded, false if it failed (Generally because On 2016/03/22 19:05:37, eroman wrote: > Might be worth clarifying that true doesn't mean the write truly succeeded (in > the sense of being received by server), just that it passed it on successfully > to next layer. Done. https://codereview.chromium.org/1732493002/diff/220001/net/base/chunked_uploa... net/base/chunked_upload_data_stream.h:46: bool AppendData(const char* data, int data_len, bool is_done); On 2016/03/22 19:05:37, eroman wrote: > Rather than having this is_done boolean, have you considered having a Close() > method instead? > > I guess this is to mirror ChunkedUploadDataStream having a similar signature, > but that one also looks bizarro to my eyes. Yea, it's to mirror ChunkedUploadDataStream's method, which mirrors URLFetcher's method, which is what all consumers of this class (indirectly) use. I'm not opposed to splitting them, just think if we do that, it should be done at all layers, and is beyond the scoped of this CL. The main reason it's one method, I believe, is so we can combine our final two chunks - i.e., the terminal empty chunk can be sent in the same over-the-wire packet as the previous chunk. This is used rarely enough that I doubt that gets us a whole lot. https://codereview.chromium.org/1732493002/diff/220001/net/base/chunked_uploa... net/base/chunked_upload_data_stream.h:63: scoped_ptr<Writer> CreateWriter(); On 2016/03/22 19:05:37, eroman wrote: > What is the effect of creating multiple writers? Is the intent to only ever have > one outstanding writer per ChunkedUploadDataStream? > > Are we going to run into problems if multiple writers are created, and one of > them closes (is_done=true) while the other keeps writing. Will this end up in > returning false, or will it violate the invariant below that > ChunkedUploadDataStream::AppendData() never be called after appending is_done = > true? > > Can you add some guidelines on use in the function's comments? Added a comment (Allowing multiple writers, mostly because I don't feel like tracking if a writer has been created, and DCHECKing it's false, to enforce only one writer at a time). https://codereview.chromium.org/1732493002/diff/220001/net/base/chunked_uploa... net/base/chunked_upload_data_stream.h:95: base::WeakPtrFactory<ChunkedUploadDataStream> weak_factory_; On 2016/03/22 19:05:37, eroman wrote: > Generally use of weak-pointers feels like we failed in our ownership model > somewhere, and is papering over a design problem. > > I haven't thought about this hard enough to suggest alternatives, and this > approach does seems reasonable as described, so I am not against it. > > But I would be interested in what other approaches you considered, as I am still > kind of paging in this code :) First off, let's discuss design constraints: 1) Let's stick with a push-based model. I'd love to make things completely pull-based instead, but that would require rewriting every consumer to do properly, and seems beyond the scope of this CL. 2) We need to be able to get data before the URLRequest is started, so we either need to create the URLRequest before the throttle starts the request (After a delay), or we need to buffer the data at the URLFetcher::Core layer, or we need to start out with the URLFetcher::Core owning the UploadDataStream. Either the first or last approaches seem fine here, adding another layer that does its own buffering seems like a bad idea - more code, more stuff to go wrong. 3) I really want to remove URLRequest::AppendData - I don't think the URLRequest should care what UploadDataStream subclass we're passing in. I could list some options that keep URLRequest::AppendData, or some version thereof, but am choosing to skip over them, for this reason. Happy to talk about them, if you want. 4) So either URLRequest or the URLRequest consumer has to own the UploadDataStream. I don't like the idea of switching to having the consumer own it, since it adds destruction ordering dependencies and just seems to make things messy. Currently URLRequest owns everything it uses, outside the URLRequestContext and the URLRequest::Delegate itself (Which typically owns the URLRequest itself), which seems like a fairly simple and robust ownership model. So that, along with the push-based model, means URLFetcher::Core needs to talk to something it doesn't own. We could keep a raw pointer to it (Either by starting with a scoped_ptr, or by creating the URLRequest earlier, and passing ownership to it of the UploadDataStream just before we check if we should throttle the request). Then we just check if the URLRequest is NULL before writing...but that's basically the same as the weak pointer approach, only without explicitly using the weak ptr. We could go through SupportsUserData to set an UploadData stream pointer on the URLRequest, and grab it as needed. I'm not a fan of SupportsUserData + casting, but it works, and no weak pointers needed. ...And there we are, unless we relax the design constraints. Ignoring constraint 4) lets the URLFetcher own the UploadDataStream, and pass a ray pointer to URLRequest, though we'd have to update a bunch of consumers to support that. If that were a solid design choice, I wouldn't mind doing that, but I think it's a big step in the wrong direction. If we ignore constraint 3), keeping URLRequest::AppendData, then we just create the URLRequest early, and call AppendData as needed, null checking the URLRequest before we call it. Everything magically works. I just don't think AppendData should be part of the URLRequest interface. I don't think mucking with constraint 2) gets us much. If we ignore constraint 1), we could have something that implements an interface much like (Exactly like?) UploadDataStream itself, that the consumers provide. That would be a pretty big CL, the consumers would then be responsible for the ownership model...And presumably use their own weak pointers, except the UploadDataStream subclass would be the thing owning the weak pointers. Or we could ignore constraint 1), and have URLFetcherDelegate have its own callbacks to stream the data. No new weak pointer would be needed, since it would piggy back off of URLFetcher::Core's weak pointer to URLFetcher. Would need redesigns similar to the other option when ignoring constraint 1). So basically, all the options I see either involve directly or indirectly using a weak pointer of some sort, other than using SupportsUserData, which I view as worse, or having URLRequest use a raw pointer to UploadDataStream, which I also view as worse. https://codereview.chromium.org/1732493002/diff/220001/net/url_request/url_fe... File net/url_request/url_fetcher_core.cc (right): https://codereview.chromium.org/1732493002/diff/220001/net/url_request/url_fe... net/url_request/url_fetcher_core.cc:523: if (is_chunked_upload_) { On 2016/03/22 19:05:37, eroman wrote: > What is the gap between when this runs, and when StartURLRequest() runs? > > Does this actually need to be set up here, will users be able to append data to > chunked_stream_ before it is moved to the URLRequest ? > > This seems weird in that ChunkedUploadDataStream() is itself just a buffer that > queues up the data. So we have a writer interface to ChunkedUploadDataStream, > and then URLRequest pulls data from that. > > I will have to dig deeper into how we do the upload abstractions to comment > better on this, although your feedback will speed that up :) There are two gaps: The ResponseWriter may take a while to initialize (Open + create file, or whatever), and in DidInitializeWriter -> StartURLRequestWhenAppropriate, we may delay the request based on the ThrottleManager (A class I would love to kill). We could move URLRequest creation up here, and have the URLRequest exist for the duration of both those things. I'm fine with doing that refactor in this CL.
https://codereview.chromium.org/1732493002/diff/220001/net/base/chunked_uploa... File net/base/chunked_upload_data_stream.h (right): https://codereview.chromium.org/1732493002/diff/220001/net/base/chunked_uploa... net/base/chunked_upload_data_stream.h:95: base::WeakPtrFactory<ChunkedUploadDataStream> weak_factory_; On 2016/03/22 20:01:50, mmenke wrote: > On 2016/03/22 19:05:37, eroman wrote: > > Generally use of weak-pointers feels like we failed in our ownership model > > somewhere, and is papering over a design problem. > > > > I haven't thought about this hard enough to suggest alternatives, and this > > approach does seems reasonable as described, so I am not against it. > > > > But I would be interested in what other approaches you considered, as I am > still > > kind of paging in this code :) > > First off, let's discuss design constraints: > > 1) Let's stick with a push-based model. I'd love to make things completely > pull-based instead, but that would require rewriting every consumer to do > properly, and seems beyond the scope of this CL. > > 2) We need to be able to get data before the URLRequest is started, so we either > need to create the URLRequest before the throttle starts the request (After a > delay), or we need to buffer the data at the URLFetcher::Core layer, or we need > to start out with the URLFetcher::Core owning the UploadDataStream. Either the > first or last approaches seem fine here, adding another layer that does its own > buffering seems like a bad idea - more code, more stuff to go wrong. > > 3) I really want to remove URLRequest::AppendData - I don't think the URLRequest > should care what UploadDataStream subclass we're passing in. I could list some > options that keep URLRequest::AppendData, or some version thereof, but am > choosing to skip over them, for this reason. Happy to talk about them, if you > want. > > 4) So either URLRequest or the URLRequest consumer has to own the > UploadDataStream. I don't like the idea of switching to having the consumer own > it, since it adds destruction ordering dependencies and just seems to make > things messy. Currently URLRequest owns everything it uses, outside the > URLRequestContext and the URLRequest::Delegate itself (Which typically owns the > URLRequest itself), which seems like a fairly simple and robust ownership model. > > So that, along with the push-based model, means URLFetcher::Core needs to talk > to something it doesn't own. We could keep a raw pointer to it (Either by > starting with a scoped_ptr, or by creating the URLRequest earlier, and passing > ownership to it of the UploadDataStream just before we check if we should > throttle the request). Then we just check if the URLRequest is NULL before > writing...but that's basically the same as the weak pointer approach, only > without explicitly using the weak ptr. > > We could go through SupportsUserData to set an UploadData stream pointer on the > URLRequest, and grab it as needed. I'm not a fan of SupportsUserData + casting, > but it works, and no weak pointers needed. > > ...And there we are, unless we relax the design constraints. > > Ignoring constraint 4) lets the URLFetcher own the UploadDataStream, and pass a > ray pointer to URLRequest, though we'd have to update a bunch of consumers to > support that. If that were a solid design choice, I wouldn't mind doing that, > but I think it's a big step in the wrong direction. > > If we ignore constraint 3), keeping URLRequest::AppendData, then we just create > the URLRequest early, and call AppendData as needed, null checking the > URLRequest before we call it. Everything magically works. I just don't think > AppendData should be part of the URLRequest interface. > > I don't think mucking with constraint 2) gets us much. > > If we ignore constraint 1), we could have something that implements an interface > much like (Exactly like?) UploadDataStream itself, that the consumers provide. > That would be a pretty big CL, the consumers would then be responsible for the > ownership model...And presumably use their own weak pointers, except the > UploadDataStream subclass would be the thing owning the weak pointers. > > Or we could ignore constraint 1), and have URLFetcherDelegate have its own > callbacks to stream the data. No new weak pointer would be needed, since it > would piggy back off of URLFetcher::Core's weak pointer to URLFetcher. Would > need redesigns similar to the other option when ignoring constraint 1). > > So basically, all the options I see either involve directly or indirectly using > a weak pointer of some sort, other than using SupportsUserData, which I view as > worse, or having URLRequest use a raw pointer to UploadDataStream, which I also > view as worse. Ah, right...Keeping AppendData as a URLRequest method, and keeping URLRequest's dependency on ChunkedUploadDataStream (Mentioned as ignoring constraint 3) also lets us avoid a weak ptr. I'm of the opinion that the URLRequest interface has too much bloat, and really want to remove cruft from it, but I can live with that option.
lgtm https://codereview.chromium.org/1732493002/diff/280001/chrome/browser/profile... File chrome/browser/profiles/profile_browsertest.cc (left): https://codereview.chromium.org/1732493002/diff/280001/chrome/browser/profile... chrome/browser/profiles/profile_browsertest.cc:530: IN_PROC_BROWSER_TEST_F(ProfileBrowserTest, Why the removals? https://codereview.chromium.org/1732493002/diff/280001/net/base/chunked_uploa... File net/base/chunked_upload_data_stream.h (right): https://codereview.chromium.org/1732493002/diff/280001/net/base/chunked_uploa... net/base/chunked_upload_data_stream.h:31: // ChunkedUploadDataStream. It's needed because URLRequest owns the Before jumping into the "why" it's needed, I suggest mentioning explicitly the mechanism it supports -- that the Writer can outlive the ChunkedUploadDataStream / URLRequest that it is associated with. This is implied from the following comment, but I think t would be useful to see it stated explicitly either here, or in the description of CreateWriter(). https://codereview.chromium.org/1732493002/diff/280001/net/url_request/url_fe... File net/url_request/url_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/1732493002/diff/280001/net/url_request/url_fe... net/url_request/url_fetcher_impl_unittest.cc:831: base::PlatformThread::Sleep(TestTimeouts::tiny_timeout()); Hmm this is subtle. Is there a way to run this test on the IO thread and pump the IO loop as needed to reach this condition? https://codereview.chromium.org/1732493002/diff/280001/net/url_request/url_fe... net/url_request/url_fetcher_impl_unittest.cc:1146: EXPECT_GE(Time::Now() - start_time, base::TimeDelta::FromSeconds(1)); Is this timing exact, or can this fail? Would it be better to leave this off? https://codereview.chromium.org/1732493002/diff/280001/net/url_request/url_re... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/1732493002/diff/280001/net/url_request/url_re... net/url_request/url_request_unittest.cc:5741: // Add a standard set of data to an upload for chunked upload integration nit: Adds https://codereview.chromium.org/1732493002/diff/280001/net/url_request/url_re... net/url_request/url_request_unittest.cc:5752: // Check that the upload data added in AddChunksToUpload() was echoed back from nit: Checks
Thanks! https://codereview.chromium.org/1732493002/diff/280001/chrome/browser/profile... File chrome/browser/profiles/profile_browsertest.cc (left): https://codereview.chromium.org/1732493002/diff/280001/chrome/browser/profile... chrome/browser/profiles/profile_browsertest.cc:530: IN_PROC_BROWSER_TEST_F(ProfileBrowserTest, On 2016/03/26 00:55:16, eroman wrote: > Why the removals? git meltdown...These are from another CL...Grr. Removed from this CL. Too easy to commit to master instead of a branch, and then once you pull from master, even if you fix master, you still have the leftovers. :( https://codereview.chromium.org/1732493002/diff/280001/net/base/chunked_uploa... File net/base/chunked_upload_data_stream.h (right): https://codereview.chromium.org/1732493002/diff/280001/net/base/chunked_uploa... net/base/chunked_upload_data_stream.h:31: // ChunkedUploadDataStream. It's needed because URLRequest owns the On 2016/03/26 00:55:16, eroman wrote: > Before jumping into the "why" it's needed, I suggest mentioning explicitly the > mechanism it supports -- that the Writer can outlive the ChunkedUploadDataStream > / URLRequest that it is associated with. > > This is implied from the following comment, but I think t would be useful to see > it stated explicitly either here, or in the description of CreateWriter(). Done. https://codereview.chromium.org/1732493002/diff/280001/net/url_request/url_fe... File net/url_request/url_fetcher_impl_unittest.cc (right): https://codereview.chromium.org/1732493002/diff/280001/net/url_request/url_fe... net/url_request/url_fetcher_impl_unittest.cc:831: base::PlatformThread::Sleep(TestTimeouts::tiny_timeout()); On 2016/03/26 00:55:16, eroman wrote: > Hmm this is subtle. > > Is there a way to run this test on the IO thread and pump the IO loop as needed > to reach this condition? This test doesn't really need to be cross-thread, though there is same-thread magic in the case of cancellation by the consumer, which does make me a bit nervous about the change. There's currently no way to know when the URLFetcher failed but has not yet been notified of the failure, and I don't want to call into the Fetcher after it's been notified of the failure. So that leaves us needing to wait for some sort of notification the request failed, but that we haven't been told yet. I don't want to subclass the URLFetcher, since that depends too much on URLFetcher implementation. We could subclass URLRequestJob, and have it notify us after after the URLFetcherCore is notified of the change, but before the URLFetcherCore notifies the URLFetcher. The problem is that URLRequestJob::NotifyStartFailure currently synchronously calls into URLRequest, which synchronously calls into URLFetcher. I've been thinking of making it asynchronous, so it could be call on URLRequestJob::Start() immediately, which would break the test. I don't see how we can do this in a way that doesn't have similar fragility issues, unfortunately. Going to land as-is, but open to other ideas, happy to go back and revise the test in a followup CL. https://codereview.chromium.org/1732493002/diff/280001/net/url_request/url_fe... net/url_request/url_fetcher_impl_unittest.cc:1146: EXPECT_GE(Time::Now() - start_time, base::TimeDelta::FromSeconds(1)); On 2016/03/26 00:55:16, eroman wrote: > Is this timing exact, or can this fail? > Would it be better to leave this off? Removed. This was copied from the above test...I don't think it's needed on this one, though. I'd really like to get rid of this throttling stuff completely, but we have one or two consumers that use it, now that we removed the main consumer. :( https://codereview.chromium.org/1732493002/diff/280001/net/url_request/url_re... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/1732493002/diff/280001/net/url_request/url_re... net/url_request/url_request_unittest.cc:5741: // Add a standard set of data to an upload for chunked upload integration On 2016/03/26 00:55:16, eroman wrote: > nit: Adds Done. https://codereview.chromium.org/1732493002/diff/280001/net/url_request/url_re... net/url_request/url_request_unittest.cc:5752: // Check that the upload data added in AddChunksToUpload() was echoed back from On 2016/03/26 00:55:16, eroman wrote: > nit: Checks Done.
The CQ bit was checked by mmenke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org Link to the patchset: https://codereview.chromium.org/1732493002/#ps300001 (title: "Response to comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1732493002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1732493002/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by mmenke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org Link to the patchset: https://codereview.chromium.org/1732493002/#ps320001 (title: "Remove unused variable")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1732493002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1732493002/320001
Message was sent while issue was closed.
Description was changed from ========== Prevent URLFetcher::AppendChunkedData from dereferencing NULL pointers. Previously, this would happen if it was called after a request failed, while a request was being throttled, or if a request was redirected and lost its body without having sent the entire upload body (Either redirected by stuff outside net with never uploading the body, or redirected by stuff outside net after a network error). BUG=568091 ========== to ========== Prevent URLFetcher::AppendChunkedData from dereferencing NULL pointers. Previously, this would happen if it was called after a request failed, while a request was being throttled, or if a request was redirected and lost its body without having sent the entire upload body (Either redirected by stuff outside net with never uploading the body, or redirected by stuff outside net after a network error). BUG=568091 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Prevent URLFetcher::AppendChunkedData from dereferencing NULL pointers. Previously, this would happen if it was called after a request failed, while a request was being throttled, or if a request was redirected and lost its body without having sent the entire upload body (Either redirected by stuff outside net with never uploading the body, or redirected by stuff outside net after a network error). BUG=568091 ========== to ========== Prevent URLFetcher::AppendChunkedData from dereferencing NULL pointers. Previously, this would happen if it was called after a request failed, while a request was being throttled, or if a request was redirected and lost its body without having sent the entire upload body (Either redirected by stuff outside net with never uploading the body, or redirected by stuff outside net after a network error). BUG=568091 Committed: https://crrev.com/56b0cbb91b8ae09e2304eb8e6503ebd729332c35 Cr-Commit-Position: refs/heads/master@{#383571} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/56b0cbb91b8ae09e2304eb8e6503ebd729332c35 Cr-Commit-Position: refs/heads/master@{#383571} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
