Bug 52216

Created attachment 78535 [details] Patch

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

Comment on attachment 78535 [details] Patch How do we test this?

Created attachment 78695 [details] Patch

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

Created attachment 78810 [details] Patch

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?

Created attachment 78936 [details] Patch

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 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 on attachment 78936 [details] Patch Clearing flags on attachment: 78936 Committed r75934: <http://trac.webkit.org/changeset/75934>

All reviewed patches have been landed. Closing bug.