Fix Simple Cache race on multiple dooms in flight.

Fix Simple Cache race on multiple dooms on flight.

Previously we didn't register dooms in the backend until after
asynchronous operations on the entry; this meant that multiple
registrations were possible if different entries were being doomed, and null dereferences abounded after doom operations. Now fixed.

This reenables DiskCacheBackendTest.SimpleCacheDoomAll.

R=pasko,clamy,ttuttle
BUG=290866, 237450, 289542
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223376

[gavinp, 2013-09-16 12:38:12]

pasko: PTAL.

clamy, ttuttle: FYI.

[gavinp, 2013-09-16 12:40:12]

https://codereview.chromium.org/23591048/diff/1/net/disk_cache/simple/simple_...
File net/disk_cache/simple/simple_entry_impl.cc (right):

https://codereview.chromium.org/23591048/diff/1/net/disk_cache/simple/simple_...
net/disk_cache/simple/simple_entry_impl.cc:568:
backend_->OnDoomStart(entry_hash_);
This is a bug. We should only do this on actual doom operations.

[gavinp, 2013-09-16 12:50:51]

I cleaned up the description, and added a new unit test specifically for this.
Egor, you asked if there's more patterns to test. Well, here's one.

[pasko, 2013-09-16 14:01:42]

Gavin, great detective work! Thanks!

LGTM

https://codereview.chromium.org/23591048/diff/20001/net/disk_cache/backend_un...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/23591048/diff/20001/net/disk_cache/backend_un...
net/disk_cache/backend_unittest.cc:3215: //
https://codereview.chromium.org/23823002/ lands.
please remove the comment

https://codereview.chromium.org/23591048/diff/20001/net/disk_cache/entry_unit...
File net/disk_cache/entry_unittest.cc (right):

https://codereview.chromium.org/23591048/diff/20001/net/disk_cache/entry_unit...
net/disk_cache/entry_unittest.cc:3090: // Create, Doom, Create, Doom.
nit: I would have written this with more clarity:
// Test sequence for operations on the same key:
// Create(entry1), Doom(entry1), Create(entry2), Doom(entry2).

not very important, the test is short and clear anyway

https://codereview.chromium.org/23591048/diff/20001/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_entry_impl.cc (right):

https://codereview.chromium.org/23591048/diff/20001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.cc:271: if (backend_)
nit: we are inconsistent at how we check the weak pointer of the backend in this
file. I'd prefer to have backend_.get() instead sinse it better suggests that
the backend_.get() is a pointer and can go away later.

[gavinp, 2013-09-16 14:14:41]

Thanks. I'm going to CQ this fairly soon, on account of it being a big crasher
on some platforms.

https://codereview.chromium.org/23591048/diff/20001/net/disk_cache/backend_un...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/23591048/diff/20001/net/disk_cache/backend_un...
net/disk_cache/backend_unittest.cc:3215: //
https://codereview.chromium.org/23823002/ lands.
On 2013/09/16 14:01:42, pasko wrote:
> please remove the comment

Done.

https://codereview.chromium.org/23591048/diff/20001/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_entry_impl.cc (right):

https://codereview.chromium.org/23591048/diff/20001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_impl.cc:271: if (backend_)
On 2013/09/16 14:01:42, pasko wrote:
> nit: we are inconsistent at how we check the weak pointer of the backend in
this
> file. I'd prefer to have backend_.get() instead sinse it better suggests that
> the backend_.get() is a pointer and can go away later.

Done.

[commit-bot: I haz the power, 2013-09-16 14:14:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/23591048/29001

[pasko, 2013-09-16 14:28:54]

On 2013/09/16 14:14:41, gavinp wrote:
> Thanks. I'm going to CQ this fairly soon, on account of it being a big crasher
> on some platforms.

I suppose you are explaining priority because before you wanted others to look
at this?

[gavinp, 2013-09-16 15:20:32]

On 2013/09/16 14:28:54, pasko wrote:
> On 2013/09/16 14:14:41, gavinp wrote:
> > Thanks. I'm going to CQ this fairly soon, on account of it being a big
crasher
> > on some platforms.
> 
> I suppose you are explaining priority because before you wanted others to look
> at this?

Ya. Also, I'm just anxious.

[Deprecated (see juliatuttle), 2013-09-16 16:41:42]

(lgtm)

[pasko, 2013-09-16 16:46:00]

On 2013/09/16 16:41:42, ttuttle wrote:
> (lgtm)

is this a hidden L G T M?

[Deprecated (see juliatuttle), 2013-09-16 17:40:49]

On 2013/09/16 16:46:00, pasko wrote:
> On 2013/09/16 16:41:42, ttuttle wrote:
> > (lgtm)
> 
> is this a hidden L G T M?

Nah, it's a "lgtm but it's already on the way to the CQ anyway".

[commit-bot: I haz the power, 2013-09-16 18:18:24]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[Deprecated (see juliatuttle), 2013-09-16 19:06:07]

On 2013/09/16 18:18:24, I haz the power (commit-bot) wrote:
> Commit queue rejected this change because the description was changed
> between the time the change entered the commit queue and the time it
> was ready to commit. You can safely check the commit box again.

I rechecked it; hope that's okay.

[commit-bot: I haz the power, 2013-09-16 19:06:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/23591048/29001

[gavinp, 2013-09-16 19:10:08]

On 2013/09/16 19:06:07, ttuttle wrote:
> On 2013/09/16 18:18:24, I haz the power (commit-bot) wrote:
> > Commit queue rejected this change because the description was changed
> > between the time the change entered the commit queue and the time it
> > was ready to commit. You can safely check the commit box again.
> 
> I rechecked it; hope that's okay.

That is OK. I hope this reply doesn't bounce the issue out of the CL.

[commit-bot: I haz the power, 2013-09-16 19:12:50]

Change committed as 223376