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

View#detachDomRoot() might operate on non-existing DOM element #16561

Open
pomek opened this issue Jun 13, 2024 · 3 comments · May be fixed by #17684
Open

View#detachDomRoot() might operate on non-existing DOM element #16561

pomek opened this issue Jun 13, 2024 · 3 comments · May be fixed by #17684
Assignees
Labels
package:engine squad:core Issue to be handled by the Core team. status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. support:2 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@pomek
Copy link
Member

pomek commented Jun 13, 2024

📝 Provide detailed reproduction steps (if any)

There is no easy way to reproduce the issue without a React application. Due to its nature and double rendering in StrictMode, the editor might try to operate on a DOM element that is no longer available.

public detachDomRoot( name: string ): void {
const domRoot = this.domRoots.get( name )!;
// Remove all root attributes so the DOM element is "bare".
Array.from( domRoot.attributes ).forEach( ( { name } ) => domRoot.removeAttribute( name ) );
const initialDomRootAttributes = this._initialDomRootAttributes.get( domRoot );
// Revert all view root attributes back to the state before attachDomRoot was called.
for ( const attribute in initialDomRootAttributes ) {
domRoot.setAttribute( attribute, initialDomRootAttributes[ attribute ] );
}
this.domRoots.delete( name );
this.domConverter.unbindDomElement( domRoot );
for ( const observer of this._observers.values() ) {
observer.stopObserving( domRoot );
}
}

L319, domRoot might be undefined when a DOM element is no longer available.

When the application crashes, it shows the following trace:

TypeError: Cannot read properties of undefined (reading 'attributes')
    at Zc.detachDomRoot (VM3382 ckeditor5.js:5:161689)
    at wC.destroy (VM3382 ckeditor5.js:5:768794)
    at vC.destroy (VM3382 ckeditor5.js:5:771313)

✔️ Expected result

No error. The application should not crash.

❌ Actual result

As reported above.

📃 Other details

  • Related PR: React 19 support ckeditor5-react#480.
  • The following diff resolves all my issues.
     diff --git a/packages/ckeditor5-engine/src/view/view.ts b/packages/ckeditor5-engine/src/view/view.ts
     index 6c1e533122..11477e433e 100644
     --- a/packages/ckeditor5-engine/src/view/view.ts
     +++ b/packages/ckeditor5-engine/src/view/view.ts
     @@ -316,7 +316,11 @@ export default class View extends /* #__PURE__ */ ObservableMixin() {
              * @param name Name of the root to detach.
              */
             public detachDomRoot( name: string ): void {
     -               const domRoot = this.domRoots.get( name )!;
     +               const domRoot = this.domRoots.get( name );
     +
     +               if ( !domRoot ) {
     +                       return;
     +               }
     
                     // Remove all root attributes so the DOM element is "bare".
                     Array.from( domRoot.attributes ).forEach( ( { name } ) => domRoot.removeAttribute( name ) );
    
  • This is not the first time that suppressing TypeSccript possible null values results in unexpected errors in the applications consuming the editor.
@pomek pomek added type:bug This issue reports a buggy (incorrect) behavior. package:engine labels Jun 13, 2024
@Reinmar
Copy link
Member

Reinmar commented Jun 13, 2024

The error mentioned in the original issue is thrown when calling destroy() twice on a single editor instance.

We should secure this method from being called twice. However, we need to warn the integrator that they called it twice. 

Idea:

  • Cache the promise that's returned on the first call of destroy().
  • Return it on subsequent calls.
  • Log a warning that destroy() was called twice and it's most likely a bug in the integration code.

@Mati365
Copy link
Member

Mati365 commented Jun 20, 2024

@Reinmar this error can happen when integration adds new root to the editor but editor is being destroyed before attaching DOM editables to roots. It's quite common in React in StrictMode and conditional rendering of editables. Like this:

const { elements } = useMultiRootEditor( ... );

...

<div>{isEnabled && elements}</div>

@f1ames
Copy link
Contributor

f1ames commented Jul 4, 2024

I stumbled upon this issue when working on new collaboration samples. But for me it happens when leaving "revision history view/editor" back to editor content (both when using "Back to editing" or "Restore this revision") in Classic editor without frameworks. I checked our demos and docs and it doesn't happen there, so either it's some specific setup/configuration or just misconfiguration 🤔 I'll try to create some minimal sample where it is reproducible.

One additional remark - I checked our RTC demo (https://ckeditor.com/collaboration/demo/#real-time-collaboration) for this case. It doesn't happen there, but datacontroller-get-detached-root warning is shown in the console, which sounds related (not sure though).

EDIT: Well, it was just misconfiguration fortunately. When copying list of plugins from import to config I added ClassicEditor there too 😓 🤦 Funny it worked just fine with only this one issue 😄

@FilipTokarski FilipTokarski added support:2 An issue reported by a commercially licensed client. squad:core Issue to be handled by the Core team. labels Nov 29, 2024
@Mati365 Mati365 self-assigned this Dec 20, 2024
@CKEditorBot CKEditorBot added the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine squad:core Issue to be handled by the Core team. status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. support:2 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
6 participants