2.4 crashes if a format uses CodeMirror.addOverlay()

webbedspace commented 2 years ago

Here's an example - even a nothing overlay like this results in a crash.

cm.addOverlay({token(s){s.skipToEnd();}});

Here's the crash message:

This does not occur in Twine 2.3, by the way.

Presubmission checklist

[ ] I am interested in working on code that would fix this bug. (This is not required to submit a bug report.)
[X] I have done a search and believe that an issue does not already exist for this bug in the GitHub repository.
[X] I have read and agree to abide by this project's Code of Conduct.

webbedspace commented 2 years ago

Update on this: it won't crash if the overlay is added after CodeMirror has finished the initial render of the passage, but it will crash if it's used before that (such as in the mode's startState() or token()).

klembot commented 2 years ago

I tried repoing this myself but couldn't. I think if you can point to code I can try out, I can make more progress on it/understand the use case more. (I've never used CM overlays before I started looking at this bug.) What I tried was adding your code snippet at the top of [Chapbook's toolbar function]:(https://github.com/klembot/chapbook/blob/develop/src/editor-extensions/twine/codemirror-toolbar.js#L9)

editor.addOverlay({token(s) {s.skipToEnd()});

It didn't do anything, but it also didn't crash. I'm wondering if it is related to how you are getting the editor instance. I don't see anywhere in CM's mode API where that's possible but I also wasn't able to find a super concise summary of CodeMirror.defineMode's API, either.

webbedspace commented 2 years ago

EDIT: Hang on, coming up with another test case...

webbedspace commented 2 years ago

Here's an in-progress build of Harlowe 3.3 for reference format.zip, and here's one which is the same except it's not minified (only run through Babel) and that cm.addOverlay({token(s){s.skipToEnd();}}); has been inserted in a function that is called the first time the mode's token() is run: format.zip. (It was inserted after this line which is called by this line.) The latter format will cause the error (ONLY in 2.4) when any passage is opened.

If you're wondering why I would put something like that there: this originally came about because I was debugging the find/replace overlay I'm adding to 3.3. The actual final overlay for that feature is handled elsewhere.

By the way, the editor instance comes from this line - the doc received here from stream.lineOracle has a cm property. You might notice this is different to how previous Harlowe versions received the editor instance - lineOracle is an undocumented property added in 2017 and that file has been unchanged for the past 5 years. I consider this safe to use (personally) because CodeMirror 5 is de facto EOL due to the existence of CM6, and its recent release history shows only very minor bug fixes.

klembot commented 2 years ago

Thanks, I'm able to reproduce the problem with the second format.zip you attached. I'll poke around and see what options there are.

In general, my reading of the CM docs is that modes are not meant to interact with editor instances, but I suppose it depends on your view of undocumented behavior. At minimum, Twine shouldn't crash in this scenario.

klembot commented 2 years ago

I've done some poking around and I have somewhat of a better idea of what is going on, but I still am unsure on what the resolution could be. The most straightforward one seems to be to defer the setOverlay call inside the format, using either window.setTimeout or Promise.resolve().then(), as you suggested. Both seem to work on the trivial use case you added. I'm not sure if this is viable for your actual use case, though.

This comment is pretty long so I'll break it up into sections.

Things I tried

I tried delaying setting the CodeMirror mode. I did this by changing line 32 of use-format-codemirror-mode.ts to:

React.useEffect(() => {
  if (extensionsDisabled) {
    return;
  }

  if (format.loadState === 'unloaded') {
    dispatch(loadFormatProperties(format));
  } else if (format.loadState === 'loaded') {
    const editorExtensions = formatEditorExtensions(format, twineVersion);

    window.setTimeout(() => {
      if (editorExtensions?.codeMirror?.mode) {
        CodeMirror.defineMode(
          namespaceForFormat(format),
          editorExtensions.codeMirror.mode
        );
        setModeName(namespaceForFormat(format));
      }
    }, 1000);
  }
}, [dispatch, extensionsDisabled, format]);

If I do this, the editor opens for 1 second, then crashes (when the mode is applied).

I tried bypassing the React bindings for CodeMirror and interacting with the instance directly by removing mode from the options object passed to the CodeMirror component, and adding this effect in passage-text.tsx:

React.useEffect(() => {
  if (mode && editor) {
    window.setTimeout(() => editor.setOption('mode', mode), 1000);
  }
}, [editor, mode]);

This shows the same behavior: looks OK for 1 second, then crashes.

Finally, I tried downgrading CodeMirror to 5.32.0, the version 2.3 uses, but that didn't change anything.

Preventing the crash

To prevent the crash, I'd need to put an error boundary around the editor component because the error happens after the story format extension code runs. The only remediation I see, though, is to disable extensions in the editor and try again. I think it would be good for users because it would allow them to continue editing, but it doesn't solve the core issue.

Even more details

Here are some more details on what I see going on:

This line of code is failing: container.insertBefore(node, cur); It's inside CodeMirror code.
- container is a div.CodeMirror-code element and is in the DOM.
- node is a pre.CodeMirror-line element and is not in the DOM. It looks like it contains the first line of the passage source with mode tokens applied (e.g. spans with classes). node.parentElement is null.
- cur is a pre.CodeMirror-line element and is not in the DOM. It looks like it contains the first line of the passage source but with no mode tokens applied. ⚠️ cur.parentElement is null. This causes the error.
Call stack (skipping stuff that doesn't look useful) is:
- patchDisplay // Sync the actual display DOM structure with display.view, removing nodes for lines that are no longer in view, and creating the ones that are not there yet, and updating the ones that are out of date.
- updateDisplayIfNeeded // Does the actual updating of the line display. Bails out (returning false) when there is nothing to be done and forced is false.
  - Call is: patchDisplay(cm, display.updateLineNumbers, update.dims);
- endOperations // The DOM updates done when an operation finishes are batched so that the minimum number of relayouts are required.
- finishOperation
- endOperation // Finish an operation, updating the display and signalling delayed events
The error occurs in react-codemirror2's hydrate function, called by its componentDidUpdate function. It looks like the purpose of this is to apply option changes to the CodeMirror instance, and from looking at the call stack it's applying the Harlowe CM mode.
- I think it's happening immediately after setting the mode, not some time afterwards.
Harlowe code doesn't appear in the error call stack at all.
- I think something about the addOverlay() call is disrupting later work in the same tick.

webbedspace commented 2 years ago

Do you have any idea why this doesn't throw in Twine 2.3? (The linked format.zip can load its highlighting mode and toolbar in 2.3, so you could compare them.)

klembot commented 2 years ago

My best guess right now is it's a side effect of how React handles DOM updates and/or how react-codemirror2 works. I have a very shallow understanding of React works under the hood, so I could be totally off. But it seems like the CodeMirror instance is spending some time not mounted in the DOM, maybe?

klembot / twinejs