Prevent URLFetcher::AppendChunkedData from dereferencing NULL pointers.

net/base/chunked_upload_data_stream.h

Index: net/base/chunked_upload_data_stream.h
diff --git a/net/base/chunked_upload_data_stream.h b/net/base/chunked_upload_data_stream.h
index 7b5e2dfecb618393d8a8bc50f05f4739cde5ea08..d93ae5f72c823268e8389195d8505afd38bf0fe9 100644
--- a/net/base/chunked_upload_data_stream.h
+++ b/net/base/chunked_upload_data_stream.h
@@ -12,6 +12,8 @@
 
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "net/base/completion_callback.h"
 #include "net/base/net_export.h"
 #include "net/base/upload_data_stream.h"
@@ -25,14 +27,47 @@ class IOBuffer;
 // seekable data, due to this buffering behavior.
 class NET_EXPORT ChunkedUploadDataStream : public UploadDataStream {
  public:
+  // Utility class that allows writing data to a particular
+  // ChunkedUploadDataStream. It's needed because URLRequest owns the
+  // ChunkedUploadDataStream and manages its lifetime (And can delete it without
+  // warning, if failures are intercepted and then redirected), but higher level
+  // code is responsible for writing to the ChunkedUploadDataStream.
+  //
+  // The writer may only be used on the ChunkedUploadDataStream's thread.
+  class NET_EXPORT Writer {
+   public:
+    ~Writer();
+
+    // Adds data to the stream. |is_done| should be true if this is the last
+    // data to be appended. |data_len| must not be 0 unless |is_done| is true.
+    // Once called with |is_done| being true, must never be called again.

[eroman, 2016/03/22 19:05:37]

This description of is_done is different than that for the underlying
ChunkedUploadDataStream. Any reason for that?

[mmenke, 2016/03/22 20:01:50]

It's the same, word-for-word, as the comment before
ChunkedUploadDataStream::AppendData.  Or am I missing something?

+    // Returns true if write succeeded, false if it failed (Generally because

[eroman, 2016/03/22 19:05:37]

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.

[mmenke, 2016/03/22 20:01:51]

Done.

+    // the underlying ChunkedUploadDataStream was destroyed).
+    bool AppendData(const char* data, int data_len, bool is_done);

[eroman, 2016/03/22 19:05:37]

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.

[mmenke, 2016/03/22 20:01:50]

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.

+
+   private:
+    friend class ChunkedUploadDataStream;
+
+    explicit Writer(base::WeakPtr<ChunkedUploadDataStream> upload_data_stream);
+
+    const base::WeakPtr<ChunkedUploadDataStream> upload_data_stream_;
+
+    DISALLOW_COPY_AND_ASSIGN(Writer);
+  };
+
   explicit ChunkedUploadDataStream(int64_t identifier);
 
   ~ChunkedUploadDataStream() override;
 
+  // Creates a Writer for appending data to |this|.
+  scoped_ptr<Writer> CreateWriter();

[eroman, 2016/03/22 19:05:37]

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?

[mmenke, 2016/03/22 20:01:51]

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).

+
   // Adds data to the stream. |is_done| should be true if this is the last
   // data to be appended. |data_len| must not be 0 unless |is_done| is true.
   // Once called with |is_done| being true, must never be called again.
   // TODO(mmenke):  Consider using IOBuffers instead, to reduce data copies.
+  // TODO(mmenke):  Consider making private, and having all consumers use
+  //     Writers.
   void AppendData(const char* data, int data_len, bool is_done);
 
  private:
@@ -57,6 +92,8 @@ class NET_EXPORT ChunkedUploadDataStream : public UploadDataStream {
   scoped_refptr<IOBuffer> read_buffer_;
   int read_buffer_len_;
 
+  base::WeakPtrFactory<ChunkedUploadDataStream> weak_factory_;

[eroman, 2016/03/22 19:05:37]

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 :)

[mmenke, 2016/03/22 20:01:50]

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.

[mmenke, 2016/03/22 20:19:19]

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.

+
   DISALLOW_COPY_AND_ASSIGN(ChunkedUploadDataStream);
 };