Bug 39288 – Geolocation causes DOMWindow to leak if position requests are in progress when the page is navigated away

• Component: WebCore Misc.
• Status: RESOLVED WORKSFORME

[Steve Block]

Geolocation causes DOMWindow to leak if position requests are in progress when the page is navigated away

[Steve Block]

Created attachment 56365 [details]
Patch

[Darin Adler]

Comment on attachment 56365 [details]
Patch

Is there any good way to test this? I'd like to prevent us from having this regress in the future.

r=me

[Steve Block]

Comment on attachment 56365 [details]
Patch

Investigating options for LayoutTests. Will upload an updated patch.

[Steve Block]

Created attachment 56503 [details]
Patch

[Steve Block]

Updated patch which handles the case where the page is destroyed, as well as when it is navigated away.

Also adds a LayoutTest which exercises this case. Currently, the test relies on external instrumentation to check for leaks. Or is there a way to do this with LayoutTestController?

[Darin Adler]

Comment on attachment 56503 [details]
Patch

This is one of those cases where a function by function comment in the change log would make it more clear why your changes are correct.

> +    if (m_frame->domWindow() && m_frame->domWindow()->navigator()->optionalGeolocation())
> +        m_frame->domWindow()->navigator()->optionalGeolocation()->stop();

If we’re calling m_frame->domWindow() you should not check it for 0. The code can either call existingDOMWindow or we can remove the null check.

>  void Geolocation::disconnectFrame()
>  {
>      if (m_frame && m_frame->page() && m_allowGeolocation == InProgress)
>          m_frame->page()->chrome()->cancelGeolocationPermissionRequestForFrame(m_frame, this);
> -    stopUpdating();

Is there a guarantee that no new geolocation activity can start after a frame stops loading? What guarantees that? There is not enough explanation here, either in change log or in comments, to make clear why this code is correct.

review- for the moment because of this one issue and one question

[Steve Block]

> If we’re calling m_frame->domWindow() you should not check it for 0.
Done

> Is there a guarantee that no new geolocation activity can start after a frame
> stops loading?
This call is made after the unload event has been sent, which is the last point at which JS can make new calls to Geolocation. This call removes all ongoing Geolocation requests and stops the service, so there's no further activity.

The reason that removing the ongoing requests in Geolocation::disconnectFrame() (as in my first patch) is insufficient, is that in the case of the Page being destroyed, disconnectFrame() is not called until the Frame has been destroyed. However, the Frame may not be destroyed if the Geolocation object still holds references to JS callback functions, as these create circular references.

[Steve Block]

Created attachment 56589 [details]
Patch

[Early Warning System Bot]

Attachment 56589 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/2319345

[Steve Block]

Created attachment 56590 [details]
Patch

[Darin Adler]

Comment on attachment 56590 [details]
Patch

> +     // Stop the Geolocation object, if present. This call is made after the unload
> +     // event has fired, so no new Geolocation activity is possible.
> +    if (m_frame->domWindow()->navigator()->optionalGeolocation())
> +        m_frame->domWindow()->navigator()->optionalGeolocation()->stop();

Incorrect indentation of the comment here.

What about if some other frame in the same domain has access to this frame, and later tries to trigger some geolocation activity in this frame with JavaScript code?

r=me

[Steve Block]

Committed r60069: <http://trac.webkit.org/changeset/60069>

[Steve Block]

> What about if some other frame in the same domain has access to this frame, and
> later tries to trigger some geolocation activity in this frame with JavaScript
> code?
I'll look into this to double-check, but I think it's best to land the current fix now.

Landed updated version of patch to reflect fix to stop timers in http://trac.webkit.org/changeset/59859 and restructuring of Geolocation LayoutTests in http://trac.webkit.org/changeset/59926

[Alexey Proskuryakov]

I second Darin's concern - the "after the unload event has fired, so no new Geolocation activity is possible" explanation seems weak. If one manages to start a Geolocation after stop(), then we have a security bug due to accessing deallocated objects.

We should revert this change unless there is a strong guarantee that this can't happen.

> However, the Frame may not be destroyed if the Geolocation object still holds
> references to JS callback functions, as these create circular references.

What exactly creates the circular reference? I think that the proper fix would be to avoid having those - disconnectFrame() makes referencing the frame from geolocation code unnecessary.

[Alexey Proskuryakov]

Even though I don't have a specific test case where this fails, I think that this is a change in a wrong direction. A more solid fix needn't build upon it, and it may even end up being simpler.

I'm going to revert this change tomorrow, unless there are objections.

[Steve Block]

> If one manages to start a Geolocation after stop(), then we have a security bug
> due to accessing deallocated objects.
I don't think there's any danger of that. Even after stop() has been called and all ongoing requests have been killed, later calls to startRequest() will happily start new requests.

> What exactly creates the circular reference? I think that the proper fix would
> be to avoid having those
On Android I find that if Geolocation requests are ongoing when the tab is closed, away the Frame is not deleted, so disconnectFrame() is never called. I think this is due to the fact that the Geolocation object holds references to script callbacks and script holds references to the page. You might be right that the correct fix is to avoid these circular refs and to rely on disconnectFrame(). Since the current patch is at least a partial fix and shouldn't introduce any further problems, can't we leave it in until we find a complete fix?

[Alexey Proskuryakov]

> I don't think there's any danger of that. Even after stop() has been called
> and all ongoing requests have been killed, later calls to startRequest() will
> happily start new requests.

That's exactly what concerns me. These requests won't be stopped, because FrameLoader::stop() won't be called again. So, the problem that was fixed by r59859 can be re-introduced.

Anyway, I found that
1) Geolocation is completely broken in ToT WebKit now (bug 39434);
2) Geolocation regression tests are all disabled on Mac and Windows.

I'm going to work on these issues first.

[Steve Block]

> That's exactly what concerns me. These requests won't be stopped, because FrameLoader::stop() won't be called again.
> So, the problem that was fixed by r59859 can be re-introduced.
It's true that a crash is possible, but I don't think this is introduced by this patch. I don't think the fix in r59859 fully solved
the problem. If I understand things correctly, stopping ongoing requests in disconnectFrame() isn't sufficient because
Geolocation activity can be started after this point by code in same-origin frames. I think that as well as stopping ongoing
requests, we would need to make sure that no new requests are started after disconnectFrame() has been called.

After this patch, we suffer from the same problem, but the critical point is now stop() rather than disconnectFrame().

Does that make sense? Apologies if not, I'm new to the loader code.

[Steve Block]

> I don't think the fix in r59859 fully solved
> the problem. If I understand things correctly, stopping ongoing requests in disconnectFrame() isn't sufficient because
> Geolocation activity can be started after this point by code in same-origin frames. I think that as well as stopping ongoing
> requests, we would need to make sure that no new requests are started after disconnectFrame() has been called.
I've filed Bug 39879 to track this.

I'm still looking into the details of the circular reference problems to see if this patch can be reverted completely, or whether it can be adapted to clear the ongoing requests in disconnectFrame() rather than in stop(). I'll continue to update the bug with my progress, but if you feel that you must revert this patch now, please go ahead. Please CC on me on any related bugs you file.

[Steve Block]

Rolled back in http://trac.webkit.org/changeset/60441

Reopening bug

[Steve Block]

The underlying problem has been fixed with other recent changes to Geolocation - probably http://trac.webkit.org/changeset/65416