Fix potential crashes in NetworkHandler code

Fix potential crashes in NetworkHandler code

This CL:
* Properly removes NetworkConnectionHandler observer in
  ~AutoConnectHandler.
* Checks observer lists in NetworkHandler classes on shutdown (to catch
  any future regressions).
* Destroys ProxyConfigServiceImpl owned by IOThread before
  NetworkHandler is destroyed to remove the NetworkStateHandler
  observer.

BUG=572914
For chrome/browser/extensions/api/vpn_provider/vpn_provider_apitest.cc:
TBR=rdevlin.cronin@chromium.org

Committed: https://crrev.com/bbbd48b2dc5bbde2472eafe4319f29937e3b7de6
Cr-Commit-Position: refs/heads/master@{#368118}

[stevenjb, 2016-01-05 22:26:37]

stevenjb@chromium.org changed reviewers:
+ oshima@chromium.org, sky@chromium.org

[stevenjb, 2016-01-05 22:26:38]

sky@ for chrome/ owner

https://codereview.chromium.org/1562593002/diff/1/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main.h (right):

https://codereview.chromium.org/1562593002/diff/1/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.h:101: BrowserProcessImpl* browser_process()
{ return browser_process_.get(); }
Note: this is protected.

[stevenjb, 2016-01-05 22:26:46]

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

[commit-bot: I haz the power, 2016-01-05 22:28:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1562593002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1562593002/1

[commit-bot: I haz the power, 2016-01-05 22:49:35]

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

[commit-bot: I haz the power, 2016-01-05 22:49:36]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[oshima, 2016-01-05 23:14:36]

can you look into test failures?

https://codereview.chromium.org/1562593002/diff/1/chromeos/network/network_st...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/1562593002/diff/1/chromeos/network/network_st...
chromeos/network/network_state_handler.cc:99: (int64_t)observer));
%p ? (or static_cast<>)

https://codereview.chromium.org/1562593002/diff/1/chromeos/network/network_st...
chromeos/network/network_state_handler.cc:110: (int64_t)observer));
ditto

[sky, 2016-01-05 23:17:11]

https://codereview.chromium.org/1562593002/diff/1/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main.h (right):

https://codereview.chromium.org/1562593002/diff/1/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.h:101: BrowserProcessImpl* browser_process()
{ return browser_process_.get(); }
On 2016/01/05 22:26:37, stevenjb wrote:
> Note: this is protected.

Is there a reason you can't use g_browser_process? ~5 lines up from where you
use this function the code uses g_browser_process.

https://codereview.chromium.org/1562593002/diff/1/chrome/browser/chromeos/chr...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/1562593002/diff/1/chrome/browser/chromeos/chr...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:828: // Destroy the
ProxyConfigServiceImpl owned by IOThread before
This just documents the code. Document why this order is important.

https://codereview.chromium.org/1562593002/diff/1/chrome/browser/chromeos/chr...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:830:
browser_process()->io_thread()->ShutdownPrefProxyConfigTracker();
Won't this mean you're destroying the PrefProxyConfigTracker on a different
thread than it is currently destroyed? Is it setup to handle that?

[stevenjb, 2016-01-06 00:42:49]

Patchset #2 (id:20001) has been deleted

[stevenjb, 2016-01-06 00:43:04]

PTAL

https://codereview.chromium.org/1562593002/diff/1/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main.h (right):

https://codereview.chromium.org/1562593002/diff/1/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.h:101: BrowserProcessImpl* browser_process()
{ return browser_process_.get(); }
On 2016/01/05 23:17:11, sky wrote:
> On 2016/01/05 22:26:37, stevenjb wrote:
> > Note: this is protected.
> 
> Is there a reason you can't use g_browser_process? ~5 lines up from where you
> use this function the code uses g_browser_process.

Duh. Done.

https://codereview.chromium.org/1562593002/diff/1/chrome/browser/chromeos/chr...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/1562593002/diff/1/chrome/browser/chromeos/chr...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:828: // Destroy the
ProxyConfigServiceImpl owned by IOThread before
On 2016/01/05 23:17:11, sky wrote:
> This just documents the code. Document why this order is important.

Clarified.

https://codereview.chromium.org/1562593002/diff/1/chrome/browser/chromeos/chr...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:830:
browser_process()->io_thread()->ShutdownPrefProxyConfigTracker();
On 2016/01/05 23:17:11, sky wrote:
> Won't this mean you're destroying the PrefProxyConfigTracker on a different
> thread than it is currently destroyed? Is it setup to handle that?

No, it is created in IOThread() which is (somewhat unintuitively) created on the
UI thread. I added a few DCHECK_CURRENTLY_ON(UI) calls in io_thread.cc to
document that.

