Bug 39908 – Reentrant Geolocation tests crash with an assertion

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

An assertion fails in GeolocationController::addObserver(), because we try to add the same # Geolocation object as an observer again.

fast/dom/Geolocation/maximum-age.html
fast/dom/Geolocation/reentrant-error.html
fast/dom/Geolocation/reentrant-success.html

[Alexey Proskuryakov]

See also: bug 40148.

[Steve Block]

reentrant-error.html and reentrant-success.html are now fixed with the fix for Bug 40148.

maximum-age.html still fails in client-based Geolocation. This seems not to be due to the client-based approach itself, but due to the different code path used to obtain permission.

[John Knottenbelt]

Created attachment 74002 [details]
Patch

[John Knottenbelt]

Comment on attachment 74002 [details]
Patch

Depends on https://bugs.webkit.org/show_bug.cgi?id=49597 for GeoNotifierVector typedef.

[Jonathan Dixon]

Comment on attachment 74002 [details]
Patch

Looks good, although hard to follow the logic behind it.
Maybe we could move the pending cache-using requests out of the vector copy and back into the set before calling SendError(). (But after clearing the set). We could do this for both oneshots and watchers (to avoid a cache-using watcher from going into limbo if a fatal error occurred before its cache-pump timer fires)

This way we'd keep the policy logic in one place, rather than half in SendError and half in the copy-back

[John Knottenbelt]

Created attachment 74101 [details]
Patch

[Jonathan Dixon]

Comment on attachment 74101 [details]
Patch

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

> WebCore/page/Geolocation.cpp:582
> +    copyToSet(oneShotsCached, m_oneShots);

It's not obvious when the watchersCached set is not re-instated here too. Specifically, if the error was fatal we won't have sent them an error above (as we removed them from the watchersCopy vector) but they won't get a cached position either (as we don't reinstate here). Perhaps it's the intermix of isFatal and isDenied that is confusing me. Would it be clearer as:

if (!isFatal) {
     removeCachedNotifiers(oneShotsCopy, oneShotsCached);
     removeCachedNotifiers(watchersCopy, watchersCached);
} else 
     m_watchers.clear();

m_oneshots.clear();


Also would it be worth adding a comment on hasListeners that we need to test that before re-instating pending one-shots?

[John Knottenbelt]

Created attachment 74119 [details]
Patch

[Jeremy Orlow]

Comment on attachment 74119 [details]
Patch

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

Steve is probably the best reviewer, but unfortunately he's on holiday.  Here are a few comments to get you started though.

> LayoutTests/ChangeLog:4
> +

include title and bug url at top...in other CLs too

> WebCore/page/Geolocation.cpp:530
> +        RefPtr<GeoNotifier> notifier = *it;

just store a pointer.  No need to ref it.

> WebCore/page/Geolocation.cpp:543
> +         RefPtr<GeoNotifier> notifier = *it;

same

> WebCore/page/Geolocation.cpp:544
> +             dest.add(notifier);

wrong indent

[Jonathan Dixon]

Comment on attachment 74119 [details]
Patch

LGTM. I'd be inclined to comment why we ignore watchersCached, or at least name it ignoreWatchersCached, or maybe even pass a NULL pointer into removeCachedNotifiers to avoid copying out the unneeded refs. I don't know which fits best with webkit style though?

[Marcus Bulach]

(In reply to comment #10)
> (From update of attachment 74119 [details])
> LGTM. I'd be inclined to comment why we ignore watchersCached, or at least name it ignoreWatchersCached, or maybe even pass a NULL pointer into removeCachedNotifiers to avoid copying out the unneeded refs. I don't know which fits best with webkit style though?

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

> WebCore/page/Geolocation.cpp:525
> +void Geolocation::removeCachedNotifiers(GeoNotifierVector& notifiers, GeoNotifierVector& cached)

hmm, I was a bit confused with this name and the fact that it has two params, my original thought was that it was removing "cached" from "notifiers"..
not sure if it'd be any clearer, but perhaps extractNotifiersWithCachedPosition?

also, both this and copyToSet could be static helper functions rather than regular methods.

> WebCore/page/Geolocation.cpp:561
> +    GeoNotifierVector oneShotsCached;

as above, perhaps oneShotsWithCachedPosition

[John Knottenbelt]

Comment on attachment 74119 [details]
Patch

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

>> WebCore/page/Geolocation.cpp:525
>> +void Geolocation::removeCachedNotifiers(GeoNotifierVector& notifiers, GeoNotifierVector& cached)
> 
> hmm, I was a bit confused with this name and the fact that it has two params, my original thought was that it was removing "cached" from "notifiers"..
> not sure if it'd be any clearer, but perhaps extractNotifiersWithCachedPosition?
> 
> also, both this and copyToSet could be static helper functions rather than regular methods.

Agree with method rename. 
GeoNotifier and GeoNotifierVector are private in Geolocation, which makes it hard to bring these methods out as non-member functions. However, they can be static member functions.

>> WebCore/page/Geolocation.cpp:561
>> +    GeoNotifierVector oneShotsCached;
> 
> as above, perhaps oneShotsWithCachedPosition

Agree.

[John Knottenbelt]

Created attachment 74240 [details]
Patch

[Marcus Bulach]

(In reply to comment #13)
> Created an attachment (id=74240) [details]
> Patch

LGTM, thanks john!

[Jeremy Orlow]

Comment on attachment 74240 [details]
Patch

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

> WebCore/ChangeLog:19
> [phone]  John Knottenbelt  <[e-mail]>

Why the duplication?

> WebCore/page/Geolocation.cpp:534
> +        } else {

no {}

[John Knottenbelt]

Created attachment 74255 [details]
Patch

[Jeremy Orlow]

Comment on attachment 74255 [details]
Patch

r=me

[Alexey Proskuryakov]

Thanks for tackling this!

[WebKit Commit Bot]

Comment on attachment 74255 [details]
Patch

Clearing flags on attachment: 74255

Committed r72396: <http://trac.webkit.org/changeset/72396>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.