Skip to content
This repository has been archived by the owner on Feb 12, 2022. It is now read-only.

Show callstack in browser upon REPL crashes #2021

Closed
wants to merge 6 commits into from
Closed

Show callstack in browser upon REPL crashes #2021

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4c5bc2f
Fix issue #1732 heapGraph crashes
May 24, 2018
2ac8819
linting
May 24, 2018
43902a6
changed test flag name, improved comments
May 24, 2018
4ffb7b2
Removed workaround, replaced with detecting error condition and throw…
May 25, 2018
511cb65
Removed specific error for heapGraph + __optmize(). Instead, now show…
May 31, 2018
4e74412
removed test runner heapGraph support
Jun 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions scripts/test-runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,7 @@ function runTest(name, code, options: PrepackOptions, args) {
options.invariantLevel = code.includes("// omit invariants") || args.verbose ? 0 : 99;
if (code.includes("// emit concrete model")) options.emitConcreteModel = true;
if (code.includes("// exceeds stack limit")) options.maxStackDepth = 10;
if (code.includes("// heapGraph")) options.heapGraphFormat = "VISJS";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think we need this change anymore.

if (code.includes("// react")) {
options.reactEnabled = true;
options.reactOutput = "jsx";
Expand Down
7 changes: 7 additions & 0 deletions src/serializer/serializer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,11 @@ export class Serializer {
additionalFunctionValuesAndEffects,
referentializer
);
// ModifiedBindingEntry caches the state from declarativeEnvironmentRecordsBindings
// during the first visitor. We pass that same cache to future visitors so
// those visitors receive the same object.
heapRefCounter.declarativeEnvironmentRecordsBindings =
residualHeapVisitor.declarativeEnvironmentRecordsBindings;
heapRefCounter.visitRoots();

const heapGraphGenerator = new ResidualHeapGraphGenerator(
Expand All @@ -193,6 +198,8 @@ export class Serializer {
heapRefCounter.getResult(),
referentializer
);
heapGraphGenerator.declarativeEnvironmentRecordsBindings =
residualHeapVisitor.declarativeEnvironmentRecordsBindings;
heapGraphGenerator.visitRoots();
invariant(this.options.heapGraphFormat);
heapGraph = heapGraphGenerator.generateResult(this.options.heapGraphFormat);
Expand Down
11 changes: 9 additions & 2 deletions src/utils/generator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,10 +253,17 @@ class ModifiedBindingEntry extends GeneratorEntry {
containingGenerator === this.containingGenerator,
"This entry requires effects to be applied and may not be moved"
);
invariant(this.modifiedBinding.value === this.newValue);
invariant(
this.modifiedBinding.value === this.newValue,
"ModifiedBinding's value has been changed since last visit."
);
let [residualBinding, newValue] = context.visitModifiedBinding(this.modifiedBinding);
invariant(this.residualFunctionBinding === undefined || this.residualFunctionBinding === residualBinding);
invariant(
this.residualFunctionBinding === undefined || this.residualFunctionBinding === residualBinding,
"ResidualFunctionBinding has been changed since last visit."
);
this.residualFunctionBinding = residualBinding;
this.modifiedBinding.value = newValue;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change really needed? Looks a bit scary to me. I'd rather do less state mutation on the side than more.

Copy link
Contributor Author

@caiismyname caiismyname May 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without it, we run into the following problem:

In the provided example

(function () {
  let a = __abstract("number", "a");
  let b, c;
  global.f = function() {
    b = a + 42;
    c = a + 42;
  }
  __optimize(f);
})();

when the ModifiedBindingEntry does the .visitModifiedBinding, there is a .visitEquivalentValue(newValue) call on the new value, and the returned newValue may not match the original (it's replaced by the "equivalent" value if one is found). This happens to us in this test case because of the

b = a + 42;
c = a + 42;

since the a + 42 look the same. The assignment realigns the state to what is expected by the invariant assertion.

This problem actually seems to be at least somewhat independent of the problem caused by the cache/ResidualFunctionBinding, but it was in the same code area and also causes the code to crash.

It seems that the more fundamental issue is that we're re-traversing a the heap, which has some sort of assumption that it won't be re-traversed.

Copy link
Contributor

@NTillmann NTillmann May 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which has some sort of assumption that it won't be re-traversed.

Yes, that's the underlying problem here...

While the introduced assignment makes the invariant violation go away, it changes what happens during the next visiting, which is bad.

The right thing to do would be to stop mutating state on the side in these GeneratorEntry's, they should be just values. Instead, build up new data structure in the visitor, a mapping of GeneratorEntry to something --- whatever mutable state there was, and pass that on to the next phase that really needs it, and just throw it away after the ResidualHeapGraphVisitor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yinghuitan was saying then that it might be easier to detect and reject calls to __optimize() when the --heapGraphFilePath flag is given, since the two haven't been made to work together yet. Would that be a more relevant solution?

this.newValue = newValue;
return true;
}
Expand Down
18 changes: 18 additions & 0 deletions test/serializer/additional-functions/heapGraph.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Tests that code with heapgraph enabled doesn't crash.
// Regression test for issue #1732.

// heapGraphFilePath

(function () {
let a = global.__abstract ? __abstract("number", "5") : 5;
let b, c;
global.f = function() {
b = a + 42;
c = a + 42;
return b;
}
global.__optimize && __optimize(f);

})();

inspect = function() { return global.f(); }