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

No longer abort already destroyed FileLoader in FileRepository. #17475

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Mati365
Copy link
Member

@Mati365 Mati365 commented Nov 15, 2024

Suggested merge commit message (convention)

Fix (upload): Editor should no longer crash when executing undo while an image is still being uploaded.


Additional information

Closes https://github.com/cksource/ckeditor5-commercial/issues/6817

It looks like undo moves uploaded node to graveyard, which causes abort image uploading here:
https://github.com/ckeditor/ckeditor5/blob/ck/6817/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts#L227

						if ( isInsertedInGraveyard ) {
							// If the image was inserted to the graveyard for good (**not** replaced by another image),
							// only then abort the loading process.
							if ( !insertedImagesIds.has( uploadId ) ) {
								loader.abort();
							}
						} else {

which causes aborted exception, that is caught here:
https://github.com/ckeditor/ckeditor5/blob/ck/6817/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts#L387-L389

			.catch( error => {
				if ( editor.ui ) {
					editor.ui.ariaLiveAnnouncer.announce( t( 'Error during image upload' ) );
				}

				// If status is not 'error' nor 'aborted' - throw error because it means that something else went wrong,
				// it might be generic error and it would be real pain to find what is going on.
				if ( loader.status !== 'error' && loader.status !== 'aborted' ) {
					throw error;
				}

				// Might be 'aborted'.
				if ( loader.status == 'error' && error ) {
					notification.showWarning( error, {
						title: t( 'Upload failed' ),
						namespace: 'upload'
					} );
				}

				// Permanently remove image from insertion batch.
				model.enqueueChange( { isUndoable: false }, writer => {
					writer.remove( imageUploadElements.get( loader.id )! );
				} );

				clean();
			} );

... and tries to abort again, which causes exception. I adjusted FileLoader method to handle similar scenarios, and asseration in image upload.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant