conditional instantiate still runs removed bindings

jmesserly commented 11 years ago

@sigmundch and I had a bug in web-ui where we were evaluating bindings in the body conditional binding, even after the condition was changed to false. I think I created the same problem in MDV by tweaking the conditional_display_using_undefined.html example.

You should be able to reproduce with the code below.

If you click the "Click me!" button, it looks like MDV tried to evaluate the expression even though the instantiate= is no longer valid. The output I got was:

Earth years is 63 conditional_display_using_undefined.html:116
Martian years is 33.49643366619116 conditional_display_using_undefined.html:121
Earth years is 44 conditional_display_using_undefined.html:116
Martian years is 23.394334624006518 conditional_display_using_undefined.html:121
Changing model! conditional_display_using_undefined.html:105
Earth years is null conditional_display_using_undefined.html:116
Bad year!!! conditional_display_using_undefined.html:118
Martian years is 0

In this case evaluating "martianAge" with a bad year is harmless, but it seems like this could cause the application to end up in an invalid state.

Here's the code:

<!DOCTYPE html>
<html>
<!--
Copyright 2011 Google Inc.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

     http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
-->
<head>
<title>Conditional Display using Undefined</title>
<link rel="stylesheet" href="common_styles.css">
<script src="include.js"></script>
</head>
<body>
  <div class="example">
    <h1>Conditional Display using Undefined</h1>
    <button id="changeModel">Click me!</button>
    <div class="content">
      <ul>
      <template iterate="people">
        <li class="person">
          Person: {{ name }}
          <template instantiate="dog">
            <div class="dog">
              <div>Dog: {{ name }}</div>
              <div>Age: {{ age }}</div>
              <div>Martian age:
                {{ expr(age) martianAge(age) }}
              </div>
              <div>Food: <ul>
                <template iterate="food">
                  <li>{{ }}</li>
                </template>
              </ul></div>
            </div>
          </template>
        </li>
      </template>
      </ul>
    </div>
  </div>

<script>
document.body.model = {
  people: [
    {
      name: 'Charlie Brown',
      dog: {
        name: 'Snoopy',
        age: new Date().getFullYear() - 1950,
        food: ['Angel food cake with seven-minute frosting', 'Pizza', 'Donuts',
               'Chocolate chip cookies']
      }
    },
    {
      name: 'Shaggy',
      dog: {
        name: 'Scooby-doo',
        age: new Date().getFullYear() - 1969,
        food: ['Scooby Snacks', 'Pizza', 'Hamburgers', 'Potato chips',
               'Ice cream', 'Chocolate']
      }
    },
    {
      name: 'Beetle Bailey'
    }
  ]
};

function changeStuff() {
  console.log('Changing model!');
  var shaggy = document.body.model.people[1];
  var scoobyDoo = shaggy.dog;
  if (scoobyDoo != null) {
    shaggy.dog = null;
    scoobyDoo.age = null; // immortality!
  }
  Model.notifyChanges();
}

function martianAge(earthYears) {
  console.log('Earth years is ' + earthYears);
  if (!earthYears) {
    console.error('Bad year!!!');
  }
  var result =  earthYears * 365.26 / 686.98;
  console.log('Martian years is ' + result);
  return result;
}

document.querySelector('button#changeModel').onclick = changeStuff;
document.body.modelDelegate = MDVDelegate;

</script>

  <div class="comment">
    <h1>Comment</h1>
    <div class="content">
      It is often common to want to hide whole sub trees if the model is
      undefined so as not to show a bunch of meaningless data. A template that
      has an undefined computed model will not be instantiated.
    </div>
  </div>
</body>
</html>

arv commented 11 years ago

Turns out we don't remove the bindings. This bug sneaked in when we ported the C++ version over. The C++ version worked (in most cases) because if the ref counting reached zero when the instance was removed and the bindings got removed for free.

arv commented 11 years ago

Fixed in 8bc1e3466aeb6930150c0d3148f0e830184bf599

googlearchive / TemplateBinding

conditional instantiate still runs removed bindings #8