[Webkit-unassigned] [Bug 39879] Geolocation activity started after frame has been disconnected can cause crash

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=39879





--- Comment #21 from Steve Block <steveblock at google.com>  2010-06-08 09:06:07 PST ---
(In reply to comment #17)
> (From update of attachment 58137 [details])
> WebCore/page/Geolocation.cpp: 
>  +      // This method is called at most once on a given GeoNotifier object.
> Why is this getting removed?  What happens if the timer has already started
> for another error?  Etc...  This probably deserves mention in the change log.
When the Frame is disconnected, we stop any timers for all ongoing requests and then set a fatal error, which restarts the timer. It's possible that the request already has a fatal error set, so the ASSERT() doesn't hold. Though on second thoughts, it's probably best to keep the original error.

> WebCore/page/Geolocation.cpp:500
>  +          RefPtr<GeoNotifier> notifier = *it;
> Why do you need to do this?  It doesn't seem like it saves any space.  I 
> guess it's slightly more readable....
Will fix.

> WebCore/page/Geolocation.cpp:508
>  +      copyToVector(m_oneShots, copy);
> Do you need to null out the list?
No, copyValuesToVector() explictly resizes the vector and sets every element

> WebCore/page/Geolocation.cpp:510
>  +      m_watchers.getNotifiersVector(copy);
> This shouldn't have had get in the name...
How about 'copyNotifiersToVector()'?

> WebCore/page/Geolocation.h:138
>  +      void cancelAllRequests(Vector<RefPtr<GeoNotifier> >&);
> Why name this the same thing and make it public?  It seems like just a helper
> function.
Neither of these are public. I'll change the name of the helper and make it static to make it clear.

> WebCore/page/Geolocation.cpp:50
>  +  static const char serviceNoLongerAvailableErrorMessage[] = "Geolocation service no longer available";
> Are these messages supposed to not have a period at the end?
I just copied the pattern of the first error message added by Greg in http://trac.webkit.org/changeset/37854/trunk/WebCore/page/Geolocation.cpp I don't have a strong opinion.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.