v8binding: Fixes Geolocation to hold wrapper-tracing objects during the use.

Geolocation::HandleError discards TraceWrapperMembers (notifiers)
in the middle of their use, so wrapper-tracing stops working.
If V8 GC runs while the callbacks are running, as
TraceWrapperMembers are already cleared, the underlying V8 objects
will be gone.

This patch fixes the issue making Geolocation hold
TraceWrapperMembers during the invocation of the callbacks.

Bug: 792604
Change-Id: I6eedc5bc3d82806b1ab0ee2cf2fc3d7a8a018e14
Reviewed-on: https://chromium-review.googlesource.com/824322Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Hitoshi Yoshida <peria@chromium.org>
Commit-Queue: Yuki Shiino <yukishiino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525283}

third_party/WebKit/Source/modules/geolocation/Geolocation.cpp

@@ -114,6 +114,8 @@ Geolocation::~Geolocation() {
 void Geolocation::Trace(blink::Visitor* visitor) {
   visitor->Trace(one_shots_);
   visitor->Trace(watchers_);
+  visitor->Trace(one_shots_being_invoked_);
+  visitor->Trace(watchers_being_invoked_);
   visitor->Trace(last_position_);
   ScriptWrappable::Trace(visitor);
   ContextLifecycleObserver::Trace(visitor);
@@ -124,6 +126,9 @@ void Geolocation::TraceWrappers(const ScriptWrappableVisitor* visitor) const {
   for (const auto& one_shot : one_shots_)
     visitor->TraceWrappers(one_shot);
   visitor->TraceWrappers(watchers_);
+  for (const auto& one_shot : one_shots_being_invoked_)
+    visitor->TraceWrappers(one_shot);
+  visitor->TraceWrappers(watchers_being_invoked_);
   ScriptWrappable::TraceWrappers(visitor);
 }
 
@@ -397,10 +402,10 @@ void Geolocation::HandleError(PositionError* error) {
   // added by calls to Geolocation methods from the callbacks, and to prevent
   // further callbacks to these notifiers.
   GeoNotifierVector one_shots_with_cached_position;
-  one_shots_.clear();
-  if (error->IsFatal())
-    watchers_.Clear();
-  else {
+  swap(one_shots_, one_shots_being_invoked_);
+  if (error->IsFatal()) {
+    swap(watchers_, watchers_being_invoked_);
+  } else {
     // Don't send non-fatal errors to notifiers due to receive a cached
     // position.
     ExtractNotifiersWithCachedPosition(one_shots_copy,
@@ -419,6 +424,9 @@ void Geolocation::HandleError(PositionError* error) {
 
   // Maintain a reference to the cached notifiers until their timer fires.
   CopyToSet(one_shots_with_cached_position, one_shots_);
+
+  one_shots_being_invoked_.clear();
+  watchers_being_invoked_.Clear();
 }
 
 void Geolocation::MakeSuccessCallbacks() {
@@ -433,13 +441,15 @@ void Geolocation::MakeSuccessCallbacks() {
   // Clear the lists before we make the callbacks, to avoid clearing notifiers
   // added by calls to Geolocation methods from the callbacks, and to prevent
   // further callbacks to these notifiers.
-  one_shots_.clear();
+  swap(one_shots_, one_shots_being_invoked_);
 
   SendPosition(one_shots_copy, last_position_);
   SendPosition(watchers_copy, last_position_);
 
   if (!HasListeners())
     StopUpdating();
+
+  one_shots_being_invoked_.clear();
 }
 
 void Geolocation::PositionChanged() {

third_party/WebKit/Source/modules/geolocation/Geolocation.h

@@ -168,6 +168,20 @@ class MODULES_EXPORT Geolocation final : public ScriptWrappable,
 
   GeoNotifierSet one_shots_;
   GeolocationWatchers watchers_;
+  // GeoNotifiers that may be scheduled to be invoked but need to be removed
+  // from |one_shots_| and |watchers_|.
+  //
+  // |HandleError(error)| and |MakeSuccessCallbacks| need to clear |one_shots_|
+  // (and optionally |watchers_|) before invoking the callbacks, in order to
+  // avoid clearing notifiers added by calls to Geolocation methods from the
+  // callbacks. Thus, something else needs to make the notifiers being invoked
+  // alive with wrapper-tracing because V8 GC may run during the callbacks.
+  // |one_shots_being_invoked_| and |watchers_being_invoked_| perform
+  // wrapper-tracing.
+  // TODO(https://crbug.com/796145): Remove this hack once on-stack objects
+  // get supported by either of wrapper-tracing or unified GC.
+  GeoNotifierSet one_shots_being_invoked_;
+  GeolocationWatchers watchers_being_invoked_;
   Member<Geoposition> last_position_;
 
   device::mojom::blink::GeolocationPtr geolocation_;

third_party/WebKit/Source/modules/geolocation/GeolocationWatchers.cpp

@@ -69,6 +69,11 @@ bool GeolocationWatchers::IsEmpty() const {
   return id_to_notifier_map_.IsEmpty();
 }
 
+void GeolocationWatchers::Swap(GeolocationWatchers& other) {
+  swap(id_to_notifier_map_, other.id_to_notifier_map_);
+  swap(notifier_to_id_map_, other.notifier_to_id_map_);
+}
+
 void GeolocationWatchers::GetNotifiersVector(
     HeapVector<Member<GeoNotifier>>& copy) const {
   CopyValuesToVector(id_to_notifier_map_, copy);

third_party/WebKit/Source/modules/geolocation/GeolocationWatchers.h

@@ -27,6 +27,7 @@ class GeolocationWatchers : public TraceWrapperBase {
   bool Contains(GeoNotifier*) const;
   void Clear();
   bool IsEmpty() const;
+  void Swap(GeolocationWatchers& other);
 
   void GetNotifiersVector(HeapVector<Member<GeoNotifier>>&) const;
 
@@ -38,6 +39,10 @@ class GeolocationWatchers : public TraceWrapperBase {
   NotifierToIdMap notifier_to_id_map_;
 };
 
+inline void swap(GeolocationWatchers& a, GeolocationWatchers& b) {
+  a.Swap(b);
+}
+
 }  // namespace blink
 
 #endif  // GeolocationWatchers_h