https://codereview.chromium.org/1562593002/diff/1/chromeos/network/network_st...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/1562593002/diff/1/chromeos/network/network_st...
chromeos/network/network_state_handler.cc:99: (int64_t)observer));
On 2016/01/05 23:14:36, oshima wrote:
> %p ? (or static_cast<>)

Done.

https://codereview.chromium.org/1562593002/diff/1/chromeos/network/network_st...
chromeos/network/network_state_handler.cc:110: (int64_t)observer));
On 2016/01/05 23:14:36, oshima wrote:
> ditto

Done.

[stevenjb, 2016-01-06 00:43:11]

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

[commit-bot: I haz the power, 2016-01-06 00:44:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1562593002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1562593002/40001

[commit-bot: I haz the power, 2016-01-06 01:35:54]

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

[commit-bot: I haz the power, 2016-01-06 01:35:55]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[oshima, 2016-01-06 08:01:33]

there are still test failures. can you look into them?

[sky, 2016-01-06 16:30:38]

https://codereview.chromium.org/1562593002/diff/1/chrome/browser/chromeos/chr...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/1562593002/diff/1/chrome/browser/chromeos/chr...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:830:
browser_process()->io_thread()->ShutdownPrefProxyConfigTracker();
On 2016/01/06 00:43:04, stevenjb wrote:
> On 2016/01/05 23:17:11, sky wrote:
> > Won't this mean you're destroying the PrefProxyConfigTracker on a different
> > thread than it is currently destroyed? Is it setup to handle that?
> 
> No, it is created in IOThread() which is (somewhat unintuitively) created on
the
> UI thread. I added a few DCHECK_CURRENTLY_ON(UI) calls in io_thread.cc to
> document that.

I'm nervous and not familiar enough with this code to know if this is correct.
Can you get someone that knows about the threading of PrefProxyConfigTracker to
review this?

[stevenjb, 2016-01-06 16:52:58]

On 2016/01/06 16:30:38, sky wrote:
>
https://codereview.chromium.org/1562593002/diff/1/chrome/browser/chromeos/chr...
> File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):
> 
>
https://codereview.chromium.org/1562593002/diff/1/chrome/browser/chromeos/chr...
> chrome/browser/chromeos/chrome_browser_main_chromeos.cc:830:
> browser_process()->io_thread()->ShutdownPrefProxyConfigTracker();
> On 2016/01/06 00:43:04, stevenjb wrote:
> > On 2016/01/05 23:17:11, sky wrote:
> > > Won't this mean you're destroying the PrefProxyConfigTracker on a
different
> > > thread than it is currently destroyed? Is it setup to handle that?
> > 
> > No, it is created in IOThread() which is (somewhat unintuitively) created on
> the
> > UI thread. I added a few DCHECK_CURRENTLY_ON(UI) calls in io_thread.cc to
> > document that.
> 
> I'm nervous and not familiar enough with this code to know if this is correct.
> Can you get someone that knows about the threading of PrefProxyConfigTracker
to
> review this?

Other than me? :) (I'm the only owner of components/proxy_config still on the
project). Let me see if I can track down someone who might be familiar with the
code in IOThread, which is really where any theading issues may occur.

[stevenjb, 2016-01-06 18:31:34]

I spent some time looking more closely at IOThread and the proxy related classes
it controls. It's... complicated, and frankly deserves some significant cleanup.

Rather than attempt that at this point, I have taken a different approach, using
a pre-existing (but unused) IsShuttingDown observer method for
NetworkStateHandlerObserver. (It was incorrectly implemented but easy to fix).

PTAL

(sky@ - your owners review is no longer strictly required, but feedback is
always welcome if you like)

[stevenjb, 2016-01-06 18:31:47]

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

