Bug 39908 - Reentrant Geolocation tests crash with an assertion
Summary: Reentrant Geolocation tests crash with an assertion
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 48518 49597
Blocks:
  Show dependency treegraph
 
Reported: 2010-05-28 15:40 PDT by Alexey Proskuryakov
Modified: 2010-11-19 07:07 PST (History)
6 users (show)

See Also:


Attachments
Patch (4.33 KB, patch)
2010-11-16 09:13 PST, John Knottenbelt
no flags Details | Formatted Diff | Diff
Patch (5.16 KB, patch)
2010-11-17 06:04 PST, John Knottenbelt
no flags Details | Formatted Diff | Diff
Patch (5.24 KB, patch)
2010-11-17 08:45 PST, John Knottenbelt
no flags Details | Formatted Diff | Diff
Patch (6.15 KB, patch)
2010-11-18 08:01 PST, John Knottenbelt
no flags Details | Formatted Diff | Diff
Patch (5.36 KB, patch)
2010-11-18 10:40 PST, John Knottenbelt
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2010-05-28 15:40:41 PDT
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
Comment 1 Alexey Proskuryakov 2010-06-03 17:06:31 PDT
See also: bug 40148.
Comment 2 Steve Block 2010-06-11 01:51:43 PDT
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.
Comment 3 John Knottenbelt 2010-11-16 09:13:27 PST
Created attachment 74002 [details]
Patch
Comment 4 John Knottenbelt 2010-11-16 09:14:42 PST
Comment on attachment 74002 [details]
Patch

Depends on https://bugs.webkit.org/show_bug.cgi?id=49597 for GeoNotifierVector typedef.
Comment 5 Jonathan Dixon 2010-11-16 10:44:51 PST
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
Comment 6 John Knottenbelt 2010-11-17 06:04:07 PST
Created attachment 74101 [details]
Patch
Comment 7 Jonathan Dixon 2010-11-17 07:42:02 PST
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?
Comment 8 John Knottenbelt 2010-11-17 08:45:20 PST
Created attachment 74119 [details]
Patch
Comment 9 Jeremy Orlow 2010-11-18 06:34:03 PST
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
Comment 10 Jonathan Dixon 2010-11-18 06:39:47 PST
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?
Comment 11 Marcus Bulach 2010-11-18 06:55:43 PST
(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
Comment 12 John Knottenbelt 2010-11-18 07:53:57 PST
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.
Comment 13 John Knottenbelt 2010-11-18 08:01:35 PST
Created attachment 74240 [details]
Patch
Comment 14 Marcus Bulach 2010-11-18 08:16:28 PST
(In reply to comment #13)
> Created an attachment (id=74240) [details]
> Patch

LGTM, thanks john!
Comment 15 Jeremy Orlow 2010-11-18 08:26:22 PST
Comment on attachment 74240 [details]
Patch

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

> WebCore/ChangeLog:19
> +2010-11-16  John Knottenbelt  <jknotten@chromium.org>

Why the duplication?

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

no {}
Comment 16 John Knottenbelt 2010-11-18 10:40:41 PST
Created attachment 74255 [details]
Patch
Comment 17 Jeremy Orlow 2010-11-18 10:43:41 PST
Comment on attachment 74255 [details]
Patch

r=me
Comment 18 Alexey Proskuryakov 2010-11-18 10:58:34 PST
Thanks for tackling this!
Comment 19 WebKit Commit Bot 2010-11-19 07:06:56 PST
Comment on attachment 74255 [details]
Patch

Clearing flags on attachment: 74255

Committed r72396: <http://trac.webkit.org/changeset/72396>
Comment 20 WebKit Commit Bot 2010-11-19 07:07:02 PST
All reviewed patches have been landed.  Closing bug.