Skip to content

Commit

Permalink
[BUGFIX] Properly cleanup the element builder in a try/finally
Browse files Browse the repository at this point in the history
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](emberjs/ember-test-helpers#768 (comment)).

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.
  • Loading branch information
Chris Garrett committed Apr 16, 2020
1 parent a47b285 commit a5fefa3
Show file tree
Hide file tree
Showing 5 changed files with 117 additions and 13 deletions.
9 changes: 6 additions & 3 deletions packages/@glimmer/integration-tests/lib/render-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
92 changes: 92 additions & 0 deletions packages/@glimmer/integration-tests/lib/suites/components.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
}
}
2 changes: 1 addition & 1 deletion packages/@glimmer/interfaces/lib/dom/attributes.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ export interface ElementBuilder extends Cursor, DOMStack, TreeOperations {
constructing: Option<SimpleElement>;
element: SimpleElement;

block(): LiveBlock;
hasBlocks: boolean;
debugBlocks(): LiveBlock[];

pushSimpleBlock(): LiveBlock;
Expand Down
21 changes: 13 additions & 8 deletions packages/@glimmer/runtime/lib/vm/append.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import {
VM as PublicVM,
JitRuntimeContext,
AotRuntimeContext,
LiveBlock,
ElementBuilder,
} from '@glimmer/interfaces';
import { LOCAL_SHOULD_LOG } from '@glimmer/local-debug-flags';
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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;
Expand Down
6 changes: 5 additions & 1 deletion packages/@glimmer/runtime/lib/vm/element-builder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
}

Expand Down

0 comments on commit a5fefa3

Please sign in to comment.