Fix cache corruption in incremental resolver

Fix cache corruption in incremental resolver

- Add an element Modifier indicating that it's a cache key.
- Don't allow the hashCode to change while it's in the cache.
- Add code in the incremental resolver to remove the cache entry
first if the hashCode will change.

Also, there appears to be a memory leak in AnalysisCache where
CacheEntry.dispose() was not usually called, so I also fixed
that.

BUG=
R=brianwilkerson@google.com

Committed: https://github.com/dart-lang/sdk/commit/6715573c6eb5e5a3c2b8257fef02e88a7d707d9f

[skybrian, 2016-02-16 07:11:06]

https://codereview.chromium.org/1696193003/diff/1/pkg/analyzer/lib/src/contex...
File pkg/analyzer/lib/src/context/cache.dart (right):

https://codereview.chromium.org/1696193003/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/cache.dart:321: (target as
ElementImpl).setModifier(Modifier.CACHE_KEY, true);
Since target is a field, the extra cast is necessary to avoid a warning.

[skybrian, 2016-02-16 07:12:19]

skybrian@google.com changed reviewers:
+ brianwilkerson@google.com, scheglov@google.com

[skybrian, 2016-02-16 07:12:19]

This is not ready to check in but what do you think of the approach?

[Brian Wilkerson, 2016-02-16 15:10:48]

The approach seems reasonable to me.

https://codereview.chromium.org/1696193003/diff/1/pkg/analyzer/lib/src/contex...
File pkg/analyzer/lib/src/context/cache.dart (right):

https://codereview.chromium.org/1696193003/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/cache.dart:244: var entry =
partition.remove(target);
Missing type

https://codereview.chromium.org/1696193003/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/cache.dart:248:
target.setModifier(Modifier.CACHE_KEY, false);
Won't `dispose` have already cleared the flag?

https://codereview.chromium.org/1696193003/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/cache.dart:600: var entry =
_partition.entryMap.remove(target);
Missing type

https://codereview.chromium.org/1696193003/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/incremental_resolution_validator.dart
(right):

https://codereview.chromium.org/1696193003/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/incremental_resolution_validator.dart:38: AstNode
other; // expected
nit: the comment should be a dartdoc

https://codereview.chromium.org/1696193003/diff/1/pkg/analyzer/test/generated...
File pkg/analyzer/test/generated/incremental_resolver_test.dart (right):

https://codereview.chromium.org/1696193003/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/incremental_resolver_test.dart:5306: lastError =
exception;
Seems like it would be useful to also capture the stack trace so that we could
include it in the failure message. Also, perhaps replace `hasError` with an
`expectNoErrors` method that calls `fail` directly because that would ensure
that we're always including the extra debugging information.

[skybrian, 2016-02-17 01:23:41]

https://codereview.chromium.org/1696193003/diff/1/pkg/analyzer/lib/src/contex...
File pkg/analyzer/lib/src/context/cache.dart (right):

https://codereview.chromium.org/1696193003/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/cache.dart:244: var entry =
partition.remove(target);
On 2016/02/16 15:10:48, Brian Wilkerson wrote:
> Missing type

Done.

https://codereview.chromium.org/1696193003/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/cache.dart:248:
target.setModifier(Modifier.CACHE_KEY, false);
On 2016/02/16 15:10:48, Brian Wilkerson wrote:
> Won't `dispose` have already cleared the flag?

Fixed. (Yes, this was left over from debugging.)

https://codereview.chromium.org/1696193003/diff/1/pkg/analyzer/lib/src/contex...
pkg/analyzer/lib/src/context/cache.dart:600: var entry =
_partition.entryMap.remove(target);
On 2016/02/16 15:10:48, Brian Wilkerson wrote:
> Missing type

Done.

https://codereview.chromium.org/1696193003/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/incremental_resolution_validator.dart
(right):

https://codereview.chromium.org/1696193003/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/incremental_resolution_validator.dart:38: AstNode
other; // expected
On 2016/02/16 15:10:48, Brian Wilkerson wrote:
> nit: the comment should be a dartdoc

