Updating selection between rendering can crash

[colinjeanne]

There is a timing window that exists from when a new editorState is passed into draftjs as a prop until that new state is rendered. If an onSelect event fires in this time, then the code will throw because the editorState doesn't yet match the DOM state. getUpdatedSelectionState should detect this case and just return editorState.getSelection() in this scenario.

This is a port of textioHQ#2 and fixes #473. #473 is closed because the original poster no longer repros the issue but others in Slack have said they are hitting the issue and that this fixes their repro.

[cameracker]

+1 this is a major problem for us, and the fix is simple. Please merge in soon! :)

[aligon]

I've also been getting this error. Any word on this pull request?

[adamskihood]

+1 this bug is very annoying. Can you merge the fix, please?

[juliankrispel]

Please merge this @facebook-github-bot - do your magic

[flarnie]

thanks for everyone's patience, I'll try to look at this over the weekend, and pinging other maintainers to see if anyone can verify it sooner

[juliankrispel]

@flarnie thank you so much <3

[juliankrispel]

@flarnie here take this delicious slice of cake 🍰 and a beer 🍺

[juliankrispel]

@flarnie while you're at it would you mind looking at this as well #1192

[juliankrispel]

@flarnie any luck? :)

[flarnie]

@juliankrispel thanks for your patience - want to get Github and facebook's Draft copies in sync before making any further changes to either, and found they had diverged more than I realized. Will be talking to folks today about how to get them to auto-sync, so that we can merge things here without creating extra work.

[Topperfalkon]

@flarnie Don't suppose there's been any progress with this?

[adamskihood]

Any update on that @flarnie?

[Topperfalkon]

@flarnie any progress with this?

[saharj]

Any update here?

[flarnie]

Thanks everyone who has pinged this - it seems like a problem worth solving.

tldr: race conditions are a known pitfall with Draft.js and I'd like to understand the problem more before taking a change like this.

I'd really like to get master back in sync with the textioHQ fork of Draft.js, which seems to have been ahead of the curve with bug fixes in the past. So @AnilRedshift @max-winderbaum @colinjeanne maybe you can help me connect the dots here.

In the example repro case from that issue with two editors, it looks like there is an 'onSelect' event which fires and somehow gets an editorState which has a ref to the DOMNode for the top editor, but the BlockTree from the second editor.

I'm trying to reconcile that with the description of this PR:

"There is a timing window ..."

-> in the repro example, it's not a window of time. It just happens automatically once you get the editorState into this broken state with the mismatch between the ref and the BlockTree.

"the code will throw because the editorState doesn't yet match the DOM state."

-> The DOM state always looks correct to me when I inspect it - the data-offset-key is unchanged in editor1 and editor2, and gets identified correctly in the event handler. It seems like somehow we are getting mixing between the two editorStates though, which could be a problem with Draft.js, or a pitfall that is easy to fall into perhaps.

If the editorState gets mixed up between two editors, we need to fix the root cause. I would like to get more info about how that happens and a PR to fix the root cause, if possible.

[intentionally-left-nil]

Hi Flarnie,

With the caveat that I haven't worked at Textio since March, I'll add my recollections below. Hopefully Colin or Max can help out with the latest of textio/draft-js

The problem here is not that you have two editors, but one editor with diverging editorStates.

At its core, the issue is this: The editorState changes at a different time than when the DOM changes. At some point after render() only then does the editorState again reflect what is in the DOM. If you end up with any events (such as selection, or otherwise) that come in before this synchronization, you end up trying to relate DOM nodes to a different editorState.

Having the time & space to think about this more, I think a more robust solution than the fix I proposed would be to tag the editorState with an incrementing key on each update, and then saving that key to the DOM. Then, when you get a new event, you can compare and see if that event happened while the DOM was out of sync. If so, you can drop the event (or take other actions)

If I recall correctly, the benefit of the fix from this PR is that it's a pretty low-risk fix. It doesn't try to change any broader architecture issues but just detects when an event occurs in this timing window. So, it may be a reasonable thing to add in the shorter term.

This issue isn't seen as much when just typing normally, because draft-js allows the browser to update the DOM when possible. Even otherwise, draft-js reuses most of the nodes so there is only one area where this timing window is possible.

On the other hand, if you replace the entire editorState with a different editorState, then there's a better chance to get an event in the middle that doesn't map.

Does this help?

[colinjeanne]

@flarnie: I would love to bring https://github.com/textioHQ/draft-js back in sync with master and have a few of our changes in PR here. The largest is that our fix for #989 has diverged quite a bit from what exists in the code (in particular, we don't have 1c6a49b#diff-638c3e5663e5bd194aefe231001f51f2 or 5863399#diff-638c3e5663e5bd194aefe231001f51f2 and we haven't had the time to investigate how to merge this into our branch).

Otherwise @AnilRedshift's recollection of the issue matches mine. I don't recall the repro because we unfortunately didn't describe it in our bug and my memory of it had already been lost when I originally opened this.

[flarnie]

Thanks both @AnilRedshift and @colinjeanne for responding! That is helpful - I still would like to get a repro, and ideally even a test, for this bug. It seems like what you are describing is related to the "DualEditor" example but not quite the same.

Hoping I'll have time to look at this more on the weekend.

[colinjeanne]

@flarnie: as I recall this was very timing related. Our editor has a rather complex decorator which likely increased the timing window for us but I wonder if a simple repro might be to simply move the DOM selection manually in a call to render.

[shpakkdv]

Faced with this issue. Crash in Safari (only) when Draft.js is updating the editor state.
The error which I see: IndexSizeError: The index is not in the allowed range.

My solution / hack:

  const nativeSelectionExtend = Selection.prototype.extend;

  Selection.prototype.extend = function (...args) {
    try {
      return nativeSelectionExtend.apply(this, args);
    } catch (error) {
      console.log('Selection error.', error);
    }
  };

It works properly for me. Maybe will be useful for somebody as well.