SSIA
Created attachment 88606 [details] Patch
Comment on attachment 88606 [details] Patch This seems correct to me. If we are removing the notifiers from the oneShots and watchers list, it also makes sense to remove them from the other lists too.
Comment on attachment 88606 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88606&action=review > Source/WebCore/ChangeLog:11 > + In fatalErrorOccurred (which is called on cancellation), the notifier What do you mean by 'cancellation'? Can you elaborate in the bug exactly what the problem is? Presumably it's not unique to google.com? > Source/WebCore/ChangeLog:18 > + Can you add a test for this? If not, you should explain here why not. > Source/WebCore/page/Geolocation.cpp:342 > + m_requestsAwaitingCachedPosition.remove(notifier); I think that the only way for a notifier to be in this list when it encounters a fatal error is when Geolocation::reset() is called. Is this what you're protecting against?
(In reply to comment #3) > (From update of attachment 88606 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=88606&action=review > > > Source/WebCore/ChangeLog:11 > > + In fatalErrorOccurred (which is called on cancellation), the notifier > > What do you mean by 'cancellation'? Can you elaborate in the bug exactly what the problem is? Presumably it's not unique to google.com? Reloading the page or loading another one, leaks document() probably due to event holding a ref to document (presumable TargetEvent). It probably happens on other pages as well (using watchPosition?), but I didn't manage to trigger the leak with my own simple tests, and debugging what exactly is happening on google.com is pretty hard due to the obscured javascript code. > > > Source/WebCore/ChangeLog:18 > > + > > Can you add a test for this? If not, you should explain here why not. The test can only work for ports using the PREEMPT_... build flag, and I am not sure now to actually test this, so any help is appreciated. Testing it is hard due to the reloading, plus the fact that I cannot AFAIK count the document references from LayoutTestController. > > Source/WebCore/page/Geolocation.cpp:342 > > + m_requestsAwaitingCachedPosition.remove(notifier); > > I think that the only way for a notifier to be in this list when it encounters a fatal error is when Geolocation::reset() is called. Is this what you're protecting against? I added this one for completions sake. It can be left out.
Comment on attachment 88606 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88606&action=review >>> Source/WebCore/ChangeLog:11 >>> + In fatalErrorOccurred (which is called on cancellation), the notifier >> >> What do you mean by 'cancellation'? Can you elaborate in the bug exactly what the problem is? Presumably it's not unique to google.com? > > Reloading the page or loading another one, leaks document() probably due to event holding a ref to document (presumable TargetEvent). > > It probably happens on other pages as well (using watchPosition?), but I didn't manage to trigger the leak with my own simple tests, and debugging what exactly is happening on google.com is pretty hard due to the obscured javascript code. Do you see this only on Chromium? I think it would be good to understand a little more about the problem before we make the fix. The Geolocation callbacks don't ref their ScriptExecutionContext, so I don't see how removing this notifier from this list will avoid a leak. Presumably the Geolocation object is leaked too?
> Do you see this only on Chromium? I think it would be good to understand a little more about the problem before we make the fix. The Geolocation callbacks don't ref their ScriptExecutionContext, so I don't see how removing this notifier from this list will avoid a leak. Presumably the Geolocation object is leaked too? This happens on our Qt WebKit2 port (a branch on gitorious). It was pretty hard to debug because Document is ref'ed for each event, but adding this plugged the leak.
Comment on attachment 88606 [details] Patch We need to find some way to test this. Manual tests are fine.
Comment on attachment 88606 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88606&action=review A test to count the number of notifiers left in the lists after page reload should be possible. However, exposing this count seems only useful for this particular test case. Is there a way to add this test without complicating the interfaces of the Geolocation, GeolocationController, GeolocationClient and embedder-specific client classes? >>> Source/WebCore/page/Geolocation.cpp:342 >>> + m_requestsAwaitingCachedPosition.remove(notifier); >> >> I think that the only way for a notifier to be in this list when it encounters a fatal error is when Geolocation::reset() is called. Is this what you're protecting against? > > I added this one for completions sake. It can be left out. Steve: Yes, we can get a build up of notifiers in this case, since the main Frame (and therefore Geolocation instance) is reused across page reloads.
fast/dom/Geolocation/page-reload-cancel-permission-requests.html is a related test that tests that geolocation permission requests are properly cancelled at the GeolocationClient on page reload. (In reply to comment #8) > (From update of attachment 88606 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=88606&action=review > > A test to count the number of notifiers left in the lists after page reload should be possible. However, exposing this count seems only useful for this particular test case. Is there a way to add this test without complicating the interfaces of the Geolocation, GeolocationController, GeolocationClient and embedder-specific client classes? > > >>> Source/WebCore/page/Geolocation.cpp:342 > >>> + m_requestsAwaitingCachedPosition.remove(notifier); > >> > >> I think that the only way for a notifier to be in this list when it encounters a fatal error is when Geolocation::reset() is called. Is this what you're protecting against? > > > > I added this one for completions sake. It can be left out. > > Steve: Yes, we can get a build up of notifiers in this case, since the main Frame (and therefore Geolocation instance) is reused across page reloads.
I've noticed the same issue. I've tested only with Chromium so not sure if it happens with other browsers as well. FWIW, I found adding the following to the Geolocation::reset() method works as well. #if USE(PREEMPT_GEOLOCATION_PERMISSION) m_pendingForPermissionNotifiers.remove(notifier); #endif
I can also confirm that the leaking of Document was due to these pending GeoNotifier objects. In chromium these were creating a persistent v8 handle which weren't released properly (as these were still alive when the page closed) caused the whole DOM to be leaked when the page closed.
Sorry, I should've mentioned that I used this code in the reset() method: #if USE(PREEMPT_GEOLOCATION_PERMISSION) m_pendingForPermissionNotifiers.clear(); #endif
I think we should also be removing the watch from m_pendingForPermissionNotifiers in Geolocation::clearWatch to avoid potential build up of watches by repeated calls to watchPosition and clearWatch.
Created attachment 111130 [details] Patch
Comment on attachment 111130 [details] Patch Nice, but can you implement John's suggestion as well? It could be a follow up.
Comment on attachment 111130 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111130&action=review > Source/WebCore/page/Geolocation.cpp:344 > +#if USE(PREEMPT_GEOLOCATION_PERMISSION) The change to Geolocation::reset() is enough - m_pendingForPermissionNotifiers is cleared in Geolocation::setIsAllowed. All other calls to setFatalError are called after permission has been determined, so the notifier won't be in this list. It would be good to move these lines to Geolocation::clearWatch(), though, to avoid a potential build up of watches in this set.
Comment on attachment 111130 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111130&action=review >> Source/WebCore/page/Geolocation.cpp:344 >> +#if USE(PREEMPT_GEOLOCATION_PERMISSION) > > The change to Geolocation::reset() is enough - m_pendingForPermissionNotifiers is cleared in Geolocation::setIsAllowed. All other calls to setFatalError are called after permission has been determined, so the notifier won't be in this list. > > It would be good to move these lines to Geolocation::clearWatch(), though, to avoid a potential build up of watches in this set. Will do.
Created attachment 111178 [details] Patch
Comment on attachment 111178 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111178&action=review > Source/WebCore/page/Geolocation.cpp:178 > +Geolocation::GeoNotifier* Geolocation::Watchers::get(int id) WebKit style would be to call this method for "find"
Comment on attachment 111178 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111178&action=review > Source/WebCore/page/Geolocation.cpp:436 > + GeoNotifier* notifier = m_watchers.get(watchId); > + if (notifier) > + m_pendingForPermissionNotifiers.remove(notifier); You could change this into: if (GeoNotifier* notifier = m_watchers.find(watchId)) m_pendingForPermissionNotifiers.remove(notifier);
> > The change to Geolocation::reset() is enough - m_pendingForPermissionNotifiers is cleared in Geolocation::setIsAllowed. I think it would be better do document such behavior using ASSERTs if possible.
Comment on attachment 111178 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111178&action=review >> Source/WebCore/page/Geolocation.cpp:178 >> +Geolocation::GeoNotifier* Geolocation::Watchers::get(int id) > > WebKit style would be to call this method for "find" Will do >> Source/WebCore/page/Geolocation.cpp:436 >> + m_pendingForPermissionNotifiers.remove(notifier); > > You could change this into: > > if (GeoNotifier* notifier = m_watchers.find(watchId)) > m_pendingForPermissionNotifiers.remove(notifier); Will do
Created attachment 111186 [details] Patch
Comment on attachment 111186 [details] Patch John are you OK with this?
(In reply to comment #24) > (From update of attachment 111186 [details]) > John are you OK with this? Looks good to me. thanks for reviewing!
Comment on attachment 111186 [details] Patch Clearing flags on attachment: 111186 Committed r97594: <http://trac.webkit.org/changeset/97594>
All reviewed patches have been landed. Closing bug.
Comment on attachment 111186 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111186&action=review > Source/WebCore/page/Geolocation.h:116 > + GeoNotifier* find(int id); Do we need this new method? All we do with the GeoNotifier returned by this method is pass it to remove(GeoNotifier*), but Watchers already offers a remove(int id) method, which achieves the same thing.
Comment on attachment 111186 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111186&action=review >> Source/WebCore/page/Geolocation.h:116 >> + GeoNotifier* find(int id); > > Do we need this new method? All we do with the GeoNotifier returned by this method is pass it to remove(GeoNotifier*), but Watchers already offers a remove(int id) method, which achieves the same thing. You're right, I don't know how I missed that :/ Will upload a patch to remove this method and use remove(int id) instead.
Comment on attachment 111186 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111186&action=review >>> Source/WebCore/page/Geolocation.h:116 >>> + GeoNotifier* find(int id); >> >> Do we need this new method? All we do with the GeoNotifier returned by this method is pass it to remove(GeoNotifier*), but Watchers already offers a remove(int id) method, which achieves the same thing. > > You're right, I don't know how I missed that :/ Will upload a patch to remove this method and use remove(int id) instead. m_pendingForPermissionNotifiers is not part of m_watchers. So calling Watchers::remove(int id) can't remove this pending item. So leaving it as is.
>>>> Source/WebCore/page/Geolocation.cpp:342 >>>> + m_requestsAwaitingCachedPosition.remove(notifier); >>> >>> I think that the only way for a notifier to be in this list when it encounters a fatal error is when Geolocation::reset() is called. Is this what you're protecting against? >> I added this one for completions sake. It can be left out. > Steve: Yes, we can get a build up of notifiers in this case, since the main Frame (and therefore Geolocation instance) is reused across page reloads. John, are you still concerned about this? How exactly do we handle cancelling regular requests in this case? It looks like the only time we do any cancelling of requests is when reset() or disconectFrame() is called. Is either of these called in this case? If you think there's a problem, let's open a new bug, preferably with a test case.
Steve, are you asking about my earlier proposed change to Geolocation::fatalErrorOccurred? If so, I think it's not necessary to make any changes there now as they will be cleared up in ::reset() with Satish's change. (In reply to comment #31) > >>>> Source/WebCore/page/Geolocation.cpp:342 > >>>> + m_requestsAwaitingCachedPosition.remove(notifier); > >>> > >>> I think that the only way for a notifier to be in this list when it encounters a fatal error is when Geolocation::reset() is called. Is this what you're protecting against? > >> I added this one for completions sake. It can be left out. > > Steve: Yes, we can get a build up of notifiers in this case, since the main Frame (and therefore Geolocation instance) is reused across page reloads. > > John, are you still concerned about this? How exactly do we handle cancelling regular requests in this case? It looks like the only time we do any cancelling of requests is when reset() or disconectFrame() is called. Is either of these called in this case? If you think there's a problem, let's open a new bug, preferably with a test case.
Yes, I was talking about your comment about clearing m_requestsAwaitingCachedPosition in fatalErrorOccurred() in the first patch set uploaded to this bug. Satish's patch doesn't touch m_requestsAwaitingCachedPosition, right?
Steve, I'm not sure if there is a problem there now. I've opened https://bugs.webkit.org/show_bug.cgi?id=70461 to track an investigation of the question.