[Webkit-unassigned] [Bug 39879] Geolocation activity started after frame has been disconnected can cause crash

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=39879


Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #58148|review?                     |review+
               Flag|                            |




--- Comment #33 from Alexey Proskuryakov <ap at webkit.org>  2010-06-08 16:28:15 PST ---
(From update of attachment 58148)
+static const char serviceNoLongerAvailableErrorMessage[] = "Geolocation service no longer available";

This message is unlikely to tell anything to a developer - I think I'd need to search WebKit sources to figure out what it is about. Would something like "Geolocation cannot be used in frameless documents" be appropriate?

-void Geolocation::Watchers::getNotifiersVector(Vector<RefPtr<GeoNotifier> >& copy) const
+void Geolocation::Watchers::copyNotifiersToVector(Vector<RefPtr<GeoNotifier> >& copy) const

The new name is slightly misleading by saying that notifiers are copied, which isn't true. But I don't feel very strongly about this any more.

-    stopTimers();
+    cancelAllRequests();

My understanding is that stopTimers() isn't needed because all timers are stopped by cancelAllRequests(). Is that accurate? Can we assert that after cancelAllRequests()? That's something that would be good to have explained in ChangeLog.

+    else if (!m_frame)
+        notifier->setFatalError(PositionError::create(PositionError::POSITION_UNAVAILABLE, serviceNoLongerAvailableErrorMessage));

Will the fatal error stay set if a document returns from b/f cache? I'm worried about the following scenario:
1. a request is attempted on a document in b/f cache;
2. user navigates back, so the document is in frame again;
3. Geolocation still doesn't work.

Note that even the fact that Geolocation prevents documents from going into b/f cache doesn't necessarily prevent this scenario. Step 1 occurs when a document is already in b/f cache. And of course, that limitation might be lifted one day.

I'm still not very familiar with Geolocation code, so I'm not sure what GeoNotifiers are. But even if there is no problem, it seems that adding a test would be useful. The two "fatal errors" are different in that "permission denied" stays forever, but "no frame" can be a temporary condition.

+    static void cancelRequests(Vector<RefPtr<GeoNotifier> >&);

This doesn't look like it needs to be a member function. We try to have as little as possible in headers to avoid unnecessary rebuilds when something changes.

It is difficult for me to navigate these tests, as there are both boilerplate resources and actual subresources - I think that this is a case where writing tests by hand would be preferable.

+        Tests: fast/dom/Geolocation/disconnected-frame-already.html
+               fast/dom/Geolocation/disconnected-frame.html

This doesn't mention the third new test (permission-denied).

I'm going to say r+, but please consider fixing as many of these comments as you see fit. You can choose to request review again if the changes are significant.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.