Improve documentation on Filter and fix pipeline overwrite.

BUG=418975
R=bnc@chromium.org

Review URL: https://codereview.chromium.org/687263002

Cr-Commit-Position: refs/heads/master@{#302307}

net/filter/filter.cc

@@ -2,6 +2,25 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+// The basic usage of the Filter interface is described in the comment at
+// the beginning of filter.h.  If Filter::Factory is passed a vector of
+// size greater than 1, that interface is implemented by a series of filters
+// connected in a chain.  In such a case the first filter
+// in the chain proxies calls to ReadData() so that its return values
+// apply to the entire chain.
+//
+// In a filter chain, the data flows from first filter (held by the
+// caller) down the chain.  When ReadData() is called on any filter
+// except for the last filter, it proxies the call down the chain,
+// filling in the input buffers of subsequent filters if needed (==
+// that filter's last_status() value is FILTER_NEED_MORE_DATA) and
+// available (== the current filter has data it can output).  The last
+// Filter will then output data if possible, and return
+// FILTER_NEED_MORE_DATA if not.  Because the indirection pushes
+// data along the filter chain at each level if it's available and the
+// next filter needs it, a return value of FILTER_NEED_MORE_DATA from the
+// final filter will apply to the entire chain.
+
 #include "net/filter/filter.h"
 
 #include "base/files/file_path.h"
@@ -87,6 +106,9 @@ Filter::FilterStatus Filter::ReadData(char* dest_buffer, int* dest_len) {
     return last_status_;
   if (!next_filter_.get())
     return last_status_ = ReadFilteredData(dest_buffer, dest_len);
+
+  // This filter needs more data, but it's not clear that the rest of
+  // the chain does; delegate the actual status return to the next filter.
   if (last_status_ == FILTER_NEED_MORE_DATA && !stream_data_len())
     return next_filter_->ReadData(dest_buffer, dest_len);
 
@@ -131,6 +153,7 @@ bool Filter::FlushStreamBuffer(int stream_data_len) {
 
   next_stream_data_ = stream_buffer()->data();
   stream_data_len_ = stream_data_len;
+  last_status_ = FILTER_OK;
   return true;
 }
 

net/filter/filter.h

@@ -19,11 +19,28 @@
 // To filter a data stream, the caller first gets filter's stream_buffer_
 // through its accessor and fills in stream_buffer_ with pre-filter data, next
 // calls FlushStreamBuffer to notify Filter, then calls ReadFilteredData
-// repeatedly to get all the filtered data. After all data have been fitlered
-// and read out, the caller may fill in stream_buffer_ again. This
+// repeatedly to get all the filtered data. After all data have been filtered
+// and read out, the caller may fill in stream_buffer_ again.  This
 // WriteBuffer-Flush-Read cycle is repeated until reaching the end of data
 // stream.
 //
+// A return of FILTER_OK from ReadData() means that more data is
+// available to a future ReadData() call and data may not be written
+// into stream_buffer().  A return of FILTER_NEED_MORE_DATA from ReadData()
+// indicates that no data will be forthcoming from the filter until
+// it receives more input data, and that the buffer at
+// stream_buffer() may be written to.
+//
+// The filter being complete (no more data to provide) may be indicated
+// by either returning FILTER_DONE or by returning FILTER_OK and indicating
+// zero bytes output; consumers understand both those signals.  Consumers
+// are responsible for not calling ReadData() on a filter after one of these
+// signals have been returned.  Note that some filters may never signal that
+// they are done (e.g. a pass-through filter will always
+// say FILTER_NEED_MORE_DATA), so the consumer will also need to
+// recognize the state of |no_more_input_data_available &&
+// filter->stream_data_len() == 0| as FILTER_DONE.
+//
 // The lifetime of a Filter instance is completely controlled by its caller.
 
 #ifndef NET_FILTER_FILTER_H__
@@ -207,6 +224,7 @@ class NET_EXPORT_PRIVATE Filter {
  protected:
   friend class GZipUnitTest;
   friend class SdchFilterChainingTest;
+  FRIEND_TEST_ALL_PREFIXES(FilterTest, ThreeFilterChain);
 
   Filter();
 

net/filter/filter_unittest.cc

@@ -2,12 +2,28 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "net/base/io_buffer.h"
 #include "net/filter/filter.h"
 #include "net/filter/mock_filter_context.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace net {
 
+namespace {
+
+class PassThroughFilter : public Filter {
+ public:
+  PassThroughFilter() {}
+
+  FilterStatus ReadFilteredData(char* dest_buffer, int* dest_len) override {
+    return CopyOut(dest_buffer, dest_len);
+  }
+
+  DISALLOW_COPY_AND_ASSIGN(PassThroughFilter);
+};
+
+}  // namespace
+
 class FilterTest : public testing::Test {
 };
 
@@ -364,4 +380,68 @@ TEST(FilterTest, SupportedMimeGzip) {
   EXPECT_TRUE(encoding_types.empty());
 }
 
-}  // namespace net
+// Make sure a series of three pass-through filters copies the data cleanly.
+// Regression test for http://crbug.com/418975.
+TEST(FilterTest, ThreeFilterChain) {
+  scoped_ptr<PassThroughFilter> filter1(new PassThroughFilter);
+  scoped_ptr<PassThroughFilter> filter2(new PassThroughFilter);
+  scoped_ptr<PassThroughFilter> filter3(new PassThroughFilter);
+
+  filter1->InitBuffer(32 * 1024);
+  filter2->InitBuffer(32 * 1024);
+  filter3->InitBuffer(32 * 1024);
+
+  filter2->next_filter_ = filter3.Pass();
+  filter1->next_filter_ = filter2.Pass();
+
+  // Initialize the input array with a varying byte sequence.
+  const size_t input_array_size = 64 * 1024;
+  char input_array[input_array_size];
+  size_t read_array_index = 0;
+  for (size_t i = 0; i < input_array_size; i++) {
+    input_array[i] = i % 113;
+  }
+
+  const size_t output_array_size = 4 * 1024;
+  char output_array[output_array_size];
+
+  size_t compare_array_index = 0;
+
+  do {
+    // Read data from the filter chain.
+    int amount_read = output_array_size;
+    Filter::FilterStatus status = filter1->ReadData(output_array, &amount_read);
+    EXPECT_NE(Filter::FILTER_ERROR, status);
+    EXPECT_EQ(0, memcmp(output_array, input_array + compare_array_index,
+                        amount_read));
+    compare_array_index += amount_read;
+
+    // Detect the various indications that data transfer along the chain is
+    // complete.
+    if (Filter::FILTER_DONE == status || Filter::FILTER_ERROR == status ||
+        (Filter::FILTER_OK == status && amount_read == 0) ||
+        (Filter::FILTER_NEED_MORE_DATA == status &&
+         read_array_index == input_array_size))
+      break;
+
+    if (Filter::FILTER_OK == status)
+      continue;
+
+    // Write needed data into the filter chain.
+    ASSERT_EQ(Filter::FILTER_NEED_MORE_DATA, status);
+    ASSERT_NE(0, filter1->stream_buffer_size());
+    size_t amount_to_copy = std::min(
+        static_cast<size_t>(filter1->stream_buffer_size()),
+        input_array_size - read_array_index);
+    memcpy(filter1->stream_buffer()->data(),
+           input_array + read_array_index,
+           amount_to_copy);
+    filter1->FlushStreamBuffer(amount_to_copy);
+    read_array_index += amount_to_copy;
+  } while (true);
+
+  EXPECT_EQ(read_array_index, input_array_size);
+  EXPECT_EQ(compare_array_index, input_array_size);
+}
+
+}  // Namespace net