Hot-restart on web may preserve the state of Expando objects

[rrousselGit]

Here is a reproduction repository created specifically for this issue https://github.com/rrousselGit/hot-restart-expando-reproduction

Explanation

When an Expando has a JS object as a key, performing a hot-restart may not empty the Expando

Example:

final expando = Expando();

@JS()
external Person getJSObject();

@JS()
@anonymous
class Person {
  external factory Person({String name});

  external String get name;
}

void main() {
  final person = getJSObject();

  // Should always print `null` but after a hot-restart, will print "42"
  print(expando[person]);

  expando[person] = 42;

  runApp(Container());
}

Where getJSObject is a global JS function defined in the index.html as:

<script>
  const person = { name: 'Mona' }
  function getJSObject() {
    // always return the same object on purpose
    return person;
  }
</script>

With this code:

• when the application starts, it will print null
• after a hot-restart, instead of null it will print 42, which is unexpected.

The expected behavior is that after a hot-restart, the application will print null again. As otherwise, this means some state is preserved between hot-restarts

[vsmenon]

fyi - @sigmundch @nshahan @Markzipan

I suspect this would be rather expensive to do as (I think) we store the expando value on the JS object directly.

[Ehesp]

Just to add additional context; on FlutterFire we store references to the JS Firebase objects in Expando pretty much everywhere. The issue was discovered as callback subscriptions (e.g. onAuthStateChanged) were being 'lost' on hot restart, due to Expando keep reference to the previous object and not recreating a subscription.

[sigmundch]

Thanks for the report!

Indeed @vsmenon, trying to clear things out with the current implementation is likely too expensive. We'd need to swap the implementation of expandos with a WeakMap instead.

[sigmundch]

Ha, there is even a TODO from Jenny to do so -- not surprising :)

[rrousselGit]

Out of curiosity, why is the instance of Expando reused to being with?

Shouldn't a hot restart create a new Expando instance?

[vsmenon]

We don't actually track / clear all instances explicitly. Instead, we track "roots" like globals and class static variables and clear them. We rely on GC to clear the rest. That is much cheaper to do.

The problem here is that the JSObject is an unaccounted for root.

[sigmundch]

Good question @rrousselGit!

@vsmenon I think what @rrousselGit is hinting here is that we are actually using a separate expando instance.

That is. Yes, the underlying js-object is the same, and the old expando properties are leaked and kept in that object. However, two expando instances shouldn't collide with one another. If you had two expandos in a single program, they would have separate data as well. The code above declares the expando in a top level Dart variable, so the expectation would be that that new expando would be created after a hot restart.

Looking more closely, I believe we do create a new expando, but that we reuse the same key to store it's data in JavaScript. The "key" is derived from a counter that is also in a static top-level variable. That top-level variable is getting reset as well after a hot restart. As a result, a brand new Expando instance after a hot restart is reusing the same key as the original program.

We should be able to fix this by not resetting the key after hot restarts. Switching to WeakMaps would elide this problem because there wouldn't be any global state to distinguish expandos anymore.

[rrousselGit]

@sigurdm I believe this was closed inadvertently, as the commit only adds tests instead of fixing the issue.

[vsmenon]

@sigmundch - did github accidentally close this? ( Thanks for the catch @rrousselGit )

[sigmundch]

... did github accidentally close this?

Not on this case :) - but it is confusing.

There are two commits referenced above. For some reason the cross reference comment mentions one (the change that adds the test - 3a0c27b) but the message indicating that dart-bot closed this issue points to the other (the change with the fix - 460887d). Maybe it is a result of the fact that I used a "stacked" CL in gerrit, which landed 2 CLs together and pushed two commits at once?