Speculative fix for chromium-os:4706 - use singleton NetworkLibrary rather than create our own

Speculative fix for chromium-os:4706 - use singleton NetworkLibrary rather than create our own

BUG=chromium-os:4706
TEST=open chrome os, go to maps.google.com click my location, check it doesn't crash.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=55204

[joth, 2010-08-05 13:18:04]

+ davemoore@chromium.org
Just noticed your http://src.chromium.org/viewvc/chrome?view=rev&revision=54950
impacted this code (alas, the european git mirror lag meant I didn't see this
sooner).

Looks like the change linked will have fixed the problem in hand, but introduce
another one: when the wifi location provider is destroyed, it will delete (via
the scoped_ptr) the singleton NetworkLibrary instance. I will see about making 
a follow-up CL to make the base class d'tors protected to avoid this class of
error.

[joth, 2010-08-06 09:16:21]

As this is beta blocker, I'll go ahead and submit TBR.


On 5 August 2010 14:18, <joth@chromium.org> wrote:

> + davemoore@chromium.org
> Just noticed your
> http://src.chromium.org/viewvc/chrome?view=rev&revision=54950
> impacted this code (alas, the european git mirror lag meant I didn't see
> this
> sooner).
>
> Looks like the change linked will have fixed the problem in hand, but
> introduce
> another one: when the wifi location provider is destroyed, it will delete
> (via
> the scoped_ptr) the singleton NetworkLibrary instance. I will see about
> making
> a follow-up CL to make the base class d'tors protected to avoid this class
> of
> error.
>
>
> http://codereview.chromium.org/3027042/show
>

[bulach, 2010-08-06 09:22:33]

LGTM

[Charlie Lee (do not use), 2010-08-06 17:28:00]

LGTM too, but I don't think that will fix this issue. We only have one  
instance of NetworkLibrary and it never gets deleted.

I will take a look at the crash to see if I see anything.

- Charlie

On Aug 6, 2010 2:22:33 AM PDT, bulach <bulach@chromium.org> wrote:
> LGTM

[joth, 2010-08-06 17:46:20]

On 6 August 2010 18:27, Charlie Lee <chocobo@google.com> wrote:

> LGTM too, but I don't think that will fix this issue. We only have one
> instance of NetworkLibrary and it never gets deleted.
>
Nothing actually enforces this though. Stashing the NetworkLibrary instance
in the scoped_ptr (like this code was) will result in premature deletion,
when the WifiDataProviderChromeOs is deleted.

However! Note the bug with call stack was reported on an earlier version of
this code, before Dave's changes:
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/geolocation/wi...

In that version, an additional instance of NetworkLibrary was being created.
But it was being constructed on the wrong thread, that is the IO thread,
thereby likely exercising an untested path in the NetworkLibrary constructor




>
> I will take a look at the crash to see if I see anything.
>
> Yes please do! more eyes the better (and it's now getting to the end of the
day/week UK time)



> - Charlie
>
> On Aug 6, 2010 2:22:33 AM PDT, bulach <bulach@chromium.org> wrote:
>
>> LGTM
>>
>