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

[Olek Nowodziński]

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".

[Szymon Kupś]

Pushed ​branch:t/13410.

[Szymon Kupś]

Pushed ​​branch:t/13410.

[Olek Nowodziński]

Rebased the branch on master.

• Correct me if I'm wrong but editor.widgets.destroy could be called with offline = 1 argument to speed up things ​https://github.com/ckeditor/ckeditor-dev/compare/d841e85d251b7...cksource:773f865#diff-062c6e53e516cb38294d117abe666254R98 since the widget is detached anyway.
• If this ​https://github.com/ckeditor/ckeditor-dev/compare/d841e85d251b7...cksource:773f865#diff-ee14e4c82fe1a3b638eb978a1e61f86bR240 fails, finalizeCreation is never restored. It will have an impact on tests that would follow in the future. try, catch, finally should do the trick.
• Typo ​https://github.com/ckeditor/ckeditor-dev/compare/d841e85d251b7...cksource:773f865#diff-ac3d886653b04d08fe92c357d8232728R478 :P
• Why wait() in ​https://github.com/ckeditor/ckeditor-dev/compare/d841e85d251b7...cksource:773f865#diff-ac3d886653b04d08fe92c357d8232728R499? Checked briefly on Chrome and it worked synchronously. Some other browser? Other reason?
• What's the tests coverage for errorCallback ​https://github.com/ckeditor/ckeditor-dev/compare/d841e85d251b7...cksource:773f865#diff-062c6e53e516cb38294d117abe666254R97? Because if I removed it from code tests remain all green :D We need to create (extend) a test to check if what happens inside errorCallback really does happen. I suppose the method matters mostly in terms of garbage collection but at least we got to be sure it works.

But other than that, it's alright!

[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 ;)

[Szymon Kupś]

• 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.

[Olek Nowodziński]

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.'

[Szymon Kupś]

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.

[Olek Nowodziński]

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.

[Szymon Kupś]

Added test to tests/plugins/widget/widgetsrepoapi.

Pushed ​branch:t/13410.

[Olek Nowodziński]

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

[Piotrek Koszuliński]

Chiming in...

[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.

[Piotrek Koszuliński]

Fixed on master with ​git:b7e85c0.