instance binding map breaks if custom elements append from created (e.g. Polymer's "lightdom")

jmesserly commented 10 years ago

I hit this in a more complex use case that involved polymer-elements and lightdom. But it reproduces with plain custom elements. The issue is that the createdCallback can append to custom element, which invalidates instanceBindingMap that was just created, resulting in incorrect bindings.

If you delay the append until enteredViewCallback the problem goes away. Note however that Polymer's lightdom will append from createdCallback. This makes it very risky to use "lightdom" and templates together.

<!DOCTYPE html>
<html>
  <head>
    <script src="load.js"></script>
  </head>
  <body>

    <template id="x-greeting">
      <ul>
        Greetings:
        <template repeat="{{ salutations }}">
          <li>{{ what }}: <input type="text" value="{{ who }}"></li>
        </template>
      </ul>
    </template>
    <template id="x-hello">
      <x-greeting from="{{speaker}}"></x-greeting>
      <h1>Hello from {{speaker}}!</h1>
    </template>

    <x-hello></x-hello>

  <script>
function initXGreeting() {
  console.log('x-greeting init');
  var template = document.getElementById('x-greeting');
  var model = {
    salutations: [
      { what: 'Hello', who: 'World' },
      { what: 'GoodBye', who: 'DOM APIs' },
      { what: 'Hello', who: 'Declarative' },
      { what: 'GoodBye', who: 'Imperative' }
    ]
  };
  this.appendChild(template.createInstance(model));
};

function initXHello() {
  console.log('x-hello init');
  var template = document.getElementById('x-hello');
  this.appendChild(template.createInstance({speaker: 'TemplateBinding'}));
};

var XGreetingProto = Object.create(HTMLElement.prototype);
var XHelloProto = Object.create(HTMLElement.prototype);

if (true) {
  XGreetingProto.createdCallback = initXGreeting;
  XHelloProto.createdCallback = initXHello;
} else {
  XGreetingProto.enteredViewCallback = initXGreeting;
  XHelloProto.enteredViewCallback = initXHello;
}

document.register('x-greeting', {prototype: XGreetingProto});
document.register('x-hello', {prototype: XHelloProto});
  </script>
  <body>
</html>

jmesserly commented 10 years ago

I think the bug is here: https://github.com/Polymer/TemplateBinding/blob/db36175f9374826644d14161a24fc09d1d702c09/src/TemplateBinding.js#L546

    createInstance: function(model, bound) {
      var content = this.ref.content;
      var map = content.bindingMap_;
      if (!map) {
        // TODO(rafaelw): Setup a MutationObserver on content to detect
        // when the instanceMap is invalid.
        map = createInstanceBindingMap(content, this.prepareBindingFn_) || [];
        content.bindingMap_ = map;
      }

      var stagingDocument = getTemplateStagingDocument(this);
      var instance = deepCloneIgnoreTemplateContent(content, stagingDocument);

      addMapBindings(instance, map, model, this.bindingDelegate_, bound);
      // TODO(rafaelw): We can do this more lazily, but setting a sentinel
      // in the parent of the template element, and creating it when it's
      // asked for by walking back to find the iterating template.
      addTemplateInstanceRecord(instance, model);
      return instance;
    },

The deepCloneIgnoreTemplateContent line can result in the "map" being out of date by the time we run addMapBindings. Depending on how it is mismatched it can result in various kinds of behavior.

Even if the TODO was addressed, I don't think it would help, the issue is that addMapBindings relies on the tree structure matching the template content structure, but with custom elements that isn't necessarily the case. Perhaps deepCloneIgnoreTemplateContent could detect that children were added immediately from createdCallback.

Maybe the answer is "don't add children from createdCallback" in which case Polymer's lightdom feature should be changed...

jmesserly commented 10 years ago

hmmmm. This only reproduces using native document.register. If I add:

<script src="../platform-dev/platform.js"></script>

it doesn't reproduce. My example might not be capturing the right problem. Going to see if I can refine it. It's tricky because the mismatch between how many children we thought we had (in createInstanceBindingMap) and how many we actually have shows up much more easily in the Dart port due to bounds checking on map.children (which of course, we could remove by converting it to a Map ... but I worry that will just change it from an early exception to corrupt data bindings).

sorvell commented 10 years ago

I believe this is another flavor of this problem: https://github.com/Polymer/polymer/issues/330. @raf can chime in, but I believe his concern was that to maintain correctness would compromise performance too much when using the polyfill.

The lightdom feature is definitely experimental, needs work, and has a couple of known issues (this, https://github.com/Polymer/polymer/issues/331)

On Thu, Dec 5, 2013 at 4:05 PM, John Messerly notifications@github.comwrote:

hmmmm. This only reproduces using native document.register. If I add:

it doesn't reproduce. My example might not be capturing the right problem. Going to see if I can refine it. It's tricky because the mismatch between how many children we thought we had (in createInstanceBindingMap) and how many we actually have shows up much more easily in the Dart port due to bounds checking on map.children (which of course, we could remove by converting it to a Map ... but I worry that will just change it from an early exception to corrupt data bindings).

— Reply to this email directly or view it on GitHubhttps://github.com/Polymer/TemplateBinding/issues/150#issuecomment-29871905 .

jmesserly commented 10 years ago

good catch. Yeah, looks like the same issue. (though I'm pretty sure it is a bug in TemplateBinding+CustomElements and not Polymer specific).

I may have found a reasonable workaround. createInstanceBindingMap makes assumptions about the number of child nodes (call this numChildren). Based on the order of cloning in deepClone*, if createdCallback adds any extra children they will be first. So addMapBindings could simply skip excess children at the beginning.

but yeah good point about issue 331. That makes it hard to use lightdom.

The reason I'm looking at this: we're trying to deploy a replacement for https://api.dartlang.org/. I noticed from profiles that Shadow DOM was taking a lot of time. Removing it shaved off ~35% of the load time, which is about a ~50% speedup. Pretty significant. The app is currently a bit too slow for us to switch to :( ... but I'm hopeful we can get it fast enough.

rafaelw commented 10 years ago

I believe the instance production logic fixes these problems and bindings can no longer be confused in this way. I'm closing this bug. Please re-open if you discover I am wrong.

googlearchive / TemplateBinding

instance binding map breaks if custom elements append from created (e.g. Polymer's "lightdom") #150