[commit-bot: I haz the power, 2016-01-06 18:32:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1562593002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1562593002/80001

[commit-bot: I haz the power, 2016-01-06 19:35:04]

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

[commit-bot: I haz the power, 2016-01-06 19:35:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sky, 2016-01-06 21:12:17]

On Wed, Jan 6, 2016 at 10:31 AM,  <stevenjb@chromium.org> wrote:
> I spent some time looking more closely at IOThread and the proxy related
> classes
> it controls. It's... complicated, and frankly deserves some significant
> cleanup.
>
> Rather than attempt that at this point, I have taken a different approach,
> using
> a pre-existing (but unused) IsShuttingDown observer method for
> NetworkStateHandlerObserver. (It was incorrectly implemented but easy to
> fix).
>
> PTAL
>
> (sky@ - your owners review is no longer strictly required, but feedback is
> always welcome if you like)

I'm going to leave it to the experts!

>
>
> https://codereview.chromium.org/1562593002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[stevenjb, 2016-01-06 22:25:17]

Tests should all be fixed now.
PTAL

[oshima, 2016-01-06 22:57:23]

https://codereview.chromium.org/1562593002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

https://codereview.chromium.org/1562593002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/proxy_config_service_impl.h:55: void IsShuttingDown()
override;
OnShuttingDown would be more consistent with the rest of chrome code.

https://codereview.chromium.org/1562593002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/network_screen.cc (right):

https://codereview.chromium.org/1562593002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/network_screen.cc:122: // this as a
NetworkStateHandler observer when the view is destroyed.
Are you going to refactor the code? If so, please file a bug for refactor and
add TODO.

https://codereview.chromium.org/1562593002/diff/100001/chromeos/network/netwo...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/1562593002/diff/100001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:72:
NetworkStateHandler::~NetworkStateHandler() {
I believe Shutodnw() must have been called at this point? Maybe

DCHECK(did_shutdown_) ?

[stevenjb, 2016-01-06 23:28:32]

https://codereview.chromium.org/1562593002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

https://codereview.chromium.org/1562593002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/proxy_config_service_impl.h:55: void IsShuttingDown()
override;
On 2016/01/06 22:57:23, oshima wrote:
> OnShuttingDown would be more consistent with the rest of chrome code.

That ship has kind of sailed, the method already exists.

https://codereview.chromium.org/1562593002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/network_screen.cc (right):

https://codereview.chromium.org/1562593002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/network_screen.cc:122: // this as a
NetworkStateHandler observer when the view is destroyed.
On 2016/01/06 22:57:23, oshima wrote:
> Are you going to refactor the code? If so, please file a bug for refactor and
> add TODO.

I wasn't planning to, I'm not super familiar with the login code. I just looked
at it enough to determine that there wasn't a simple/straightforward to ensure
this class was destroyed before NetworkHandler. There shouldn't be any problems
with putting this call here, I just added the explanation in case someone does
decide to re-factor this.

https://codereview.chromium.org/1562593002/diff/100001/chromeos/network/netwo...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/1562593002/diff/100001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:72:
NetworkStateHandler::~NetworkStateHandler() {
On 2016/01/06 22:57:23, oshima wrote:
> I believe Shutodnw() must have been called at this point? Maybe
> 
> DCHECK(did_shutdown_) ?

So, -usually- Shutdown will get called. However, in unit tests we do not use
NetworkHandler, instead we instantiate individual handlers as needed, which is
why I added the extra Shutdown() call. I can add the test in Shutdown() here
instead and add a comment.

[stevenjb, 2016-01-06 23:28:38]

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

[commit-bot: I haz the power, 2016-01-06 23:29:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1562593002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1562593002/120001

[commit-bot: I haz the power, 2016-01-06 23:57:53]

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

[commit-bot: I haz the power, 2016-01-06 23:57:54]

Dry run: This issue passed the CQ dry run.

[oshima, 2016-01-07 00:21:13]

https://codereview.chromium.org/1562593002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

https://codereview.chromium.org/1562593002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/proxy_config_service_impl.h:55: void IsShuttingDown()
override;
On 2016/01/06 23:28:31, stevenjb wrote:
> On 2016/01/06 22:57:23, oshima wrote:
> > OnShuttingDown would be more consistent with the rest of chrome code.
> 
> That ship has kind of sailed, the method already exists.

There is only one existing client right?

https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/network/h...

https://codereview.chromium.org/1562593002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/screens/network_screen.cc (right):

https://codereview.chromium.org/1562593002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/screens/network_screen.cc:122: // this as a
NetworkStateHandler observer when the view is destroyed.
On 2016/01/06 23:28:31, stevenjb wrote:
> On 2016/01/06 22:57:23, oshima wrote:
> > Are you going to refactor the code? If so, please file a bug for refactor
and
> > add TODO.
> 
> I wasn't planning to, I'm not super familiar with the login code. I just
looked
> at it enough to determine that there wasn't a simple/straightforward to ensure
> this class was destroyed before NetworkHandler. There shouldn't be any
problems
> with putting this call here, I just added the explanation in case someone does
> decide to re-factor this.
> 

Acknowledged.

https://codereview.chromium.org/1562593002/diff/100001/chromeos/network/netwo...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/1562593002/diff/100001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:72:
NetworkStateHandler::~NetworkStateHandler() {
On 2016/01/06 23:28:31, stevenjb wrote:
> On 2016/01/06 22:57:23, oshima wrote:
> > I believe Shutodnw() must have been called at this point? Maybe
> > 
> > DCHECK(did_shutdown_) ?
> 
> So, -usually- Shutdown will get called. However, in unit tests we do not use
> NetworkHandler, instead we instantiate individual handlers as needed, which is
> why I added the extra Shutdown() call. I can add the test in Shutdown() here
> instead and add a comment.

Yeah, I prefer to have the same code path on test and real code path.

[stevenjb, 2016-01-07 17:07:47]

https://codereview.chromium.org/1562593002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

https://codereview.chromium.org/1562593002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/proxy_config_service_impl.h:55: void IsShuttingDown()
override;
On 2016/01/07 00:21:12, oshima wrote:
> On 2016/01/06 23:28:31, stevenjb wrote:
> > On 2016/01/06 22:57:23, oshima wrote:
> > > OnShuttingDown would be more consistent with the rest of chrome code.
> > 
> > That ship has kind of sailed, the method already exists.
> 
> There is only one existing client right?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/network/h...

Sure, I guess. I hate to mix re-factorings, but I'll go ahead and change it just
to get this in.

https://codereview.chromium.org/1562593002/diff/100001/chromeos/network/netwo...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/1562593002/diff/100001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:72:
NetworkStateHandler::~NetworkStateHandler() {
On 2016/01/07 00:21:12, oshima wrote:
> On 2016/01/06 23:28:31, stevenjb wrote:
> > On 2016/01/06 22:57:23, oshima wrote:
> > > I believe Shutodnw() must have been called at this point? Maybe
> > > 
> > > DCHECK(did_shutdown_) ?
> > 
> > So, -usually- Shutdown will get called. However, in unit tests we do not use
> > NetworkHandler, instead we instantiate individual handlers as needed, which
is
> > why I added the extra Shutdown() call. I can add the test in Shutdown() here
> > instead and add a comment.
> 
> Yeah, I prefer to have the same code path on test and real code path.

So do I.

Unfortunately sometimes that conflicts with the principal of minimizing code in
unit tests. Because of the complexity of the NetworkHandler class we decided to
allow unit tests to instantiate individual networking classes (e.g. just
NetworkStateHandler).

If we call Shutdown() in the destructor the global accessor no longer exists
(the scoped pointer that owns the class is null) so classes that use
NetworkHandler::Get()->network_state_handler() will be unable to access
NetworkStateHandler to remove their observer (e.g.
ProxyConfigServiceImpl::OnShuttingDown). 

However in a unit test that just instantiates a NetworkStateHandler instance,
there is no NetworkHandler so we have to call Shutdown() from here for classes
that rely on OnShuttingDown for their cleanup, e.g. HostResolverImpllChromeos).

[oshima, 2016-01-07 18:14:17]

https://codereview.chromium.org/1562593002/diff/100001/chromeos/network/netwo...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/1562593002/diff/100001/chromeos/network/netwo...
chromeos/network/network_state_handler.cc:72:
NetworkStateHandler::~NetworkStateHandler() {
On 2016/01/07 17:07:47, stevenjb wrote:
> On 2016/01/07 00:21:12, oshima wrote:
> > On 2016/01/06 23:28:31, stevenjb wrote:
> > > On 2016/01/06 22:57:23, oshima wrote:
> > > > I believe Shutodnw() must have been called at this point? Maybe
> > > > 
> > > > DCHECK(did_shutdown_) ?
> > > 
> > > So, -usually- Shutdown will get called. However, in unit tests we do not
use
> > > NetworkHandler, instead we instantiate individual handlers as needed,
which
> is
> > > why I added the extra Shutdown() call. I can add the test in Shutdown()
here
> > > instead and add a comment.
> > 
> > Yeah, I prefer to have the same code path on test and real code path.
> 
> So do I.
> 
> Unfortunately sometimes that conflicts with the principal of minimizing code
in
> unit tests.

There are multiple principals (minimizing divergence between test and real code)
and we just need to keep good balance of them, and I think testing what's
actually happening in production is more important, especially this CL is to fix
the shutdown sequence.

> Because of the complexity of the NetworkHandler class we decided to
> allow unit tests to instantiate individual networking classes (e.g. just
> NetworkStateHandler).
> 
> If we call Shutdown() in the destructor the global accessor no longer exists
> (the scoped pointer that owns the class is null) so classes that use
> NetworkHandler::Get()->network_state_handler() will be unable to access
> NetworkStateHandler to remove their observer (e.g.
> ProxyConfigServiceImpl::OnShuttingDown). 
> 
> However in a unit test that just instantiates a NetworkStateHandler instance,
> there is no NetworkHandler so we have to call Shutdown() from here for classes
> that rely on OnShuttingDown for their cleanup, e.g.
HostResolverImpllChromeos).

I fully understand why it will not necessary for certain unit tests. It's just
my recommendation which may help to catch future regression, as memory problems
aren't always be catch-able in CQ bots. I won't block this since you own this.
lgtm.

[stevenjb, 2016-01-07 18:30:22]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2016-01-07 18:32:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1562593002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1562593002/140001

[commit-bot: I haz the power, 2016-01-07 18:44:48]

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

[commit-bot: I haz the power, 2016-01-07 18:44:49]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[stevenjb, 2016-01-07 19:14:26]

Description was changed from

==========
Fix potential crashes in NetworkHandler code

This CL:
* Properly removes NetworkConnectionHandler observer in
  ~AutoConnectHandler.
* Checks observer lists in NetworkHandler classes on shutdown (to catch
  any future regressions).
* Destroys ProxyConfigServiceImpl owned by IOThread before
  NetworkHandler is destroyed to remove the NetworkStateHandler
  observer.

BUG=572914
==========

to

==========
Fix potential crashes in NetworkHandler code

This CL:
* Properly removes NetworkConnectionHandler observer in
  ~AutoConnectHandler.
* Checks observer lists in NetworkHandler classes on shutdown (to catch
  any future regressions).
* Destroys ProxyConfigServiceImpl owned by IOThread before
  NetworkHandler is destroyed to remove the NetworkStateHandler
  observer.

BUG=572914
For chrome/browser/extensions/api/vpn_provider/vpn_provider_apitest.cc:
TBR=rdevlin.cronin@chromium.org
==========

[stevenjb, 2016-01-07 19:17:40]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2016-01-07 19:18:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1562593002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1562593002/140001

[commit-bot: I haz the power, 2016-01-07 19:25:54]

Description was changed from

==========
Fix potential crashes in NetworkHandler code

This CL:
* Properly removes NetworkConnectionHandler observer in
  ~AutoConnectHandler.
* Checks observer lists in NetworkHandler classes on shutdown (to catch
  any future regressions).
* Destroys ProxyConfigServiceImpl owned by IOThread before
  NetworkHandler is destroyed to remove the NetworkStateHandler
  observer.

BUG=572914
For chrome/browser/extensions/api/vpn_provider/vpn_provider_apitest.cc:
TBR=rdevlin.cronin@chromium.org
==========

to

==========
Fix potential crashes in NetworkHandler code

This CL:
* Properly removes NetworkConnectionHandler observer in
  ~AutoConnectHandler.
* Checks observer lists in NetworkHandler classes on shutdown (to catch
  any future regressions).
* Destroys ProxyConfigServiceImpl owned by IOThread before
  NetworkHandler is destroyed to remove the NetworkStateHandler
  observer.

BUG=572914
For chrome/browser/extensions/api/vpn_provider/vpn_provider_apitest.cc:
TBR=rdevlin.cronin@chromium.org
==========

[commit-bot: I haz the power, 2016-01-07 19:25:55]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-01-07 19:26:43]

Description was changed from

==========
Fix potential crashes in NetworkHandler code

This CL:
* Properly removes NetworkConnectionHandler observer in
  ~AutoConnectHandler.
* Checks observer lists in NetworkHandler classes on shutdown (to catch
  any future regressions).
* Destroys ProxyConfigServiceImpl owned by IOThread before
  NetworkHandler is destroyed to remove the NetworkStateHandler
  observer.

BUG=572914
For chrome/browser/extensions/api/vpn_provider/vpn_provider_apitest.cc:
TBR=rdevlin.cronin@chromium.org
==========

to

==========
Fix potential crashes in NetworkHandler code

This CL:
* Properly removes NetworkConnectionHandler observer in
  ~AutoConnectHandler.
* Checks observer lists in NetworkHandler classes on shutdown (to catch
  any future regressions).
* Destroys ProxyConfigServiceImpl owned by IOThread before
  NetworkHandler is destroyed to remove the NetworkStateHandler
  observer.

BUG=572914
For chrome/browser/extensions/api/vpn_provider/vpn_provider_apitest.cc:
TBR=rdevlin.cronin@chromium.org

Committed: https://crrev.com/bbbd48b2dc5bbde2472eafe4319f29937e3b7de6
Cr-Commit-Position: refs/heads/master@{#368118}
==========

[commit-bot: I haz the power, 2016-01-07 19:26:44]

Patchset 7 (id:??) landed as
https://crrev.com/bbbd48b2dc5bbde2472eafe4319f29937e3b7de6
Cr-Commit-Position: refs/heads/master@{#368118}