From a5fefa30c6538cbdff0e0725cf21de51b22486cc Mon Sep 17 00:00:00 2001 From: Chris Garrett Date: Wed, 15 Apr 2020 17:25:48 -0700 Subject: [PATCH] [BUGFIX] Properly cleanup the element builder in a try/finally Currently we don't do any cleanup in the event of an error occuring during the actual execution of the VM. This can leave the VM in a bad state, in particular the element builder, since it doesn't actually finalize block nodes until _after_ the entire block has executed. This means that if an error occurs during the execution of a block, the VM will never properly initialize those blocks, and their first and last nodes will be `null`. While we don't currently support recovering from this type of an error, we do need to be able to cleanup the application after one, specifically for tests. Previously, this worked no matter what because of the Application Wrapper optional feature - there was always a root `div` element that we could clear, so there was always a first and last node for us to track. With the feature disabled, we started seeing failures such as [this issue within user tests](https://github.com/emberjs/ember-test-helpers/issues/768#issuecomment-583752979). This PR refactors VM updates, specifically, to allow them to properly clean up the element builder on failures. It now closes all remaining open blocks, finalizing them to ensure they have nodes. This only works for VM updates because the initial append is an _iterator_, which the user controls execution of. We'll need to have a different strategy for this API, but it shouldn't be a regression at the moment because any failures during the iteration will result in a completely broken app from the get-go. There is no result, and no accompanying `destroy` function, so existing tests and apps cannot be affected by failures during append. --- .../integration-tests/lib/render-test.ts | 9 +- .../lib/suites/components.ts | 92 +++++++++++++++++++ .../interfaces/lib/dom/attributes.d.ts | 2 +- packages/@glimmer/runtime/lib/vm/append.ts | 21 +++-- .../runtime/lib/vm/element-builder.ts | 6 +- 5 files changed, 117 insertions(+), 13 deletions(-) diff --git a/packages/@glimmer/integration-tests/lib/render-test.ts b/packages/@glimmer/integration-tests/lib/render-test.ts index d606f8cfbd..2a5ebb1360 100644 --- a/packages/@glimmer/integration-tests/lib/render-test.ts +++ b/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 { diff --git a/packages/@glimmer/integration-tests/lib/suites/components.ts b/packages/@glimmer/integration-tests/lib/suites/components.ts index 4792f575f4..a2131f44ed 100644 --- a/packages/@glimmer/integration-tests/lib/suites/components.ts +++ b/packages/@glimmer/integration-tests/lib/suites/components.ts @@ -778,4 +778,96 @@ export class BasicComponents extends RenderTest { this.render(''); this.assertHTML('
'); } + + @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}}{{/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}}
{{/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 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', '
'); + + this.render('{{#if showing}}
{{/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'); + } } diff --git a/packages/@glimmer/interfaces/lib/dom/attributes.d.ts b/packages/@glimmer/interfaces/lib/dom/attributes.d.ts index de172cda73..14012a7bd9 100644 --- a/packages/@glimmer/interfaces/lib/dom/attributes.d.ts +++ b/packages/@glimmer/interfaces/lib/dom/attributes.d.ts @@ -90,7 +90,7 @@ export interface ElementBuilder extends Cursor, DOMStack, TreeOperations { constructing: Option; element: SimpleElement; - block(): LiveBlock; + hasBlocks: boolean; debugBlocks(): LiveBlock[]; pushSimpleBlock(): LiveBlock; diff --git a/packages/@glimmer/runtime/lib/vm/append.ts b/packages/@glimmer/runtime/lib/vm/append.ts index bc178bd6c0..9ac8e08bc0 100644 --- a/packages/@glimmer/runtime/lib/vm/append.ts +++ b/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 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 implements PublicVM, I let result: RichIteratorResult; - 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; diff --git a/packages/@glimmer/runtime/lib/vm/element-builder.ts b/packages/@glimmer/runtime/lib/vm/element-builder.ts index 374e22a60d..af37337f1f 100644 --- a/packages/@glimmer/runtime/lib/vm/element-builder.ts +++ b/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'); }