Opened 3 years ago

Closed 3 years ago

#13410 closed Bug (fixed)

[Embedbase, autoembed] Error thrown when undo action follows widget.loadContent

Reported by: Olek Nowodziński Owned by: Szymon Kupś
Priority: Normal Milestone: CKEditor 4.5.2
Component: UI : Widgets Version: 4.5.0
Keywords: Cc:

Description

  1. Go to http://tests.ckeditor.dev:1030/tests/plugins/autoembed/manual/autoembed.
  2. Copy an embed link.
  3. Paste it.
  4. CTRL+Z immediately, before it embeds.

Expected: Embedding is aborted.

Actual: TypeError: Cannot set property 'ready' of null error is thrown

The reason: autoEmbedLink() function calls loadContent() on a temporary widget instance (located in docFragment), which is registered in widgetRepo.instances. When undo is executed, editor's DOM is reloaded and the widget repository demolished, then re–built for the new DOM. Thus when loadContent() callback is executed, there's a brand new widget repository, which does not contain that initial temporary widget instance since it was located in docFragment. Finally editor.widgets.finalizeCreation( temp ) is called. However, because temp belongs to a widget instance, which is no longer registered in the repository, the widget is not found and the error is thrown.

Cake recipe: The fix should land in embedbase. loadContent should check if the widget still belongs to editor's widget repository first.

To–do: Check if the fix covers the case when "Source" is pressed instead of "Undo".

Change History (17)

comment:1 Changed 3 years ago by Olek Nowodziński

Summary: [Embedbase, autoembed] Error thrown when undo action follows window.loadContent[Embedbase, autoembed] Error thrown when undo action follows widget.loadContent

comment:2 Changed 3 years ago by Szymon Kupś

Owner: set to Szymon Kupś
Status: newassigned

comment:3 Changed 3 years ago by Szymon Kupś

Status: assignedreview

comment:4 Changed 3 years ago by Szymon Kupś

Status: reviewassigned

comment:5 Changed 3 years ago by Szymon Kupś

Status: assignedreview

Pushed ​branch:t/13410.

comment:6 Changed 3 years ago by Olek Nowodziński

Status: reviewreview_failed

Rebased the branch on master.

But other than that, it's alright!

comment:7 Changed 3 years ago by Olek Nowodziński

As for wait(), it turned out to be necessary because request mock uses hardcoded setTimeout. But I think it's possible to avoid magic numbers (200) like that:

'test if embedding is cancelled when widget is no longer valid': function() {
	var bot = this.editorBots.inline,
		editor = bot.editor,
		successCallbackSpy = sinon.spy(),
		errorCallbackSpy = sinon.spy();
 
	bot.setData( dataWithWidget, function() {
		var widget = getWidgetById( editor, 'w1' ),
			task;
 
		jsonpCallback = function( urlTemplate, urlParams, callback ) {
			resume( function() {
				callback( {
					type: 'rich',
					html: '<p>url:' + urlParams.url + '</p>'
				} );
 
				assert.isTrue( task.isDone(), 'task.isDone' );
				assert.isFalse( successCallbackSpy.called, 'successCallbackSpy.called' );
				assert.isFalse( errorCallbackSpy.called, 'errorCallbackSpy.called' );
			} );
		};
 
		widget.on( 'sendRequest', function( evt ) {
			task = evt.data.task;
		} );
 
		widget.loadContent( '//canceling/handleresponse', {
			callback: successCallbackSpy,
			errorCallback: errorCallbackSpy
		} );
 
		editor.widgets.destroy( widget );
 
		wait();
	} );
}

At least assertions fail if the patch is undone ;)

comment:8 Changed 3 years ago by Szymon Kupś

Status: review_failedreview
  • editor.widgets.destroy is called before finalizeCreation - it should be called with 'offline = true'.
  • Instead of using try, catch and finally I've changed order of execution. Sinon.js stub remembers information about calls even after 'restore()' method is called.
  • Typo fixed.
  • Fixed wait to proposed solution without magic numbers.
  • Added assert that checks if request failure fires 'errorCallback'. Assert checks if instanceDestroyed event was fired during failed request.

Pushed ​​branch:t/13410.

comment:9 Changed 3 years ago by Olek Nowodziński

Status: reviewreview_failed

I pushed some minor fixes to branch:t/13410. Still there's one scenario that fails:

  1. Go to ​http://tests.ckeditor.dev:1030/tests/plugins/autoembed/manual/autoembed.
  2. Copy an embed link.
  3. Paste it.
  4. Paste it.
  5. Paste it.

Note: Paste fast, before predecessor gets embedded.

Expected: 3 link are embedded.

Actual: 1 link is embedded, others are discarded with '[CKEDITOR.plugins.embedBase.baseDefinition.loadContent] Widget no longer belongs to current editor\'s widgets list.'

comment:10 Changed 3 years ago by Szymon Kupś

Status: review_failedassigned

comment:11 Changed 3 years ago by Szymon Kupś

Status: assignedreview

There was a problem connected with checkWidgets method. Every time link for embedding was pasted afterInsertHtml event was fired and checkWidgets method was called. It removes all widgets instances that have no representation in DOM tree. This was causing that not yet fully loaded widgets were removed from widgets repository and then loading was aborted.

Even without fix for this ticket, this behavior is not desired. It creates situation that after pasting couple of links one after another all widgets are loaded but widget repository contains only last loaded instance.

Proposed solution checks if widget is ready before removing it from widgets repository.

Pushed branch:t/13410.

comment:12 Changed 3 years ago by Olek Nowodziński

Status: reviewreview_failed

I'm sorry to say that but https://github.com/cksource/ckeditor-dev/commit/96224e05fc9ca8ddccdc3d979a28dcfca95a15cb#diff-d932030cd4e22ec8b0e30a362269b7e8R1993 needs test(s) :D

The right place is http://tests.ckeditor.dev:1030/tests/plugins/widget/widgetsrepoapi. There's a number of widgets.checkWidgets* tests already there and a new test with a simple simulation like in widgets.finalizeCreation* should be enough.

comment:13 Changed 3 years ago by Szymon Kupś

Status: review_failedreview

Added test to tests/plugins/widget/widgetsrepoapi.

Pushed branch:t/13410.

comment:14 Changed 3 years ago by Olek Nowodziński

Status: reviewreview_passed

Still waiting for Reinmar to chime in regarding changes in Widget plugin.

comment:15 Changed 3 years ago by Piotrek Koszuliński

Chiming in...

comment:16 Changed 3 years ago by Piotrek Koszuliński

The change in checkWidgets looks ok. It may cause some memory leaks, but not due to itself being incorrect, but other plugins carelessly initialising widgets. Autoembed is not one of them because it correctly destroys the widget if embedding fails for some reason.

comment:17 Changed 3 years ago by Piotrek Koszuliński

Resolution: fixed
Status: review_passedclosed

Fixed on master with git:b7e85c0.

Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy