Merge pull request #1073 from glimmerjs/bugfix/properly-try-finally-e…

…lement-builder

Ensure errors during component creation do not cause secondary errors during DOM cleanup

packages/@glimmer/integration-tests/lib/render-test.ts

@@ -394,9 +394,12 @@ export class RenderTest implements IRenderTest {
 
     let result = expect(this.renderResult, 'the test should call render() before rerender()');
 
-    result.env.begin();
-    result.rerender();
-    result.env.commit();
+    try {
+      result.env.begin();
+      result.rerender();
+    } finally {
+      result.env.commit();
+    }
   }
 
   destroy(): void {

packages/@glimmer/integration-tests/lib/suites/components.ts

@@ -778,4 +778,96 @@ export class BasicComponents extends RenderTest {
     this.render('<Foo class="top" />');
     this.assertHTML('<div class="top foo bar qux"></div>');
   }
+
+  @test({ kind: 'fragment' })
+  'throwing an error during component construction does not put result into a bad state'() {
+    this.registerComponent(
+      'Glimmer',
+      'Foo',
+      'Hello',
+      class extends EmberishGlimmerComponent {
+        constructor(args: EmberishGlimmerArgs) {
+          super(args);
+          throw new Error('something went wrong!');
+        }
+      }
+    );
+
+    this.render('{{#if showing}}<Foo/>{{/if}}', {
+      showing: false,
+    });
+
+    this.assert.throws(() => {
+      this.rerender({ showing: true });
+    }, 'something went wrong!');
+
+    this.assertHTML('<!---->', 'values rendered before the error rendered correctly');
+    this.destroy();
+
+    this.assertHTML('', 'destroys correctly');
+  }
+
+  @test({ kind: 'fragment' })
+  'throwing an error during component construction does not put result into a bad state with multiple prior nodes'() {
+    this.registerComponent(
+      'Glimmer',
+      'Foo',
+      'Hello',
+      class extends EmberishGlimmerComponent {
+        constructor(args: EmberishGlimmerArgs) {
+          super(args);
+          throw new Error('something went wrong!');
+        }
+      }
+    );
+
+    this.render('{{#if showing}}<div class="first"></div><div class="second"></div><Foo/>{{/if}}', {
+      showing: false,
+    });
+
+    this.assert.throws(() => {
+      this.rerender({ showing: true });
+    }, 'something went wrong!');
+
+    this.assertHTML(
+      '<div class="first"></div><div class="second"></div><!---->',
+      'values rendered before the error rendered correctly'
+    );
+    this.destroy();
+
+    this.assertHTML('', 'destroys correctly');
+  }
+
+  @test({ kind: 'fragment' })
+  'throwing an error during component construction does not put result into a bad state with nested components'() {
+    this.registerComponent(
+      'Glimmer',
+      'Foo',
+      'Hello',
+      class extends EmberishGlimmerComponent {
+        constructor(args: EmberishGlimmerArgs) {
+          super(args);
+          throw new Error('something went wrong!');
+        }
+      }
+    );
+
+    this.registerComponent('Basic', 'Bar', '<div class="second"></div><Foo/>');
+
+    this.render('{{#if showing}}<div class="first"></div><Bar/>{{/if}}', {
+      showing: false,
+    });
+
+    this.assert.throws(() => {
+      this.rerender({ showing: true });
+    }, 'something went wrong!');
+
+    this.assertHTML(
+      '<div class="first"></div><div class="second"></div><!---->',
+      'values rendered before the error rendered correctly'
+    );
+    this.destroy();
+
+    this.assertHTML('', 'destroys correctly');
+  }
 }

packages/@glimmer/interfaces/lib/dom/attributes.d.ts

@@ -90,7 +90,7 @@ export interface ElementBuilder extends Cursor, DOMStack, TreeOperations {
   constructing: Option<SimpleElement>;
   element: SimpleElement;
 
-  block(): LiveBlock;
+  hasBlocks: boolean;
   debugBlocks(): LiveBlock[];
 
   pushSimpleBlock(): LiveBlock;

packages/@glimmer/runtime/lib/vm/append.ts

@@ -19,7 +19,6 @@ import {
   VM as PublicVM,
   JitRuntimeContext,
   AotRuntimeContext,
-  LiveBlock,
   ElementBuilder,
 } from '@glimmer/interfaces';
 import { LOCAL_SHOULD_LOG } from '@glimmer/local-debug-flags';
@@ -165,10 +164,6 @@ export default abstract class VM<C extends JitOrAotBlock> implements PublicVM, I
     return this[INNER_VM].stack as EvaluationStack;
   }
 
-  currentBlock(): LiveBlock {
-    return this.elements().block();
-  }
-
   /* Registers */
 
   get pc(): number {
@@ -529,9 +524,19 @@ export default abstract class VM<C extends JitOrAotBlock> implements PublicVM, I
 
     let result: RichIteratorResult<null, RenderResult>;
 
-    while (true) {
-      result = this.next();
-      if (result.done) break;
+    try {
+      while (true) {
+        result = this.next();
+        if (result.done) break;
+      }
+    } finally {
+      // If any existing blocks are open, due to an error or something like
+      // that, we need to close them all and clean things up properly.
+      let elements = this.elements();
+
+      while (elements.hasBlocks) {
+        elements.popBlock();
+      }
     }
 
     return result.value;

packages/@glimmer/runtime/lib/vm/element-builder.ts

@@ -131,7 +131,11 @@ export class NewElementBuilder implements ElementBuilder {
     return this[CURSOR_STACK].current!.nextSibling;
   }
 
-  block(): LiveBlock {
+  get hasBlocks() {
+    return this.blockStack.size > 0;
+  }
+
+  protected block(): LiveBlock {
     return expect(this.blockStack.current, 'Expected a current live block');
   }