Clear cached JS for stale types even when currently unreachable - #10390
Clear cached JS for stale types even when currently unreachable#10390kevinushey wants to merge 3 commits into
Conversation
In incremental compiles, computeAndClearStaleTypesCache() filtered the stale type set down to reachable types before clearing cached JS. A stale type that was unreachable in the previous compile therefore kept its outdated cached JS. If a later compile made the type reachable again, the compiler saw JS present for it (UnifyAst.needsNewJs) and spliced the stale chunk into the output instead of regenerating it. When the type's shape changed in between -- for example synthetic lambda types being renumbered after a lambda was added earlier in the file -- freshly generated call sites reference a newly allocated global name while the stale chunk defines the old one, producing runtime errors like: ReferenceError: Foo$lambda$2$Type_2_g$ is not defined Even when names happen to line up, the output silently runs outdated code. Clear the cached output for the full stale set before the reachability filter; the filter still limits which types are artificially retraversed. This restores the invariant that a cached JS entry is always current, which is exactly what the reachability filter's comment assumed. Symptom reported upstream as gwtproject#9565.
| staleTypeNames.removeAll(JProgram.SYNTHETIC_TYPE_NAMES); | ||
| } | ||
|
|
||
| // Clear the cached output of every stale type, including those that are not currently |
There was a problem hiding this comment.
Maybe the Javadoc for the method should make it clear that the set of types that are removed from cache is not the same as the set returned from this method.
There was a problem hiding this comment.
Good point -- the two sets diverged with this change. Added a paragraph in 756c8a6 noting that cached output is cleared for every stale type while the returned set is narrowed to the reachable ones.
|
|
||
| MockJavaResource widgetsWithThreeLambdas = | ||
| JavaResourceBase.createMockJavaResource("com.foo.Widgets", | ||
| "package com.foo;", |
There was a problem hiding this comment.
nit: Recent PRs are using text blocks, this is one of the places where it could increase readability.
There was a problem hiding this comment.
Done in aaf4998, matching the existing text block usage in JavaResourceBase.OBJECTS. Makes the two Widgets variants much easier to diff by eye.
computeAndClearStaleTypesCache() now clears cached output for the full stale set but still returns only the reachable subset. Say so in the Javadoc so callers do not read the two as interchangeable.
Matches the style already used for createMockJavaResource() in JavaResourceBase, and makes the two Widgets variants easier to diff by eye.
Problem
In incremental (Super Dev Mode) compiles,
MinimalRebuildCache.computeAndClearStaleTypesCache()filters the stale type set down to currently-reachable types before clearing cached JS. The filter's comment assumes that an unreachable stale type has no cached JS ("since they're missing JS, they will already be fully traversed when seen in Unify") -- but that invariant does not hold: a type that merely loses reachability (without its compilation unit being modified) keeps its cached JS.When such a type becomes reachable again in a later compile,
UnifyAst.needsNewJs()sees JS present, marks the type reference-only, and splices the stale chunk into the output. If the type's shape changed in the interim -- for example synthetic lambda types being renumbered because a lambda was added earlier in the file -- freshly generated call sites reference a newly allocated global name while the stale chunk defines the old one:Even when the names happen to line up, the output silently runs outdated code. Because the code server persists the MinimalRebuildCache to disk (
$TMPDIR/gwt-cache-*), the corruption survives code-server restarts and "clean" rebuilds of the application, which makes it look thoroughly mysterious to users. This is the likely root cause of the long-standing symptom reports in #9565 (see analysis), where several commenters found that deleting the code server's tmp files resolved the error.Reproduction
Three-step incremental compile sequence (encoded in the included test):
TestEntryPoint.onModuleLoad()calling bothWidgets.ping()andWidgets.trigger();trigger()contains several lambdas. Their JS is cached.trigger().Widgetsis not modified, so its lambda types keep their cached JS -- but they are now unreachable.trigger()again, and modifyWidgetsso an extra lambda is inserted first (renumbering the synthetic lambda types). The lambda types are stale, but the reachability filter (based on the previous compile's reachability) skips clearing them; the output mixes stale definitions with fresh call sites and contains anew Widgets$lambda$2$Type_2_g$(...)whose constructor is never defined.Fix
Clear cached output for the full stale set before applying the reachability filter. The filter still limits which types are artificially retraversed (its stated purpose); clearing the JS of a currently-unreachable stale type costs nothing (it is not emitted) and restores the invariant that a cached JS entry is always current -- when the type becomes reachable again,
needsNewJs()correctly triggers a fresh traversal.Testing
CompilerTest#testIncrementalRecompile_unreachableStaleTypeRegainsReachabilityfails on currentmainwithlambda type constructor Widgets$lambda$2$Type_2_g$ is referenced but never definedand passes with the fix.MinimalRebuildCacheTestpasses unchanged.Fixes #9565.