commit b2d3435d8e03744f4506018abe60255629e3bdb0
author Adam Barth <abarth@chromium.org> Tue Aug 25 14:32:29 2015 -0700
committer Adam Barth <abarth@chromium.org> Tue Aug 25 14:45:18 2015 -0700
tree cf025b3db0a5b92671757bc36b7ecd02326f7688
parent fd3e989c988cef37fab3d6b561f6d7f93c63a3f5

Fix global key error while using fitness app

We were making local copies of the listener maps, but we were actually
iterating the underlying sets. Now we make local copies of the sets.

Fixes #803

sky/packages/sky/lib/widgets/framework.dart

diff --git a/sky/packages/sky/lib/widgets/framework.dart b/sky/packages/sky/lib/widgets/framework.dart
index 1e4ead3..9c4908d 100644
--- a/sky/packages/sky/lib/widgets/framework.dart
+++ b/sky/packages/sky/lib/widgets/framework.dart
@@ -97,10 +97,7 @@
     _syncedKeys.add(this);
   }
 
-  static bool _notifyingListeners = false;
-
   static void registerSyncListener(GlobalKey key, GlobalKeySyncListener listener) {
-    assert(!_notifyingListeners);
     assert(key != null);
     Set<GlobalKeySyncListener> listeners =
         _syncListeners.putIfAbsent(key, () => new Set<GlobalKeySyncListener>());
@@ -109,7 +106,6 @@
   }
 
   static void unregisterSyncListener(GlobalKey key, GlobalKeySyncListener listener) {
-    assert(!_notifyingListeners);
     assert(key != null);
     assert(_syncListeners.containsKey(key));
     bool removed = _syncListeners[key].remove(listener);
@@ -119,7 +115,6 @@
   }
 
   static void registerRemoveListener(GlobalKey key, GlobalKeyRemoveListener listener) {
-    assert(!_notifyingListeners);
     assert(key != null);
     Set<GlobalKeyRemoveListener> listeners =
         _removeListeners.putIfAbsent(key, () => new Set<GlobalKeyRemoveListener>());
@@ -128,7 +123,6 @@
   }
 
   static void unregisterRemoveListener(GlobalKey key, GlobalKeyRemoveListener listener) {
-    assert(!_notifyingListeners);
     assert(key != null);
     assert(_removeListeners.containsKey(key));
     bool removed = _removeListeners[key].remove(listener);
@@ -148,31 +142,27 @@
     assert(_debugDuplicates.isEmpty);
     if (_syncedKeys.isEmpty && _removedKeys.isEmpty)
       return;
-    _notifyingListeners = true;
     try {
-      Map<GlobalKey, Set<GlobalKeyRemoveListener>> localRemoveListeners =
-          new Map<GlobalKey, Set<GlobalKeyRemoveListener>>.from(_removeListeners);
-      Map<GlobalKey, Set<GlobalKeySyncListener>> localSyncListeners =
-          new Map<GlobalKey, Set<GlobalKeySyncListener>>.from(_syncListeners);
 
       for (GlobalKey key in _syncedKeys) {
         Widget widget = _registry[key];
-        if (widget != null && localSyncListeners.containsKey(key)) {
-          for (GlobalKeySyncListener listener in localSyncListeners[key])
+        if (widget != null && _syncListeners.containsKey(key)) {
+          Set<GlobalKeySyncListener> localListeners = new Set<GlobalKeySyncListener>.from(_syncListeners[key]);
+          for (GlobalKeySyncListener listener in localListeners)
             listener(key, widget);
         }
       }
 
       for (GlobalKey key in _removedKeys) {
-        if (!_registry.containsKey(key) && localRemoveListeners.containsKey(key)) {
-          for (GlobalKeyRemoveListener listener in localRemoveListeners[key])
+        if (!_registry.containsKey(key) && _removeListeners.containsKey(key)) {
+          Set<GlobalKeyRemoveListener> localListeners = new Set<GlobalKeyRemoveListener>.from(_removeListeners[key]);
+          for (GlobalKeyRemoveListener listener in localListeners)
             listener(key);
         }
       }
     } finally {
       _removedKeys.clear();
       _syncedKeys.clear();
-      _notifyingListeners = false;
     }
   }
 

sky/unit/test/widget/global_key_test.dart

diff --git a/sky/unit/test/widget/global_key_test.dart b/sky/unit/test/widget/global_key_test.dart
index 18e1406..a410e05 100644
--- a/sky/unit/test/widget/global_key_test.dart
+++ b/sky/unit/test/widget/global_key_test.dart
@@ -106,4 +106,48 @@
     GlobalKey.unregisterSyncListener(globalKey, syncListener);
     GlobalKey.unregisterRemoveListener(globalKey, removeListener);
   });
+
+  test('Global key mutate during iteration', () {
+    GlobalKey globalKey = new GlobalKey();
+
+    bool syncListenerCalled = false;
+    bool removeListenerCalled = false;
+
+    void syncListener(GlobalKey key, Widget widget) {
+      GlobalKey.unregisterSyncListener(globalKey, syncListener);
+      syncListenerCalled = true;
+    }
+
+    void removeListener(GlobalKey key) {
+      GlobalKey.unregisterRemoveListener(globalKey, removeListener);
+      removeListenerCalled = true;
+    }
+
+    GlobalKey.registerSyncListener(globalKey, syncListener);
+    GlobalKey.registerRemoveListener(globalKey, removeListener);
+    WidgetTester tester = new WidgetTester();
+
+    tester.pumpFrame(() {
+      return new Container(key: globalKey);
+    });
+    expect(syncListenerCalled, isTrue);
+    expect(removeListenerCalled, isFalse);
+
+    syncListenerCalled = false;
+    removeListenerCalled = false;
+    tester.pumpFrame(() {
+      return new Container();
+    });
+    expect(syncListenerCalled, isFalse);
+    expect(removeListenerCalled, isTrue);
+
+    syncListenerCalled = false;
+    removeListenerCalled = false;
+    tester.pumpFrame(() {
+      return new Container(key: globalKey);
+    });
+    expect(syncListenerCalled, isFalse);
+    expect(removeListenerCalled, isFalse);
+
+  });
 }