Bug 52216

Summary: GeolocationController should call stopUpdating on destruction
Product: WebKit Reporter: John Knottenbelt <jknotten>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: andreip, commit-queue, jorlow, sam, steveblock
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description John Knottenbelt 2011-01-11 08:46:11 PST
The GeolocationController calls stopUpdating on its client when the last observer (Geolocation object) removes itself. However, it is possible for the Geolocation objects to survive the controller. This is because the GeolocationController is owned by Page, and the Geolocation objects are owned by Frame (indirectly, via DOMWindow, Navigator). http://code.google.com/p/chromium/issues/detail?id=69069 shows a situation where the Page is destroyed, but the Frame is not destroyed because its reference count does not fall to 0. 

If the client is sending position updates, we should tell it to to stop when the GeolocationController is destroyed.
Comment 1 John Knottenbelt 2011-01-11 08:49:43 PST
Created attachment 78535 [details]
Patch
Comment 2 Andrei Popescu 2011-01-11 10:09:30 PST
I think you should add a layout test for this. Would creating a watch and immediately closing the window do the trick?
Comment 3 Eric Seidel (no email) 2011-01-11 11:50:59 PST
Comment on attachment 78535 [details]
Patch

How do we test this?
Comment 4 John Knottenbelt 2011-01-12 08:28:50 PST
Created attachment 78695 [details]
Patch
Comment 5 John Knottenbelt 2011-01-12 08:45:10 PST
(In reply to comment #4)
> Created an attachment (id=78695) [details]
> Patch

The patch adds a layout test to ensure that the corresponding assertion in the WebCore::GeolocationClientMock is executed. The test creates a geolocation watch in a separate window, and then closes that window and waits for the close to complete. If the watch is still updating at the time GeolocationClientMock::geolocationDestroyed() is invoked, the assertion will fail. 

The shutdown code in DumpRenderTree WebViewHost::~WebViewHost includes a navigation to "about:blank" before invoking WebWidget::close. This ensures that the watches are detached (and consequently the GeolocationClient will stop updating) before the GeolocationController is destroyed. 

Without the navigation to about:blank, the test crashes on the assertion, analogously to http://code.google.com/p/chromium/issues/detail?id=69069. 

The test is therefore adding coverage to ensure that future changes to WebKit maintains the assertion that the GeolocationClient is not updating when the GeolocationController is destroyed.
Comment 6 Jeremy Orlow 2011-01-12 09:28:54 PST
Comment on attachment 78695 [details]
Patch

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

> LayoutTests/fast/dom/Geolocation/script-tests/window-close-crash.js:12
> +    debug('This test can not be run without the LayoutTestController');

testFailed

> LayoutTests/fast/dom/Geolocation/script-tests/window-close-crash.js:15
> +  debug("Received Geoposition.");

testPassed

> LayoutTests/fast/dom/Geolocation/script-tests/window-close-crash.js:17
> +  window.setTimeout(waitForWindowToClose, 1);

Why 1?   Only use 0 to keep things more deterministic.

> LayoutTests/fast/dom/Geolocation/script-tests/window-close-crash.js:22
> +    window.setTimeout(waitForWindowToClose, 1);

ditto

> LayoutTests/fast/dom/Geolocation/script-tests/window-close-crash.js:25
> +  debug("Success - no crash!");

testPassed

> LayoutTests/fast/dom/Geolocation/script-tests/window-close-crash.js:30
> +  debug("Failed to create watch: " + e);

Use the function that prints it out in red text and suhc.....testFialed maybe?

> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:1545
> +        windowList[i]->geolocationClientMock()->setError(arguments[0].toInt32(), cppVariantToWebString(arguments[1]));

Will this work as expected?  For example, if one window sets one thing and another sets another thing, only the last one wins.

Should this be a per-window thing?  Can it be?
Comment 7 John Knottenbelt 2011-01-13 08:15:49 PST
Created attachment 78810 [details]
Patch
Comment 8 Jeremy Orlow 2011-01-14 03:46:21 PST
Comment on attachment 78810 [details]
Patch

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

> LayoutTests/fast/dom/Geolocation/script-tests/window-close-crash.js:30
> +  testFailed("Failed to create watch: " + e);

Use webkit style in this file

> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:1526
> +    for (size_t i = 0; i < windowList.size(); i++)

Do these changes need to be made to other DRTs?
Comment 9 John Knottenbelt 2011-01-14 06:14:50 PST
Created attachment 78936 [details]
Patch
Comment 10 Jeremy Orlow 2011-01-14 06:19:24 PST
Comment on attachment 78936 [details]
Patch

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

r=me

> LayoutTests/platform/gtk/Skipped:4919
> +fast/dom/Geolocation/window-close-crash.html

Just skip the whole directory

> LayoutTests/platform/mac-wk2/Skipped:1578
> +fast/dom/Geolocation/window-close-crash.html

Just skip the whole directory

> LayoutTests/platform/qt-wk2/Skipped:1740
> +fast/dom/Geolocation/window-close-crash.html

Just skip the whole directory
Comment 11 WebKit Review Bot 2011-01-17 01:45:57 PST
Comment on attachment 78936 [details]
Patch

Rejecting attachment 78936 [details] from commit-queue.

jknotten@chromium.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 12 WebKit Commit Bot 2011-01-17 04:22:18 PST
Comment on attachment 78936 [details]
Patch

Clearing flags on attachment: 78936

Committed r75934: <http://trac.webkit.org/changeset/75934>
Comment 13 WebKit Commit Bot 2011-01-17 04:22:23 PST
All reviewed patches have been landed.  Closing bug.