Bug 52216 – GeolocationController should call stopUpdating on destruction

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[John Knottenbelt]

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.

[John Knottenbelt]

Created attachment 78535 [details]
Patch

[Andrei Popescu]

I think you should add a layout test for this. Would creating a watch and immediately closing the window do the trick?

[Eric Seidel (no email)]

Comment on attachment 78535 [details]
Patch

How do we test this?

[John Knottenbelt]

Created attachment 78695 [details]
Patch

[John Knottenbelt]

(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.

[Jeremy Orlow]

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?

[John Knottenbelt]

Created attachment 78810 [details]
Patch

[Jeremy Orlow]

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?

[John Knottenbelt]

Created attachment 78936 [details]
Patch

[Jeremy Orlow]

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

[WebKit Review Bot]

Comment on attachment 78936 [details]
Patch

Rejecting attachment 78936 [details] from commit-queue.

[e-mail] 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.

[WebKit Commit Bot]

Comment on attachment 78936 [details]
Patch

Clearing flags on attachment: 78936

Committed r75934: <http://trac.webkit.org/changeset/75934>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.