Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mapper bindings are removed too late when reconversion is involved #15411

Closed
scofalik opened this issue Nov 24, 2023 · 0 comments · Fixed by #15412
Closed

Mapper bindings are removed too late when reconversion is involved #15411

scofalik opened this issue Nov 24, 2023 · 0 comments · Fixed by #15412
Assignees
Labels
package:engine squad:collaboration Issue to be handled by the Collaboration team. support:2 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@scofalik
Copy link
Contributor

scofalik commented Nov 24, 2023

📝 Provide detailed reproduction steps (if any)

Okay, I know the title says it all. But this is a very technical issue.

This is DowncastDispatcher#converChanges() with unimportant parts removed:

public convertChanges( ... ) {
	// Convert changes that happened on model tree.
	for ( const entry of changes ) {
		// ( 1 )
	}

	// ( 2 )
	for ( const markerName of conversionApi.mapper.flushUnboundMarkerNames() ) {
		const markerRange = markers.get( markerName )!.getRange();

		this._convertMarkerRemove( markerName, markerRange, conversionApi );
		this._convertMarkerAdd( markerName, markerRange, conversionApi );
	}
	
	// ( 3 )
	conversionApi.mapper.flushDeferredBindings();
}

The following happens:

  1. During ( 1 ) some elements are removed and reinserted.
  2. After a view element is removed, we need to clear its model mapping.
  3. However, for reconversion, purposes, it does not happen immediately (at ( 1 )), instead it is deferred to ( 3 ).
  4. When a removed view element has a marker, we want to save this information, and refresh this marker at ( 2 ).
  5. But, as mentioned, the view element is not unbound immediately. The bindings are removed at ( 3 ).
  6. So, only at ( 3 ) we add markers to unbound marker names.
  7. But it is too late, as we are already after ( 2 ). This means that the markers are not refreshed at ( 2 ) and they are still cached in the mapper.
  8. This may lead to an error later on if the marker got removed, but is still returned by Mapper#flushUnboundMarkerNames().

To solve this, we need to flushDeferredBindings() as soon as possible, i.e., straight after ( 1 ).


Side note -- why didn't this crash earlier? Steps 1-9 are the same no matter if reconversion is involved or not...

First, this crashes only if the marker path has not changed and it was the mapper who found a view element with a marker inside and added it to unbound marker names. The mapper can do it only if the marker was not registered to be refreshed in the differ.

Second, this crashes only if an element is removed and it had a marker. Element is removed in four cases: it is removed (duh), it is merged, it is renamed, it is reconverted.

When element is removed and merged, the marker path changes. We listen to that and inform differ that it should be refreshed. When element is renamed, differ itself looks for markers inside such element and refreshes them.

Reconversion is the only mechanism that does not refresh markers in the differ and lets mapper find them and add to unbound marker names. And we use reconversion rarely.

@scofalik scofalik added type:bug This issue reports a buggy (incorrect) behavior. package:engine squad:collaboration Issue to be handled by the Collaboration team. labels Nov 24, 2023
@scofalik scofalik self-assigned this Nov 24, 2023
@scofalik scofalik added the support:2 An issue reported by a commercially licensed client. label Nov 24, 2023
niegowski added a commit that referenced this issue Nov 28, 2023
Fix (engine): Fixed crash happening in a very peculiar scenario involving reconversion of an element containing a marker. Closes #15411.
@CKEditorBot CKEditorBot added this to the iteration 69 milestone Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine squad:collaboration Issue to be handled by the Collaboration team. support:2 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants