Fix cache corruption in incremental resolver

pkg/analyzer/lib/src/context/cache.dart

Index: pkg/analyzer/lib/src/context/cache.dart
diff --git a/pkg/analyzer/lib/src/context/cache.dart b/pkg/analyzer/lib/src/context/cache.dart
index afe021626062dab159e58d4ebd91a185f7e631e1..bbb0e698e3acb1add0ae5e9e3952a3eab1b12b99 100644
--- a/pkg/analyzer/lib/src/context/cache.dart
+++ b/pkg/analyzer/lib/src/context/cache.dart
@@ -7,6 +7,8 @@ library analyzer.src.context.cache;
 import 'dart:async';
 import 'dart:collection';
 
+import 'package:analyzer/src/dart/element/element.dart'
+    show ElementImpl, Modifier;

[scheglov, 2016/02/17 02:34:25]

Do we want to introduce Element dependency into cache?
Can we create an interface for objects that have freeze / unfreeze?

Ideally we want stop putting objects that may change their hashCode or == over
time.
I.e. simply ban local VariableElement and local FunctionElement, and implement
corresponding functionality without putting these elements into the cache. I
think that would be better than adding a check that we would have to fix anyway.

[skybrian, 2016/02/17 03:05:57]

To avoid this import statement, an alternative might be to add an isCacheKey
property to AnalysisTarget, and set it for all cache keys. But there are many
implementations of AnalysisTarget and perhaps it's not worth doing temporarily.

I consider this a temporary fix. I don't know the best solution.

The problem is that I don't think we fully understand which subtypes of Element
can used as cache keys. (We missed DefaultParameterElementImpl and there might
be others.) This way ensures that the incremental resolver will always do the
right thing in one spot, but there could be unhandled exceptions somewhere else.

I think it would be good to make Element stop implementing AnalysisTarget so
that we can tell statically which subtypes are used as cache keys. I
experimented in [1] with moving AnalysisTarget to subtypes of Element but it's a
complicated class hierarchy.

Another possibility would be to give each Element subtype that can be used in
the cache a cacheKey property. Generally, using mutable objects as map keys is
something we should avoid. But if an element's cacheKey property changes then it
would still be a memory leak, even if the key itself is immutable.

[1] https://codereview.chromium.org/1657463002/

[Brian Wilkerson, 2016/02/17 04:03:54]

I definitely do not want to add a temporary method to public API.

Other than figuring out what the correct solution is and implementing it, that
seems to imply that we need to build in a temporary dependency on ElementImpl.

 import 'package:analyzer/src/generated/engine.dart';
 import 'package:analyzer/src/generated/java_engine.dart';
 import 'package:analyzer/src/generated/source.dart';
@@ -239,7 +241,11 @@ class AnalysisCache {
           AnalysisEngine.instance.logger
               .logInformation('Removed the cache entry for $target.');
         }
-        return partition.remove(target);
+        CacheEntry entry = partition.remove(target);
+        if (entry != null) {
+          entry.dispose();
+        }
+        return entry;
       }
     }
     return null;
@@ -307,7 +313,11 @@ class CacheEntry {
   Map<ResultDescriptor, ResultData> _resultMap =
       new HashMap<ResultDescriptor, ResultData>();
 
-  CacheEntry(this.target);
+  CacheEntry(this.target) {
+    if (target is ElementImpl) {
+      (target as ElementImpl).setModifier(Modifier.CACHE_KEY, true);
+    }
+  }
 
   /**
    * The exception that caused one or more values to have a state of
@@ -350,6 +360,9 @@ class CacheEntry {
       }
     });
     _resultMap.clear();
+    if (target is ElementImpl) {
+      (target as ElementImpl).setModifier(Modifier.CACHE_KEY, false);
+    }
   }
 
   /**
@@ -384,7 +397,7 @@ class CacheEntry {
    * Return the value of the result represented by the given [descriptor], or
    * the default value for the result if this entry does not have a valid value.
    */
-  dynamic /*=V*/ getValue /*<V>*/ (ResultDescriptor /*<V>*/ descriptor) {
+  dynamic/*=V*/ getValue/*<V>*/(ResultDescriptor/*<V>*/ descriptor) {
     ResultData data = _resultMap[descriptor];
     if (data == null) {
       return descriptor.defaultValue;
@@ -482,8 +495,8 @@ class CacheEntry {
    * Set the value of the result represented by the given [descriptor] to the
    * given [value].
    */
-  void setValue /*<V>*/ (ResultDescriptor /*<V>*/ descriptor,
-      dynamic /*=V*/ value, List<TargetedResult> dependedOn) {
+  void setValue/*<V>*/(ResultDescriptor/*<V>*/ descriptor, dynamic/*=V*/ value,
+      List<TargetedResult> dependedOn) {
 //    {
 //      String valueStr = '$value';
 //      if (valueStr.length > 20) {
@@ -581,7 +594,10 @@ class CacheEntry {
     _invalidateDependentResults(id, thisData, delta, level + 1);
     // If empty and not explicitly added, remove the entry altogether.
     if (_resultMap.isEmpty && !explicitlyAdded) {
-      _partition.entryMap.remove(target);
+      CacheEntry entry = _partition.entryMap.remove(target);
+      if (entry != null) {
+        entry.dispose();
+      }
       _partition._removeIfSource(target);
     }
     // Notify controller.