[webkit-reviews] review granted: [Bug 39879] Geolocation activity started after frame has been disconnected can cause crash : [Attachment 58148] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Jeremy Orlow
<jorlow at chromium.org>'s request for review:
Bug 39879: Geolocation activity started after frame has been disconnected can
cause crash
https://bugs.webkit.org/show_bug.cgi?id=39879

Attachment 58148: Patch
https://bugs.webkit.org/attachment.cgi?id=58148&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+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_UNAVAILAB
LE, 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.