Done. (Actually the variable should be renamed, but I won't do that now.)

https://codereview.chromium.org/1696193003/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/incremental_resolver.dart (right):

https://codereview.chromium.org/1696193003/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/incremental_resolver.dart:2077: } on StateError {
StateError is definitely not the right exception, because Errors should not
normally be caught [1].

Alternatives are to create a new exception (perhaps FrozenHashCodeException) or
avoid using an exception here.

We want to avoid clearing the cache entry if at all possible. I don't know how
to do that without actually calculating the new hashCode to see if it's
different. So perhaps I could add an uncachedHashCode property. This would
require subclasses to override uncachedHashCode instead of hashCode. I'm not
sure everyone will remember to do that. Also it's tricky since we would want to
use the uncached location as well.

So, I think introducing FrozenHashCodeException and sticking with this approach
is simplest.

[1]
https://www.dartlang.org/effective-dart/usage/#dont-explicitly-catch-error-or...

https://codereview.chromium.org/1696193003/diff/1/pkg/analyzer/test/generated...
File pkg/analyzer/test/generated/incremental_resolver_test.dart (right):

https://codereview.chromium.org/1696193003/diff/1/pkg/analyzer/test/generated...
pkg/analyzer/test/generated/incremental_resolver_test.dart:5306: lastError =
exception;
On 2016/02/16 15:10:48, Brian Wilkerson wrote:
> Seems like it would be useful to also capture the stack trace so that we could
> include it in the failure message. Also, perhaps replace `hasError` with an
> `expectNoErrors` method that calls `fail` directly because that would ensure
> that we're always including the extra debugging information.

Done.

[skybrian, 2016-02-17 02:03:39]

Would you please take another look?

[scheglov, 2016-02-17 02:34:26]

https://codereview.chromium.org/1696193003/diff/20001/pkg/analyzer/lib/src/co...
File pkg/analyzer/lib/src/context/cache.dart (right):

https://codereview.chromium.org/1696193003/diff/20001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/context/cache.dart:11: show ElementImpl, Modifier;
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]

https://codereview.chromium.org/1696193003/diff/20001/pkg/analyzer/lib/src/co...
File pkg/analyzer/lib/src/context/cache.dart (right):

https://codereview.chromium.org/1696193003/diff/20001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/context/cache.dart:11: show ElementImpl, Modifier;
On 2016/02/17 02:34:25, scheglov wrote:
> 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.

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]

https://codereview.chromium.org/1696193003/diff/20001/pkg/analyzer/lib/src/co...
File pkg/analyzer/lib/src/context/cache.dart (right):

https://codereview.chromium.org/1696193003/diff/20001/pkg/analyzer/lib/src/co...
pkg/analyzer/lib/src/context/cache.dart:11: show ElementImpl, Modifier;
> I consider this a temporary fix.

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.

[skybrian, 2016-02-17 18:15:05]

Should I spend time looking into another solution (which will probably involve
refactoring), or is this good enough for now?

[Brian Wilkerson, 2016-02-17 22:51:45]

I think the preferred approach will probably take too long and we should go
ahead and land this CL. So, LGTM.

[skybrian, 2016-02-19 02:29:45]

Description was changed from

==========
Fix cache corruption in incremental resolver

- Add an element Modifier indicating that it's a cache key.
- Don't allow the hashCode to change while it's in the cache.
- Add code in the incremental resolver to remove the cache entry
first if the hashCode will change.

Also, there appears to be a memory leak in AnalysisCache where
CacheEntry.dispose() was not usually called, so I also fixed
that.

BUG=
==========

to

==========
Fix cache corruption in incremental resolver

- Add an element Modifier indicating that it's a cache key.
- Don't allow the hashCode to change while it's in the cache.
- Add code in the incremental resolver to remove the cache entry
first if the hashCode will change.

Also, there appears to be a memory leak in AnalysisCache where
CacheEntry.dispose() was not usually called, so I also fixed
that.

BUG=
R=brianwilkerson@google.com

Committed:
https://github.com/dart-lang/sdk/commit/6715573c6eb5e5a3c2b8257fef02e88a7d707d9f
==========

[skybrian, 2016-02-19 02:29:47]

Committed patchset #2 (id:20001) manually as
6715573c6eb5e5a3c2b8257fef02e88a7d707d9f (presubmit successful).