Skip to content

Commit a5fefa3

Browse files
author
Chris Garrett
committed
[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](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.
1 parent a47b285 commit a5fefa3

File tree

5 files changed

+117
-13
lines changed

5 files changed

+117
-13
lines changed

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

+6-3
Original file line numberDiff line numberDiff line change
@@ -394,9 +394,12 @@ export class RenderTest implements IRenderTest {
394394

395395
let result = expect(this.renderResult, 'the test should call render() before rerender()');
396396

397-
result.env.begin();
398-
result.rerender();
399-
result.env.commit();
397+
try {
398+
result.env.begin();
399+
result.rerender();
400+
} finally {
401+
result.env.commit();
402+
}
400403
}
401404

402405
destroy(): void {

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

+92
Original file line numberDiff line numberDiff line change
@@ -778,4 +778,96 @@ export class BasicComponents extends RenderTest {
778778
this.render('<Foo class="top" />');
779779
this.assertHTML('<div class="top foo bar qux"></div>');
780780
}
781+
782+
@test({ kind: 'fragment' })
783+
'throwing an error during component construction does not put result into a bad state'() {
784+
this.registerComponent(
785+
'Glimmer',
786+
'Foo',
787+
'Hello',
788+
class extends EmberishGlimmerComponent {
789+
constructor(args: EmberishGlimmerArgs) {
790+
super(args);
791+
throw new Error('something went wrong!');
792+
}
793+
}
794+
);
795+
796+
this.render('{{#if showing}}<Foo/>{{/if}}', {
797+
showing: false,
798+
});
799+
800+
this.assert.throws(() => {
801+
this.rerender({ showing: true });
802+
}, 'something went wrong!');
803+
804+
this.assertHTML('<!---->', 'values rendered before the error rendered correctly');
805+
this.destroy();
806+
807+
this.assertHTML('', 'destroys correctly');
808+
}
809+
810+
@test({ kind: 'fragment' })
811+
'throwing an error during component construction does not put result into a bad state with multiple prior nodes'() {
812+
this.registerComponent(
813+
'Glimmer',
814+
'Foo',
815+
'Hello',
816+
class extends EmberishGlimmerComponent {
817+
constructor(args: EmberishGlimmerArgs) {
818+
super(args);
819+
throw new Error('something went wrong!');
820+
}
821+
}
822+
);
823+
824+
this.render('{{#if showing}}<div class="first"></div><div class="second"></div><Foo/>{{/if}}', {
825+
showing: false,
826+
});
827+
828+
this.assert.throws(() => {
829+
this.rerender({ showing: true });
830+
}, 'something went wrong!');
831+
832+
this.assertHTML(
833+
'<div class="first"></div><div class="second"></div><!---->',
834+
'values rendered before the error rendered correctly'
835+
);
836+
this.destroy();
837+
838+
this.assertHTML('', 'destroys correctly');
839+
}
840+
841+
@test({ kind: 'fragment' })
842+
'throwing an error during component construction does not put result into a bad state with nested components'() {
843+
this.registerComponent(
844+
'Glimmer',
845+
'Foo',
846+
'Hello',
847+
class extends EmberishGlimmerComponent {
848+
constructor(args: EmberishGlimmerArgs) {
849+
super(args);
850+
throw new Error('something went wrong!');
851+
}
852+
}
853+
);
854+
855+
this.registerComponent('Basic', 'Bar', '<div class="second"></div><Foo/>');
856+
857+
this.render('{{#if showing}}<div class="first"></div><Bar/>{{/if}}', {
858+
showing: false,
859+
});
860+
861+
this.assert.throws(() => {
862+
this.rerender({ showing: true });
863+
}, 'something went wrong!');
864+
865+
this.assertHTML(
866+
'<div class="first"></div><div class="second"></div><!---->',
867+
'values rendered before the error rendered correctly'
868+
);
869+
this.destroy();
870+
871+
this.assertHTML('', 'destroys correctly');
872+
}
781873
}

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ export interface ElementBuilder extends Cursor, DOMStack, TreeOperations {
9090
constructing: Option<SimpleElement>;
9191
element: SimpleElement;
9292

93-
block(): LiveBlock;
93+
hasBlocks: boolean;
9494
debugBlocks(): LiveBlock[];
9595

9696
pushSimpleBlock(): LiveBlock;

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

+13-8
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ import {
1919
VM as PublicVM,
2020
JitRuntimeContext,
2121
AotRuntimeContext,
22-
LiveBlock,
2322
ElementBuilder,
2423
} from '@glimmer/interfaces';
2524
import { LOCAL_SHOULD_LOG } from '@glimmer/local-debug-flags';
@@ -165,10 +164,6 @@ export default abstract class VM<C extends JitOrAotBlock> implements PublicVM, I
165164
return this[INNER_VM].stack as EvaluationStack;
166165
}
167166

168-
currentBlock(): LiveBlock {
169-
return this.elements().block();
170-
}
171-
172167
/* Registers */
173168

174169
get pc(): number {
@@ -529,9 +524,19 @@ export default abstract class VM<C extends JitOrAotBlock> implements PublicVM, I
529524

530525
let result: RichIteratorResult<null, RenderResult>;
531526

532-
while (true) {
533-
result = this.next();
534-
if (result.done) break;
527+
try {
528+
while (true) {
529+
result = this.next();
530+
if (result.done) break;
531+
}
532+
} finally {
533+
// If any existing blocks are open, due to an error or something like
534+
// that, we need to close them all and clean things up properly.
535+
let elements = this.elements();
536+
537+
while (elements.hasBlocks) {
538+
elements.popBlock();
539+
}
535540
}
536541

537542
return result.value;

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

+5-1
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,11 @@ export class NewElementBuilder implements ElementBuilder {
131131
return this[CURSOR_STACK].current!.nextSibling;
132132
}
133133

134-
block(): LiveBlock {
134+
get hasBlocks() {
135+
return this.blockStack.size > 0;
136+
}
137+
138+
protected block(): LiveBlock {
135139
return expect(this.blockStack.current, 'Expected a current live block');
136140
}
137141

0 commit comments

Comments
 (0)