Show callstack in browser upon REPL crashes #2021

caiismyname · 2018-05-24T21:31:19Z

Fixing some incompatibility between heapGraph and __optimize().
Essence of the issue was an implicit assumption that only one visitor would visit certain entries.

Original Issue: #1732

hermanventer · 2018-05-24T21:41:09Z

src/utils/generator.js

@@ -253,10 +253,11 @@ 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 improperly modified.");


This is a good example of an utterly useless comment. Much better (and much harder) is to write a comment that explains to a reader of the code why this invariant is expected to hold. In terms of debug logging, for those who care about such things, it would be better to include information about the two values.

jeffreytan81

Great job!

jeffreytan81 · 2018-05-24T22:03:34Z

scripts/test-runner.js

@@ -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("// heapGraphFilePath")) options.heapGraphFormat = "VISJS";


It is not a file path, you are using VISJS format here. I would call "// heap graph" instead of heapGraphFilePath

Will do. I put it like that originally to avoid confusion with the actual flag (the flag itself is --heapGraphFilePath so I kept it the same)

Yeah, heapGraphFilePath is a command line level option not actually the engine/serializer level option, which is heapGraphFormat. But either way, we are just trying to enable heap graph and do not care about the output in this test.

jeffreytan81 · 2018-05-24T22:06:34Z

src/serializer/serializer.js

@@ -182,6 +182,9 @@ export class Serializer {
            additionalFunctionValuesAndEffects,
            referentializer
          );
+          // Copying dERB map so cache lookup returns the same object, and the subsequent


I would write the comment like: ModifiedBindingEntry caches the state from declarativeEnvironmentRecordsBindings during visitor so we have to pass the same cache to future visitors.

NTillmann · 2018-05-24T23:07:20Z

src/utils/generator.js

    this.residualFunctionBinding = residualBinding;
+    this.modifiedBinding.value = newValue;


Is this change really needed? Looks a bit scary to me. I'd rather do less state mutation on the side than 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.

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.

@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?

…ing error

caiismyname · 2018-05-25T18:11:18Z

src/serializer/serializer.js

+          if (additionalFunctionValuesAndEffects.size > 0) {
+            let diagnostic = new CompilerDiagnostic(
+              "Using __optimize with --heapGraphFilePath flag is not currently supported",
+              undefined,


Wasn't sure what to give these fields since they're not really applicable. Suggestions?

NTillmann

If the new approach is now to fail with an error, then this would make our website REPL unusable as soon as __optimize is used.

Some heuristics could be used in the website code to disable the heap graph if the text contains the word __optimize. I am not a fan of that complicated working around the actual issue, but still better than crashing.

… callstacks along with error message (for all errors) in repl

caiismyname · 2018-05-31T23:15:28Z

Screenshot of new behavior on the code snippet that previously caused the REPL to crash

jeffreytan81

Awesome! Much better REPL diagnostics experience.
You may want to retitle the PR since we are not fixing crash anymore.

jeffreytan81 · 2018-06-01T04:52:05Z

scripts/test-runner.js

@@ -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";


I do not think we need this change anymore.

facebook-github-bot

@trueadm is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Fix issue #1732 heapGraph crashes

4c5bc2f

caiismyname requested review from cblappert, NTillmann, hermanventer and jeffreytan81 May 24, 2018 21:31

facebook-github-bot added the CLA Signed label May 24, 2018

hermanventer approved these changes May 24, 2018

View reviewed changes

jeffreytan81 approved these changes May 24, 2018

View reviewed changes

David Cai added 2 commits May 24, 2018 15:09

linting

2ac8819

changed test flag name, improved comments

43902a6

NTillmann reviewed May 24, 2018

View reviewed changes

Removed workaround, replaced with detecting error condition and throw…

4ffb7b2

…ing error

caiismyname commented May 25, 2018

View reviewed changes

NTillmann suggested changes May 25, 2018

View reviewed changes

Removed specific error for heapGraph + __optmize(). Instead, now show…

511cb65

… callstacks along with error message (for all errors) in repl

jeffreytan81 approved these changes Jun 1, 2018

View reviewed changes

caiismyname changed the title ~~Fix issue #1732 heapGraph crashes~~ Show callstack in browser upon REPL crashes Jun 1, 2018

removed test runner heapGraph support

4e74412

NTillmann approved these changes Jun 1, 2018

View reviewed changes

trueadm approved these changes Jun 3, 2018

View reviewed changes

facebook-github-bot reviewed Jun 3, 2018

View reviewed changes

facebook-github-bot closed this in d96eaf4 Jun 3, 2018

hermanventer deleted the bug1732 branch June 5, 2018 20:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show callstack in browser upon REPL crashes #2021

Show callstack in browser upon REPL crashes #2021

caiismyname commented May 24, 2018

hermanventer May 24, 2018

jeffreytan81 left a comment

jeffreytan81 May 24, 2018

caiismyname May 24, 2018

jeffreytan81 May 24, 2018

jeffreytan81 May 24, 2018

NTillmann May 24, 2018

caiismyname May 24, 2018 •

edited

Loading

NTillmann May 25, 2018 •

edited

Loading

caiismyname May 25, 2018

caiismyname May 25, 2018

NTillmann left a comment

caiismyname commented May 31, 2018 •

edited

Loading

jeffreytan81 left a comment

jeffreytan81 Jun 1, 2018

facebook-github-bot left a comment

		this.residualFunctionBinding = residualBinding;
		this.modifiedBinding.value = newValue;

Show callstack in browser upon REPL crashes #2021

Show callstack in browser upon REPL crashes #2021

Conversation

caiismyname commented May 24, 2018

Choose a reason for hiding this comment

jeffreytan81 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caiismyname May 24, 2018 • edited Loading

Choose a reason for hiding this comment

NTillmann May 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NTillmann left a comment

Choose a reason for hiding this comment

caiismyname commented May 31, 2018 • edited Loading

jeffreytan81 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

caiismyname May 24, 2018 •

edited

Loading

NTillmann May 25, 2018 •

edited

Loading

caiismyname commented May 31, 2018 •

edited

Loading