Bug 151532 – Fix crash in ~WebProcessPool when using Geolocation with useNetworkProcess=1

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Fix crash in ~WebProcessPool when using Geolocation with useNetworkProcess=1

[Alex Christensen]

Created attachment 266026 [details]
Patch

[Benjamin Poulain]

Comment on attachment 266026 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=266026&action=review

> Source/WebKit2/UIProcess/WebGeolocationManagerProxy.cpp:66
> +    if (wasUpdating && !isUpdating())
> +        m_provider.stopUpdating(this);
> +    else {

Actually, you only need 
    ASSERT(!isUpdating());
    if (wasUpdating)
        m_provider.stopUpdating(this);

If isUpdating() is true, something really fucked up is going on.

It looks like the API contract does not require you to change the high accuracy settings if you stop the providers. Let's remove the else {} case.

> Tools/ChangeLog:14
> +        Properly load about:blank in all WebViews to clean up.

IMHO, it would be worth explaining why.
Can you add a paragraph explaining that we had a Geolocation provider stopping after its state tracker was destroyed with its stack frame?

> Tools/TestWebKitAPI/Tests/WebKit2/Geolocation.cpp:235
> +            EXPECT_EQ(GeolocationEvent::DisableHighAccuracy, events[3]);

We know these do not work. Can you find a way to make them work?

If not, maybe turn them into assertions?

[Alex Christensen]

(In reply to comment #2)
> Comment on attachment 266026 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=266026&action=review
> 
> > Source/WebKit2/UIProcess/WebGeolocationManagerProxy.cpp:66
> > +    if (wasUpdating && !isUpdating())
> > +        m_provider.stopUpdating(this);
> > +    else {
> 
> Actually, you only need 
>     ASSERT(!isUpdating());
>     if (wasUpdating)
>         m_provider.stopUpdating(this);
> 
> If isUpdating() is true, something really fucked up is going on.
> 
> It looks like the API contract does not require you to change the high
> accuracy settings if you stop the providers. Let's remove the else {} case.
Done.
> 
> > Tools/ChangeLog:14
> > +        Properly load about:blank in all WebViews to clean up.
> 
> IMHO, it would be worth explaining why.
> Can you add a paragraph explaining that we had a Geolocation provider
> stopping after its state tracker was destroyed with its stack frame?
Done.
> 
> > Tools/TestWebKitAPI/Tests/WebKit2/Geolocation.cpp:235
> > +            EXPECT_EQ(GeolocationEvent::DisableHighAccuracy, events[3]);
> 
> We know these do not work. Can you find a way to make them work?
> 
> If not, maybe turn them into assertions?
These do not work if the process crashes, but they do work if the process does not crash.  I'm leaving them as they are.

[Alex Christensen]

http://trac.webkit.org/changeset/192747