Bug 39288 - Geolocation causes DOMWindow to leak if position requests are in progress when the page is navigated away
Summary: Geolocation causes DOMWindow to leak if position requests are in progress whe...
Status: RESOLVED WORKSFORME
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-18 06:22 PDT by Steve Block
Modified: 2010-09-16 08:18 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.21 KB, patch)
2010-05-18 06:25 PDT, Steve Block
no flags Details | Formatted Diff | Diff
Patch (6.26 KB, patch)
2010-05-19 11:23 PDT, Steve Block
no flags Details | Formatted Diff | Diff
Patch (6.66 KB, patch)
2010-05-20 05:42 PDT, Steve Block
no flags Details | Formatted Diff | Diff
Patch (6.87 KB, patch)
2010-05-20 06:38 PDT, Steve Block
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Steve Block 2010-05-18 06:22:20 PDT
Geolocation causes DOMWindow to leak if position requests are in progress when the page is navigated away
Comment 1 Steve Block 2010-05-18 06:25:24 PDT
Created attachment 56365 [details]
Patch
Comment 2 Darin Adler 2010-05-18 09:20:15 PDT
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
Comment 3 Steve Block 2010-05-19 04:33:49 PDT
Comment on attachment 56365 [details]
Patch

Investigating options for LayoutTests. Will upload an updated patch.
Comment 4 Steve Block 2010-05-19 11:23:29 PDT
Created attachment 56503 [details]
Patch
Comment 5 Steve Block 2010-05-19 11:26:31 PDT
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?
Comment 6 Darin Adler 2010-05-19 11:29:38 PDT
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
Comment 7 Steve Block 2010-05-20 05:40:45 PDT
> 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.
Comment 8 Steve Block 2010-05-20 05:42:13 PDT
Created attachment 56589 [details]
Patch
Comment 9 Early Warning System Bot 2010-05-20 06:34:30 PDT
Attachment 56589 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/2319345
Comment 10 Steve Block 2010-05-20 06:38:54 PDT
Created attachment 56590 [details]
Patch
Comment 11 Darin Adler 2010-05-20 11:31:26 PDT
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
Comment 12 Steve Block 2010-05-24 04:16:30 PDT
Committed r60069: <http://trac.webkit.org/changeset/60069>
Comment 13 Steve Block 2010-05-24 04:25:49 PDT
> 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
Comment 14 Alexey Proskuryakov 2010-05-24 10:39:45 PDT
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.
Comment 15 Alexey Proskuryakov 2010-05-26 14:21:47 PDT
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.
Comment 16 Steve Block 2010-05-27 08:12:08 PDT
> 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?
Comment 17 Alexey Proskuryakov 2010-05-27 13:27:16 PDT
> 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.
Comment 18 Steve Block 2010-05-27 13:59:34 PDT
> 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.
Comment 19 Steve Block 2010-05-28 09:24:29 PDT
> 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.
Comment 20 Steve Block 2010-05-31 09:31:03 PDT
Rolled back in http://trac.webkit.org/changeset/60441

Reopening bug
Comment 21 Steve Block 2010-09-16 06:06:17 PDT
The underlying problem has been fixed with other recent changes to Geolocation - probably http://trac.webkit.org/changeset/65416