Fix race causing crash in net::FileNetLogObserver shutdown

Fix race causing crash in net::FileNetLogObserver shutdown

The class must stop listening for NetLog events before it attempts to
shut itself down.  Without this fix these two things happen simultaneously:
1. FileNetLogObserver::StopObserving() posts FlushAndStop() to file thread.
2. FileNetLogObserver::OnAddEntry() posts Flush() to file thread.
The file thread then runs FlushAndStop() which clears event_files_, then
the file thread runs Flush() which indexes into event_files_ and crashes.
Also, re-enable test that caught this crash.

BUG=729257
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2932613002
Cr-Commit-Position: refs/heads/master@{#478151}
Committed: https://chromium.googlesource.com/chromium/src/+/316c57df2b949823a1fbd31d64150edb3d3af1e0

[pauljensen, 2017-06-08 00:33:04]

Description was changed from

==========
Fix race causing crash in net::FileNetLogObserver shutdown

The class must stop listening for NetLog events before it attempts to
shut itself down.  Without this fix these two things happen simultaneously:
1. FileNetLogObserver::StopObserving() posts FlushAndStop() to file thread.
2. FileNetLogObserver::OnAddEntry() posts Flush() to file thread.
The file thread then runs FlushAndStop() which clears event_files_, then
the file thread runs Flush() which indexes into event_files_ and crashes.
Also, re-enable test that caught this crash.

BUG=729257
==========

to

==========
Fix race causing crash in net::FileNetLogObserver shutdown

The class must stop listening for NetLog events before it attempts to
shut itself down.  Without this fix these two things happen simultaneously:
1. FileNetLogObserver::StopObserving() posts FlushAndStop() to file thread.
2. FileNetLogObserver::OnAddEntry() posts Flush() to file thread.
The file thread then runs FlushAndStop() which clears event_files_, then
the file thread runs Flush() which indexes into event_files_ and crashes.
Also, re-enable test that caught this crash.

BUG=729257
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[pauljensen, 2017-06-08 00:39:20]

pauljensen@chromium.org changed reviewers:
+ eroman@chromium.org, wangyix@chromium.org

[pauljensen, 2017-06-08 00:39:20]

Eric & Yixin, PTAL, thank you!

I considered Eric's suggestion to make Flush() a no-op when event_files_ is
empty, but I'm very hesitant to write code that silently fails in undetectable
ways, so instead I added a simple CHECK so if this ever happens again it will be
easy to track down.  I don't feel too strongly either way so feel free to let me
know if you want it changed.

[pauljensen, 2017-06-08 00:39:39]

The CQ bit was checked by pauljensen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-08 00:39:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2017-06-08 00:45:52]

mmenke@chromium.org changed reviewers:
+ mmenke@chromium.org

[mmenke, 2017-06-08 00:45:53]

Random driveby suggestion.

https://codereview.chromium.org/2932613002/diff/1/net/log/file_net_log_observ...
File net/log/file_net_log_observer.cc (right):

https://codereview.chromium.org/2932613002/diff/1/net/log/file_net_log_observ...
net/log/file_net_log_observer.cc:267: // StopObserving was not called.
Maybe just call StopObserving with an empty callback instead?

[pauljensen, 2017-06-08 00:48:57]

https://codereview.chromium.org/2932613002/diff/1/net/log/file_net_log_observ...
File net/log/file_net_log_observer.cc (right):

https://codereview.chromium.org/2932613002/diff/1/net/log/file_net_log_observ...
net/log/file_net_log_observer.cc:267: // StopObserving was not called.
On 2017/06/08 00:45:53, mmenke wrote:
> Maybe just call StopObserving with an empty callback instead?

This code posts DeleteAllFiles() while the other posts FlushThenStop(), so I'm
not sure they're interchangeable.

[mmenke, 2017-06-08 00:49:40]

On 2017/06/08 00:48:57, pauljensen wrote:
>
https://codereview.chromium.org/2932613002/diff/1/net/log/file_net_log_observ...
> File net/log/file_net_log_observer.cc (right):
> 
>
https://codereview.chromium.org/2932613002/diff/1/net/log/file_net_log_observ...
> net/log/file_net_log_observer.cc:267: // StopObserving was not called.
> On 2017/06/08 00:45:53, mmenke wrote:
> > Maybe just call StopObserving with an empty callback instead?
> 
> This code posts DeleteAllFiles() while the other posts FlushThenStop(), so I'm
> not sure they're interchangeable.

