fixing an undefined case

saranrapjs commented

2017-10-17 22:48:31 +02:00

(Migrated from github.com)

I'm not sure that I have a clear description of when exactly this bug takes places, because it takes place so deep into the history of documents where we've recorded steps that it's hard to describe exactly what's happening that leads to this, but not infrequently the let next = inserted[j] line will be undefined, which leads to error later on within this undo cleanup loop. This fix guards against that. I'm realizing that we actually patched this previously, but didn't do so on the prototype, so it's possible this change got lost in the transition to the public module!

I was trying to take a stab at developing a test case for this (trying to understand the find test harness) but in the interest of time, I figured I'd send this patch your way in case the test case would be relatively simple and I'm just missing something!

I'm not sure that I have a clear description of when exactly this bug takes places, because it takes place so deep into the history of documents where we've recorded steps that it's hard to describe exactly what's happening that leads to this, but not infrequently the `let next = inserted[j]` line will be undefined, which leads to error later on within this undo cleanup loop. This fix guards against that. I'm realizing that we actually patched this previously, but didn't do so on the prototype, so it's possible this change got lost in the transition to the public module! I was trying to take a stab at developing a test case for this (trying to understand the `find` test harness) but in the interest of time, I figured I'd send this patch your way in case the test case would be relatively simple and I'm just missing something!

marijnh commented

2017-10-18 13:15:19 +02:00

(Migrated from github.com)

Ah, I screwed up with that loop index magic brinkmanship and the inner loop would sometimes break out while leaving an index at an incorrect position. Attached patch fixes it. Could you verify that it helps?

saranrapjs commented

2017-10-18 16:46:57 +02:00

(Migrated from github.com)

That definitely does the trick! Presume this'll get published out as a version bump?

marijnh commented

2017-10-18 17:11:07 +02:00

(Migrated from github.com)

Yes, I've released 1.0.1

saranrapjs commented

2017-10-18 23:53:02 +02:00

(Migrated from github.com)

I've found another related case of this, but am unable to come up with a great test-case for it. Here's some hopefully helpful evidence I've found:

This sameEnd condition happens, where the nextJ value is zero, and so the nextJ becomes -1, which breaks on the next iteration of the loop.

At that moment, the deleted[i] looks like this:

DeletedSpan {
  from: 1014,
  to: 1033,
  data:
   [ Change {
       step: [Object],
       userId: 464,
       timestamp: 1507826480302,
       id: 715,
       isPersisted: true } ],
  pos: 969,
  slice:
   Slice {
     content: Fragment { content: [Array], size: 19 },
     openStart: 0,
     openEnd: 0 } }

And the inserted[j] looks like this:

Span {
  from: 968,
  to: 969,
  data:
   [ Change {
       step: [Object],
       userId: 464,
       timestamp: 1507826481569,
       id: 716,
       isPersisted: true } ] }

...happy to collect further info if it feels relevant. A simple undefined check on next seems to fix the bug, but that may create an incorrect result in some way I don't understand... Also: lemme know if you'd rather move this into an issue, sorry to be contorting a pull request for this purpose!

I've found another related case of this, but am unable to come up with a great test-case for it. Here's some hopefully helpful evidence I've found: This [`sameEnd` condition happens](https://github.com/ProseMirror/prosemirror-changeset/blob/master/src/changeset.js#L139), where the `nextJ ` value is zero, and so the `nextJ` becomes `-1`, which breaks on the next iteration of the loop. At that moment, the `deleted[i]` looks like this: ``` DeletedSpan { from: 1014, to: 1033, data: [ Change { step: [Object], userId: 464, timestamp: 1507826480302, id: 715, isPersisted: true } ], pos: 969, slice: Slice { content: Fragment { content: [Array], size: 19 }, openStart: 0, openEnd: 0 } } ``` And the `inserted[j]` looks like this: ``` Span { from: 968, to: 969, data: [ Change { step: [Object], userId: 464, timestamp: 1507826481569, id: 716, isPersisted: true } ] } ``` ...happy to collect further info if it feels relevant. A simple `undefined` check on `next` seems to fix the bug, but that may create an incorrect result in some way I don't understand... Also: lemme know if you'd rather move this into an issue, sorry to be contorting a pull request for this purpose!

marijnh commented

2017-10-19 10:42:23 +02:00

(Migrated from github.com)

where the nextJ value is zero

That shouldn't happen, but I did figure out a case where it does. Does patch 2fc94e7 solve this for you?

(That loop is a horror, with all the splicing and side effects it does, but since this is the inner loop of a rather demanding piece of computation, I tried to avoid unnecessary allocations.)

> where the nextJ value is zero That shouldn't happen, but I did figure out a case where it does. Does patch 2fc94e7 solve this for you? (That loop is a horror, with all the splicing and side effects it does, but since this is the inner loop of a rather demanding piece of computation, I tried to avoid unnecessary allocations.)

saranrapjs commented

2017-10-19 16:11:09 +02:00

(Migrated from github.com)

That seems to fix the bug we were seeing!

saranrapjs commented

2017-10-19 16:55:50 +02:00

(Migrated from github.com)

(I presume that this patch will get published as well?)

marijnh commented

2017-10-19 17:03:30 +02:00

(Migrated from github.com)

It's been released as 1.0.2