Fix heapcheck errors

Fix the heapcheck errors caused by StreamContext by using the same delete code as ChromeBlobStorageContext

BUG=None

[darin (slow to review), 2013-03-11 20:26:41]

https://codereview.chromium.org/12615004/diff/9001/content/browser/streams/st...
File content/browser/streams/stream_context.cc (right):

https://codereview.chromium.org/12615004/diff/9001/content/browser/streams/st...
content/browser/streams/stream_context.cc:44: void
StreamContext::DeleteOnCorrectThread() const {
I'm really not a big fan of test-only code like this.  It runs the risk of being
used in production, bloats production code for the sake of testing, and just
generally makes the code more complex.  Did you consider adding an IO thread to
the tests?

[Zachary Kuznia, 2013-03-11 21:46:26]

On 2013/03/11 20:26:41, darin wrote:
>
https://codereview.chromium.org/12615004/diff/9001/content/browser/streams/st...
> File content/browser/streams/stream_context.cc (right):
> 
>
https://codereview.chromium.org/12615004/diff/9001/content/browser/streams/st...
> content/browser/streams/stream_context.cc:44: void
> StreamContext::DeleteOnCorrectThread() const {
> I'm really not a big fan of test-only code like this.  It runs the risk of
being
> used in production, bloats production code for the sake of testing, and just
> generally makes the code more complex.  Did you consider adding an IO thread
to
> the tests?

I did, but it looks like it causes leaks in 50+ tests spread across unit_tests,
ash_unittests, and content_unittests.  How about I add a TODO, and fix this and
Blob Context in a follow-up CL?

[darin (slow to review), 2013-03-11 22:15:29]

If you are going to do it as a follow-up CL, then please add a TODO(zork)
to that section.
I think it is still worth looking to see if there isn't a simple solution
now.

-Darin



On Mon, Mar 11, 2013 at 2:46 PM, <zork@chromium.org> wrote:

> Reviewers: darin,
>
> Message:
>
> On 2013/03/11 20:26:41, darin wrote:
>
> https://codereview.chromium.**org/12615004/diff/9001/**
>
content/browser/streams/**stream_context.cc<https://codereview.chromium.org/12615004/diff/9001/content/browser/streams/stream_context.cc>
>
>> File content/browser/streams/**stream_context.cc (right):
>>
>
>
> https://codereview.chromium.**org/12615004/diff/9001/**
>
content/browser/streams/**stream_context.cc#newcode44<https://codereview.chromium.org/12615004/diff/9001/content/browser/streams/stream_context.cc#newcode44>
>
>> content/browser/streams/**stream_context.cc:44: void
>> StreamContext::**DeleteOnCorrectThread() const {
>> I'm really not a big fan of test-only code like this.  It runs the risk of
>>
> being
>
>> used in production, bloats production code for the sake of testing, and
>> just
>> generally makes the code more complex.  Did you consider adding an IO
>> thread
>>
> to
>
>> the tests?
>>
>
> I did, but it looks like it causes leaks in 50+ tests spread across
> unit_tests,
> ash_unittests, and content_unittests.  How about I add a TODO, and fix
> this and
> Blob Context in a follow-up CL?
>
> Description:
> Fix the heapcheck errors caused by StreamContext by using the same delete
> code
> as ChromeBlobStorageContext
>
> BUG=None
>
> Please review this at
https://codereview.chromium.**org/12615004/<https://codereview.chromium.org/1...
>
> SVN Base:
http://git.chromium.org/**chromium/src.git@master<http://git.chromium.org/chr...
>
> Affected files:
>   M content/browser/streams/**stream.h
>   M content/browser/streams/**stream.cc
>   M content/browser/streams/**stream_context.h
>   M content/browser/streams/**stream_context.cc
>
>
> Index: content/browser/streams/**stream.cc
> diff --git a/content/browser/streams/**stream.cc
> b/content/browser/streams/**stream.cc
> index 0efec1349e4cca6b5a75ae68ecf07f**a361ca67fb..**
> e72a8872dc541270aa99a6e637cab2**dbead7dbaf 100644
> --- a/content/browser/streams/**stream.cc
> +++ b/content/browser/streams/**stream.cc
> @@ -29,7 +29,8 @@ Stream::Stream(StreamRegistry* registry,
>        data_length_(0),
>        registry_(registry),
>        read_observer_(NULL),
> -      write_observer_(write_**observer) {
> +      write_observer_(write_**observer),
> +      weak_ptr_factory_(ALLOW_THIS_**IN_INITIALIZER_LIST(this)) {
>    CreateByteStream(base::**MessageLoopProxy::current(),
>                     base::MessageLoopProxy::**current(),
>                     kDeferSizeThreshold,
> @@ -37,8 +38,10 @@ Stream::Stream(StreamRegistry* registry,
>                     &reader_);
>
>    // Setup callback for writing.
> -  writer_->RegisterCallback(**base::Bind(&Stream::**OnSpaceAvailable,
> this));
> -  reader_->RegisterCallback(**base::Bind(&Stream::**OnDataAvailable,
> this));
> +  writer_->RegisterCallback(**base::Bind(&Stream::**OnSpaceAvailable,
> +                                       weak_ptr_factory_.GetWeakPtr()**
> ));
> +  reader_->RegisterCallback(**base::Bind(&Stream::**OnDataAvailable,
> +                                       weak_ptr_factory_.GetWeakPtr()**
> ));
>
>    registry_->RegisterStream(**this);
>  }
> Index: content/browser/streams/**stream.h
> diff --git a/content/browser/streams/**stream.h b/content/browser/streams/
> **stream.h
> index 567b10b111495bb9e9d7973dfd88ed**1cd02c0ee8..**
> 3aa389c8d96326868fcdda340d44d8**5db55f85cc 100644
> --- a/content/browser/streams/**stream.h
> +++ b/content/browser/streams/**stream.h
> @@ -7,6 +7,7 @@
>
>  #include "base/basictypes.h"
>  #include "base/memory/ref_counted.h"
> +#include "base/memory/weak_ptr.h"
>  #include "content/browser/download/**byte_stream.h"
>  #include "content/common/content_**export.h"
>  #include "googleurl/src/gurl.h"
> @@ -91,6 +92,7 @@ class CONTENT_EXPORT Stream : public
> base::RefCountedThreadSafe<**Stream> {
>    StreamReadObserver* read_observer_;
>    StreamWriteObserver* write_observer_;
>
> +  base::WeakPtrFactory<Stream> weak_ptr_factory_;
>    DISALLOW_COPY_AND_ASSIGN(**Stream);
>  };
>
> Index: content/browser/streams/**stream_context.cc
> diff --git a/content/browser/streams/**stream_context.cc
> b/content/browser/streams/**stream_context.cc
> index ca77df1722ce3692f489f9474ebd57**51bbe41e8a..**
> e68d0bfc35ed5b659b3d9e7091155c**917a3bb1aa 100644
> --- a/content/browser/streams/**stream_context.cc
> +++ b/content/browser/streams/**stream_context.cc
> @@ -41,4 +41,15 @@ void StreamContext::**InitializeOnIOThread() {
>
>  StreamContext::~StreamContext(**) {}
>
> +void StreamContext::**DeleteOnCorrectThread() const {
> +  // In many tests, there isn't a valid IO thread.  In that case, just
> delete on
> +  // the current thread.
> +  if (BrowserThread::**IsMessageLoopValid(**BrowserThread::IO) &&
> +      !BrowserThread::CurrentlyOn(**BrowserThread::IO)) {
> +    BrowserThread::DeleteSoon(**BrowserThread::IO, FROM_HERE, this);
> +    return;
> +  }
> +  delete this;
> +}
> +
>  }  // namespace content
> Index: content/browser/streams/**stream_context.h
> diff --git a/content/browser/streams/**stream_context.h
> b/content/browser/streams/**stream_context.h
> index b4c6d307fa50484ef241cb0805d0cb**8c998f9e29..**
> 7b8adcbdea8edf36888d03465e2e01**0dc1dd03e4 100644
> --- a/content/browser/streams/**stream_context.h
> +++ b/content/browser/streams/**stream_context.h
> @@ -13,6 +13,7 @@
>  namespace content {
>  class BrowserContext;
>  class StreamRegistry;
> +struct StreamContextDeleter;
>
>  // A context class that keeps track of StreamRegistry used by the chrome.
>  // There is an instance associated with each BrowserContext. There could
> be
> @@ -23,7 +24,7 @@ class StreamRegistry;
>  // the IO thread (unless specifically called out in doc comments).
>  class StreamContext
>      : public base::RefCountedThreadSafe<**StreamContext,
> -                                        BrowserThread::**DeleteOnIOThread>
> {
> +                                        StreamContextDeleter> {
>   public:
>    StreamContext();
>
> @@ -38,11 +39,21 @@ class StreamContext
>
>   private:
>    friend class base::DeleteHelper<**StreamContext>;
> -  friend struct BrowserThread::DeleteOnThread<**BrowserThread::IO>;
> +  friend class base::RefCountedThreadSafe<**StreamContext,
> +                                          StreamContextDeleter>;
> +  friend struct StreamContextDeleter;
> +
> +  void DeleteOnCorrectThread() const;
>
>    scoped_ptr<StreamRegistry> registry_;
>  };
>
> +struct StreamContextDeleter {
> +  static void Destruct(const StreamContext* context) {
> +    context->**DeleteOnCorrectThread();
> +  }
> +};
> +
>  }  // namespace content
>
>  #endif  // CONTENT_BROWSER_STREAMS_**STREAM_CONTEXT_H_
>
>
>

[Zachary Kuznia, 2013-03-11 22:24:22]

On 2013/03/11 22:15:29, darin wrote:
> If you are going to do it as a follow-up CL, then please add a TODO(zork)
> to that section.
> I think it is still worth looking to see if there isn't a simple solution
> now.
> 
> -Darin
> 

Done.