Oops, missed that

[eroman, 2017-06-08 00:54:40]

lgtm, thanks!

Can you add a test to net/log/file_net_log_observer_unittest.cc ?

I realize there was some coverage on the cronet test, however would be nice to
have a broader one in our .cc file.

[commit-bot: I haz the power, 2017-06-08 02:43:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-08 02:43:43]

Dry run: This issue passed the CQ dry run.

[pauljensen, 2017-06-08 11:48:58]

On 2017/06/08 00:54:40, eroman wrote:
> Can you add a test to net/log/file_net_log_observer_unittest.cc ?

Done, I added two tests and verified that both tests crash without the fix, and
not with the fix.

[mmenke, 2017-06-08 17:46:43]

Description was changed from

==========
Fix race causing crash in net::FileNetLogObserver shutdown

The class must stop listening for NetLog events before it attempts to
shut itself down.  Without this fix these two things happen simultaneously:
1. FileNetLogObserver::StopObserving() posts FlushAndStop() to file thread.
2. FileNetLogObserver::OnAddEntry() posts Flush() to file thread.
The file thread then runs FlushAndStop() which clears event_files_, then
the file thread runs Flush() which indexes into event_files_ and crashes.
Also, re-enable test that caught this crash.

BUG=729257
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Fix race causing crash in net::FileNetLogObserver shutdown

The class must stop listening for NetLog events before it attempts to
shut itself down.  Without this fix these two things happen simultaneously:
1. FileNetLogObserver::StopObserving() posts FlushAndStop() to file thread.
2. FileNetLogObserver::OnAddEntry() posts Flush() to file thread.
The file thread then runs FlushAndStop() which clears event_files_, then
the file thread runs Flush() which indexes into event_files_ and crashes.
Also, re-enable test that caught this crash.

BUG=729257
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[mmenke, 2017-06-08 17:46:43]

mmenke@chromium.org changed reviewers:
- mmenke@chromium.org

[eroman, 2017-06-08 21:50:53]

lgtm, thanks for the tests!

[pauljensen, 2017-06-08 22:33:51]

The CQ bit was checked by pauljensen@chromium.org

[commit-bot: I haz the power, 2017-06-08 22:34:20]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-09 01:12:58]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1496961231013990,
"parent_rev": "4c5f755739fcbe4021d3a2938571d066a1b2d5cf", "commit_rev":
"316c57df2b949823a1fbd31d64150edb3d3af1e0"}

[commit-bot: I haz the power, 2017-06-09 01:13:07]

Description was changed from

==========
Fix race causing crash in net::FileNetLogObserver shutdown

The class must stop listening for NetLog events before it attempts to
shut itself down.  Without this fix these two things happen simultaneously:
1. FileNetLogObserver::StopObserving() posts FlushAndStop() to file thread.
2. FileNetLogObserver::OnAddEntry() posts Flush() to file thread.
The file thread then runs FlushAndStop() which clears event_files_, then
the file thread runs Flush() which indexes into event_files_ and crashes.
Also, re-enable test that caught this crash.

BUG=729257
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Fix race causing crash in net::FileNetLogObserver shutdown

The class must stop listening for NetLog events before it attempts to
shut itself down.  Without this fix these two things happen simultaneously:
1. FileNetLogObserver::StopObserving() posts FlushAndStop() to file thread.
2. FileNetLogObserver::OnAddEntry() posts Flush() to file thread.
The file thread then runs FlushAndStop() which clears event_files_, then
the file thread runs Flush() which indexes into event_files_ and crashes.
Also, re-enable test that caught this crash.

BUG=729257
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2932613002
Cr-Commit-Position: refs/heads/master@{#478151}
Committed:
https://chromium.googlesource.com/chromium/src/+/316c57df2b949823a1fbd31d6415...
==========

[commit-bot: I haz the power, 2017-06-09 01:13:09]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/316c57df2b949823a1fbd31d6415...