indexeddb: Removed use of dangling ptr in writeBlobToFileOnIOThread.

indexeddb: Removed use of dangling ptr in writeBlobToFileOnIOThread.

LocalWriteClosure only had a raw pointer to a ChainedBlobWriter which
was held by a scoped_refptr in IndexedDBBackingStore::Transaction. If
the transaction was deleted before writeBlobToFileOnIOThread was called
then a crash would ensue. Converted raw ptr to scoped_refptr prevents this.

BUG=395650
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285428

[cmumford, 2014-07-23 17:27:13]

PTAL. I'm not sure this is the right way to do this, but id does fix the bug. I
wanted to use ReleaseSoon(), but that required a SequencedTaskRunner. Maybe
there's a better way?

[jsbell, 2014-07-23 17:41:15]

lgtm...

The IDB TaskRunner is actually SequencedTaskRunner, it's just not passed around
as such. We could fix that, possibly scoped down so this patch (which we'll want
to merge) is as small as possible.

https://codereview.chromium.org/417573004/diff/1/content/browser/indexed_db/i...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/417573004/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_backing_store.cc:2288: // Don't actually
release the writer (the closure will do that) Merely
Nit: Needs a period at the end of the sentence.

[cmumford, 2014-07-23 17:46:10]

https://codereview.chromium.org/417573004/diff/1/content/browser/indexed_db/i...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/417573004/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_backing_store.cc:2288: // Don't actually
release the writer (the closure will do that) Merely
On 2014/07/23 17:41:15, jsbell wrote:
> Nit: Needs a period at the end of the sentence.

Done.

[michaeln1, 2014-07-23 20:15:15]

https://codereview.chromium.org/417573004/diff/20001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/417573004/diff/20001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.cc:711: base::TaskRunner*
task_runner)
this really is a SequencedTaskRunner so ReleaseSoon should be available

https://codereview.chromium.org/417573004/diff/20001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.cc:2256: public
base::RefCountedThreadSafe<LocalWriteClosure> {
yikes! good catch

https://codereview.chromium.org/417573004/diff/20001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.cc:2318:
task_runner_->PostTask(
probably should use ReleaseSoon here

as coded if the task runs prior to return from PostTask, i think the final
release of chained_blob_writer_ will happen on the current thread

https://codereview.chromium.org/417573004/diff/20001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.cc:2323: friend class
base::RefCountedThreadSafe<LocalWriteClosure>;
style nit: i think we usually put friend decls prior to ctor/dtor/methods?

https://codereview.chromium.org/417573004/diff/20001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.cc:2325: void
callBlobCallbackOnIDBTaskRunner(bool succeeded) {
this intermediary may not be needed, at the callsite PostTask() directly to
ReportWriteCompletion() instead?

https://codereview.chromium.org/417573004/diff/20001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_backing_store.h (right):

https://codereview.chromium.org/417573004/diff/20001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.h:476: friend class
base::RefCountedThreadSafe<ChainedBlobWriter>;
style nit: ditto

[jsbell, 2014-07-23 20:17:47]

https://codereview.chromium.org/417573004/diff/20001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/417573004/diff/20001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.cc:2323: friend class
base::RefCountedThreadSafe<LocalWriteClosure>;
On 2014/07/23 20:15:15, michaeln1 wrote:
> style nit: i think we usually put friend decls prior to ctor/dtor/methods?

I thought so to... but oddly enough it's not in the style guide, and content/
seems inconsistent about it. :(

Since we both think they should be first (just after private:) then +1 to moving
them up.

[cmumford, 2014-07-23 23:26:58]

+jochen@chromium.org for content/public/browser/indexed_db_context.h review.

As Michael suggested I switched to ReleaseSoon which is a cleaner way to delete
the object on the correct thread. This required the conversion of a bunch of
TaskRunner vars/params to SequencedTaskRunner (which is what IDB's runner is
anyhow).

https://codereview.chromium.org/417573004/diff/20001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/417573004/diff/20001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.cc:711: base::TaskRunner*
task_runner)
On 2014/07/23 20:15:15, michaeln1 wrote:
> this really is a SequencedTaskRunner so ReleaseSoon should be available

Done.

https://codereview.chromium.org/417573004/diff/20001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.cc:2323: friend class
base::RefCountedThreadSafe<LocalWriteClosure>;
On 2014/07/23 20:17:47, jsbell wrote:
> On 2014/07/23 20:15:15, michaeln1 wrote:
> > style nit: i think we usually put friend decls prior to ctor/dtor/methods?
> 
> I thought so to... but oddly enough it's not in the style guide, and content/
> seems inconsistent about it. :(
> 
> Since we both think they should be first (just after private:) then +1 to
moving
> them up.

Done.

https://codereview.chromium.org/417573004/diff/20001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.cc:2325: void
callBlobCallbackOnIDBTaskRunner(bool succeeded) {
On 2014/07/23 20:15:15, michaeln1 wrote:
> this intermediary may not be needed, at the callsite PostTask() directly to
> ReportWriteCompletion() instead?

You're correct - it's not necessary. It's unrelated, but a small change so I'll
do it in this CL.

[michaeln, 2014-07-24 00:20:29]

lgtm but lets see what jsbell thinks about fixing the rawptrs to the TaskRunner
too [i'd say do that too)

https://codereview.chromium.org/417573004/diff/60001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/417573004/diff/60001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.cc:2280:
base::Bind(&IndexedDBBackingStore::Transaction::ChainedBlobWriter::
aside: wow, didn't realize how deeply nested things are

https://codereview.chromium.org/417573004/diff/60001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.cc:2323:
base::SequencedTaskRunner* task_runner_;
somewhat related, this probably s/b a scope_refptr<T> too

https://codereview.chromium.org/417573004/diff/60001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_backing_store.h (right):

https://codereview.chromium.org/417573004/diff/60001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.h:18: #include
"base/sequenced_task_runner.h"
would a fwrd decl work in here?

https://codereview.chromium.org/417573004/diff/60001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.h:569:
base::SequencedTaskRunner* task_runner_;
huh... i'm surprised this is not a scoped_refptr<T>?

https://codereview.chromium.org/417573004/diff/60001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_context_impl.h (right):

https://codereview.chromium.org/417573004/diff/60001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_context_impl.h:149:
base::SequencedTaskRunner* task_runner_;
rawptr alert!!!

these rawptrs datamembers_ are pretty clearly not quite right and should be
scoped_refptr<base::SequencedTaskRunner>'s

[jochen (gone - plz use gerrit), 2014-07-24 09:00:35]

content/public lgtm

[cmumford, 2014-07-24 18:18:27]

jsbell: because I scoped the TaskRunner pointers you may want to take a second
look.

https://codereview.chromium.org/417573004/diff/60001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/417573004/diff/60001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.cc:2323:
base::SequencedTaskRunner* task_runner_;
On 2014/07/24 00:20:23, michaeln wrote:
> somewhat related, this probably s/b a scope_refptr<T> too

Yeah, I counted 395 "task_runner_" members in Chromium classes - only 19 of them
are raw pointers.

https://codereview.chromium.org/417573004/diff/60001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_backing_store.h (right):

https://codereview.chromium.org/417573004/diff/60001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.h:18: #include
"base/sequenced_task_runner.h"
On 2014/07/24 00:20:23, michaeln wrote:
> would a fwrd decl work in here?

If I change task_runner() to return a SequencedTaskRunner then we can forward
declare type.

https://codereview.chromium.org/417573004/diff/60001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.h:569:
base::SequencedTaskRunner* task_runner_;
On 2014/07/24 00:20:24, michaeln wrote:
> huh... i'm surprised this is not a scoped_refptr<T>?

Not in indexed_db_context_impl.h either.

https://codereview.chromium.org/417573004/diff/60001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_context_impl.h (right):

https://codereview.chromium.org/417573004/diff/60001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_context_impl.h:149:
base::SequencedTaskRunner* task_runner_;
On 2014/07/24 00:20:24, michaeln wrote:
> rawptr alert!!!
> 
> these rawptrs datamembers_ are pretty clearly not quite right and should be
> scoped_refptr<base::SequencedTaskRunner>'s

Done.

[jsbell, 2014-07-24 18:44:09]

lgtm...

So far as I can tell, the task runner lifetime was previously safe as the task
runner is a message loop proxy owned by the message loop owned by the IDB thread
and everything should be occurring on the IDB thread.

https://codereview.chromium.org/417573004/diff/80001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/417573004/diff/80001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.cc:2315: raw_tmp->AddRef();
Sad that there isn't a LeakRef() like there is in Blink.

https://codereview.chromium.org/417573004/diff/80001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.cc:2323:
base::SequencedTaskRunner* task_runner_;
Should this be a scoped_refptr too?

[cmumford, 2014-07-24 18:57:08]

https://codereview.chromium.org/417573004/diff/80001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/417573004/diff/80001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.cc:2323:
base::SequencedTaskRunner* task_runner_;
On 2014/07/24 18:44:09, jsbell wrote:
> Should this be a scoped_refptr too?

Done.

[cmumford, 2014-07-24 18:57:12]

The CQ bit was checked by cmumford@chromium.org

[commit-bot: I haz the power, 2014-07-24 18:58:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/cmumford@chromium.org/417573004/100001

[commit-bot: I haz the power, 2014-07-24 23:02:06]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)

[commit-bot: I haz the power, 2014-07-25 00:17:19]

Change committed as 285428