Bug 58027 – Avoid leaking document when leaving google.com due to geolocation permission request

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Kenneth Rohde Christiansen]

SSIA

[Kenneth Rohde Christiansen]

Created attachment 88606 [details]
Patch

[John Knottenbelt]

Comment on attachment 88606 [details]
Patch

This seems correct to me. If we are removing the notifiers from the oneShots and watchers list, it also makes sense to remove them from the other lists too.

[Steve Block]

Comment on attachment 88606 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        In fatalErrorOccurred (which is called on cancellation), the notifier

What do you mean by 'cancellation'? Can you elaborate in the bug exactly what the problem is? Presumably it's not unique to google.com?

> Source/WebCore/ChangeLog:18
> +

Can you add a test for this? If not, you should explain here why not.

> Source/WebCore/page/Geolocation.cpp:342
> +    m_requestsAwaitingCachedPosition.remove(notifier);

I think that the only way for a notifier to be in this list when it encounters a fatal error is when Geolocation::reset() is called. Is this what you're protecting against?

[Kenneth Rohde Christiansen]

(In reply to comment #3)
> (From update of attachment 88606 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88606&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        In fatalErrorOccurred (which is called on cancellation), the notifier
> 
> What do you mean by 'cancellation'? Can you elaborate in the bug exactly what the problem is? Presumably it's not unique to google.com?

Reloading the page or loading another one, leaks document() probably due to event holding a ref to document (presumable TargetEvent).

It probably happens on other pages as well (using watchPosition?), but I didn't manage to trigger the leak with my own simple tests, and debugging what exactly is happening on google.com is pretty hard due to the obscured javascript code.

> 
> > Source/WebCore/ChangeLog:18
> > +
> 
> Can you add a test for this? If not, you should explain here why not.

The test can only work for ports using the PREEMPT_... build flag, and I am not sure now to actually test this, so any help is appreciated. Testing it is hard due to the reloading, plus the fact that I cannot AFAIK count the document references from LayoutTestController.

> > Source/WebCore/page/Geolocation.cpp:342
> > +    m_requestsAwaitingCachedPosition.remove(notifier);
> 
> I think that the only way for a notifier to be in this list when it encounters a fatal error is when Geolocation::reset() is called. Is this what you're protecting against?

I added this one for completions sake. It can be left out.

[Steve Block]

Comment on attachment 88606 [details]
Patch

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

>>> Source/WebCore/ChangeLog:11
>>> +        In fatalErrorOccurred (which is called on cancellation), the notifier
>> 
>> What do you mean by 'cancellation'? Can you elaborate in the bug exactly what the problem is? Presumably it's not unique to google.com?
> 
> Reloading the page or loading another one, leaks document() probably due to event holding a ref to document (presumable TargetEvent).
> 
> It probably happens on other pages as well (using watchPosition?), but I didn't manage to trigger the leak with my own simple tests, and debugging what exactly is happening on google.com is pretty hard due to the obscured javascript code.

Do you see this only on Chromium? I think it would be good to understand a little more about the problem before we make the fix. The Geolocation callbacks don't ref their ScriptExecutionContext, so I don't see how removing this notifier from this list will avoid a leak. Presumably the Geolocation object is leaked too?

[Kenneth Rohde Christiansen]

> Do you see this only on Chromium? I think it would be good to understand a little more about the problem before we make the fix. The Geolocation callbacks don't ref their ScriptExecutionContext, so I don't see how removing this notifier from this list will avoid a leak. Presumably the Geolocation object is leaked too?

This happens on our Qt WebKit2 port (a branch on gitorious). It was pretty hard to debug because Document is ref'ed for each event, but adding this plugged the leak.

[Eric Seidel (no email)]

Comment on attachment 88606 [details]
Patch

We need to find some way to test this.  Manual tests are fine.

[John Knottenbelt]

Comment on attachment 88606 [details]
Patch

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

A test to count the number of notifiers left in the lists after page reload should be possible. However, exposing this count seems only useful for this particular test case. Is there a way to add this test without complicating the interfaces of the Geolocation, GeolocationController, GeolocationClient and embedder-specific client classes?

>>> Source/WebCore/page/Geolocation.cpp:342
>>> +    m_requestsAwaitingCachedPosition.remove(notifier);
>> 
>> I think that the only way for a notifier to be in this list when it encounters a fatal error is when Geolocation::reset() is called. Is this what you're protecting against?
> 
> I added this one for completions sake. It can be left out.

Steve: Yes, we can get a build up of notifiers in this case, since the main Frame (and therefore Geolocation instance) is reused across page reloads.

[John Knottenbelt]

fast/dom/Geolocation/page-reload-cancel-permission-requests.html is a related test that tests that geolocation permission requests are properly cancelled at the GeolocationClient on page reload.

(In reply to comment #8)
> (From update of attachment 88606 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88606&action=review
> 
> A test to count the number of notifiers left in the lists after page reload should be possible. However, exposing this count seems only useful for this particular test case. Is there a way to add this test without complicating the interfaces of the Geolocation, GeolocationController, GeolocationClient and embedder-specific client classes?
> 
> >>> Source/WebCore/page/Geolocation.cpp:342
> >>> +    m_requestsAwaitingCachedPosition.remove(notifier);
> >> 
> >> I think that the only way for a notifier to be in this list when it encounters a fatal error is when Geolocation::reset() is called. Is this what you're protecting against?
> > 
> > I added this one for completions sake. It can be left out.
> 
> Steve: Yes, we can get a build up of notifiers in this case, since the main Frame (and therefore Geolocation instance) is reused across page reloads.

[Satish Sampath]

I've noticed the same issue. I've tested only with Chromium so not sure if it happens with other browsers as well. FWIW, I found adding the following to the Geolocation::reset() method works as well.

#if USE(PREEMPT_GEOLOCATION_PERMISSION)
    m_pendingForPermissionNotifiers.remove(notifier);
#endif

[Satish Sampath]

I can also confirm that the leaking of Document was due to these pending GeoNotifier objects. In chromium these were creating a persistent v8 handle which weren't released properly (as these were still alive when the page closed) caused the whole DOM to be leaked when the page closed.

[Satish Sampath]

Sorry, I should've mentioned that I used this code in the reset() method:

#if USE(PREEMPT_GEOLOCATION_PERMISSION)
    m_pendingForPermissionNotifiers.clear();
#endif

[John Knottenbelt]

I think we should also be removing the watch from  m_pendingForPermissionNotifiers in Geolocation::clearWatch to avoid potential build up of watches by repeated calls to watchPosition and clearWatch.

[Satish Sampath]

Created attachment 111130 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 111130 [details]
Patch

Nice, but can you implement John's suggestion as well? It could be a follow up.

[John Knottenbelt]

Comment on attachment 111130 [details]
Patch

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

> Source/WebCore/page/Geolocation.cpp:344
> +#if USE(PREEMPT_GEOLOCATION_PERMISSION)

The change to Geolocation::reset() is enough - m_pendingForPermissionNotifiers is cleared in Geolocation::setIsAllowed. All other calls to setFatalError are called after permission has been determined, so the notifier won't be in this list.

It would be good to move these lines to Geolocation::clearWatch(), though, to avoid a potential build up of watches in this set.

[Satish Sampath]

Comment on attachment 111130 [details]
Patch

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

>> Source/WebCore/page/Geolocation.cpp:344
>> +#if USE(PREEMPT_GEOLOCATION_PERMISSION)
> 
> The change to Geolocation::reset() is enough - m_pendingForPermissionNotifiers is cleared in Geolocation::setIsAllowed. All other calls to setFatalError are called after permission has been determined, so the notifier won't be in this list.
> 
> It would be good to move these lines to Geolocation::clearWatch(), though, to avoid a potential build up of watches in this set.

Will do.

[Satish Sampath]

Created attachment 111178 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 111178 [details]
Patch

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

> Source/WebCore/page/Geolocation.cpp:178
> +Geolocation::GeoNotifier* Geolocation::Watchers::get(int id)

WebKit style would be to call this method for "find"

[Kenneth Rohde Christiansen]

Comment on attachment 111178 [details]
Patch

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

> Source/WebCore/page/Geolocation.cpp:436
> +    GeoNotifier* notifier = m_watchers.get(watchId);
> +    if (notifier)
> +        m_pendingForPermissionNotifiers.remove(notifier);

You could change this into:

 if (GeoNotifier* notifier = m_watchers.find(watchId))
     m_pendingForPermissionNotifiers.remove(notifier);

[Kenneth Rohde Christiansen]

> > The change to Geolocation::reset() is enough - m_pendingForPermissionNotifiers is cleared in Geolocation::setIsAllowed.

I think it would be better do document such behavior using ASSERTs if possible.

[Satish Sampath]

Comment on attachment 111178 [details]
Patch

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

>> Source/WebCore/page/Geolocation.cpp:178
>> +Geolocation::GeoNotifier* Geolocation::Watchers::get(int id)
> 
> WebKit style would be to call this method for "find"

Will do

>> Source/WebCore/page/Geolocation.cpp:436
>> +        m_pendingForPermissionNotifiers.remove(notifier);
> 
> You could change this into:
> 
>  if (GeoNotifier* notifier = m_watchers.find(watchId))
>      m_pendingForPermissionNotifiers.remove(notifier);

Will do

[Satish Sampath]

Created attachment 111186 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 111186 [details]
Patch

John are you OK with this?

[John Knottenbelt]

(In reply to comment #24)
> (From update of attachment 111186 [details])
> John are you OK with this?

Looks good to me. thanks for reviewing!

[WebKit Review Bot]

Comment on attachment 111186 [details]
Patch

Clearing flags on attachment: 111186

Committed r97594: <http://trac.webkit.org/changeset/97594>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Steve Block]

Comment on attachment 111186 [details]
Patch

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

> Source/WebCore/page/Geolocation.h:116
> +        GeoNotifier* find(int id);

Do we need this new method? All we do with the GeoNotifier returned by this method is pass it to remove(GeoNotifier*), but Watchers already offers a remove(int id) method, which achieves the same thing.

[Satish Sampath]

Comment on attachment 111186 [details]
Patch

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

>> Source/WebCore/page/Geolocation.h:116
>> +        GeoNotifier* find(int id);
> 
> Do we need this new method? All we do with the GeoNotifier returned by this method is pass it to remove(GeoNotifier*), but Watchers already offers a remove(int id) method, which achieves the same thing.

You're right, I don't know how I missed that :/ Will upload a patch to remove this method and use remove(int id) instead.

[Satish Sampath]

Comment on attachment 111186 [details]
Patch

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

>>> Source/WebCore/page/Geolocation.h:116
>>> +        GeoNotifier* find(int id);
>> 
>> Do we need this new method? All we do with the GeoNotifier returned by this method is pass it to remove(GeoNotifier*), but Watchers already offers a remove(int id) method, which achieves the same thing.
> 
> You're right, I don't know how I missed that :/ Will upload a patch to remove this method and use remove(int id) instead.

m_pendingForPermissionNotifiers is not part of m_watchers. So calling Watchers::remove(int id) can't remove this pending item. So leaving it as is.

[Steve Block]

>>>> Source/WebCore/page/Geolocation.cpp:342
>>>> +    m_requestsAwaitingCachedPosition.remove(notifier);
>>> 
>>> I think that the only way for a notifier to be in this list when it encounters a fatal error is when Geolocation::reset() is called. Is this what you're protecting against?
>> I added this one for completions sake. It can be left out.
> Steve: Yes, we can get a build up of notifiers in this case, since the main Frame (and therefore Geolocation instance) is reused across page reloads.

John, are you still concerned about this? How exactly do we handle cancelling regular requests in this case? It looks like the only time we do any cancelling of requests is when reset() or disconectFrame() is called. Is either of these called in this case? If you think there's a problem, let's open a new bug, preferably with a test case.

[John Knottenbelt]

Steve, are you asking about my earlier proposed change to Geolocation::fatalErrorOccurred? If so, I think it's not necessary to make any changes there now as they will be cleared up in ::reset() with Satish's change.

(In reply to comment #31)
> >>>> Source/WebCore/page/Geolocation.cpp:342
> >>>> +    m_requestsAwaitingCachedPosition.remove(notifier);
> >>> 
> >>> I think that the only way for a notifier to be in this list when it encounters a fatal error is when Geolocation::reset() is called. Is this what you're protecting against?
> >> I added this one for completions sake. It can be left out.
> > Steve: Yes, we can get a build up of notifiers in this case, since the main Frame (and therefore Geolocation instance) is reused across page reloads.
> 
> John, are you still concerned about this? How exactly do we handle cancelling regular requests in this case? It looks like the only time we do any cancelling of requests is when reset() or disconectFrame() is called. Is either of these called in this case? If you think there's a problem, let's open a new bug, preferably with a test case.

[Steve Block]

Yes, I was talking about your comment about clearing m_requestsAwaitingCachedPosition in fatalErrorOccurred() in the first patch set uploaded to this bug. Satish's patch doesn't touch m_requestsAwaitingCachedPosition, right?

[John Knottenbelt]

Steve, I'm not sure if there is a problem there now. I've opened https://bugs.webkit.org/show_bug.cgi?id=70461 to track an investigation of the question.