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.

Sign up for GitHub

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

Jump to bottom

core/scriptloader.js During the uploaded scripts check the scriptLoader checks only 50 per cent of them and doesn’t execute callback. #4282

Closed
aldoom opened this issue Sep 16, 2020 · 2 comments · Fixed by #4283
Closed

core/scriptloader.js During the uploaded scripts check the scriptLoader checks only 50 per cent of them and doesn’t execute callback. #4282

aldoom opened this issue Sep 16, 2020 · 2 comments · Fixed by #4283
Labels
core The issue is caused by the editor core code. status:confirmed An issue confirmed by the development team. type:bug A bug.
Milestone
4.15.1

Comments

@aldoom
Copy link
Contributor

aldoom commented Sep 16, 2020

Type of report

During the uploaded scripts check the scriptLoader checks only 50 per cent of them and doesn’t execute callback.

Provide detailed reproduction steps (if any)

ckeditor4/core/scriptloader.js

Line 61 in 8fccb33

var scriptCount = scriptUrl.length,

ckeditor4/core/scriptloader.js

Lines 155 to 157 in 8fccb33

for ( var i = 0; i < scriptCount; i++ ) {
loadScript( scriptUrl[ i ] );
}

ckeditor4/core/scriptloader.js

Lines 101 to 105 in 8fccb33

var loadScript = function( url ) {
if ( uniqueScripts[ url ] ) {
checkLoaded( url, true );
return;
}

ckeditor4/core/scriptloader.js

Lines 79 to 86 in 8fccb33

var checkLoaded = function( url, success ) {
( success ? completed : failed ).push( url );
if ( --scriptCount <= 0 ) {
showBusy && CKEDITOR.document.getDocumentElement().removeStyle( 'cursor' );
doCallback( success );
}
};

ckeditor4/core/scriptloader.js

Lines 52 to 164 in 8fccb33

load: function( scriptUrl, callback, scope, showBusy ) {
var isString = ( typeof scriptUrl == 'string' );
if ( isString )
scriptUrl = [ scriptUrl ];
if ( !scope )
scope = CKEDITOR;
var scriptCount = scriptUrl.length,
completed = [],
failed = [];
var doCallback = function( success ) {
if ( callback ) {
if ( isString )
callback.call( scope, success );
else
callback.call( scope, completed, failed );
}
};
if ( scriptCount === 0 ) {
doCallback( true );
return;
}
var checkLoaded = function( url, success ) {
( success ? completed : failed ).push( url );
if ( --scriptCount <= 0 ) {
showBusy && CKEDITOR.document.getDocumentElement().removeStyle( 'cursor' );
doCallback( success );
}
};
var onLoad = function( url, success ) {
// Mark this script as loaded.
uniqueScripts[ url ] = 1;
// Get the list of callback checks waiting for this file.
var waitingInfo = waitingList[ url ];
delete waitingList[ url ];
// Check all callbacks waiting for this file.
for ( var i = 0; i < waitingInfo.length; i++ )
waitingInfo[ i ]( url, success );
};
var loadScript = function( url ) {
if ( uniqueScripts[ url ] ) {
checkLoaded( url, true );
return;
}
var waitingInfo = waitingList[ url ] || ( waitingList[ url ] = [] );
waitingInfo.push( checkLoaded );
// Load it only for the first request.
if ( waitingInfo.length > 1 )
return;
// Create the <script> element.
var script = new CKEDITOR.dom.element( 'script' );
script.setAttributes( {
type: 'text/javascript',
src: url
} );
if ( callback ) {
// The onload or onerror event does not fire in IE8 and IE9 Quirks Mode (https://dev.ckeditor.com/ticket/14849).
if ( CKEDITOR.env.ie && ( CKEDITOR.env.version <= 8 || CKEDITOR.env.ie9Compat ) ) {
script.$.onreadystatechange = function() {
if ( script.$.readyState == 'loaded' || script.$.readyState == 'complete' ) {
script.$.onreadystatechange = null;
onLoad( url, true );
}
};
} else {
script.$.onload = function() {
// Some browsers, such as Safari, may call the onLoad function
// immediately. This will break the loading sequence. (https://dev.ckeditor.com/ticket/3661)
setTimeout( function() {
removeListeners( script );
onLoad( url, true );
}, 0 );
};
script.$.onerror = function() {
removeListeners( script );
onLoad( url, false );
};
}
}
// Append it to <head>.
script.appendTo( CKEDITOR.document.getHead() );
CKEDITOR.fire( 'download', url ); // %REMOVE_LINE%
};
showBusy && CKEDITOR.document.getDocumentElement().setStyle( 'cursor', 'wait' );
for ( var i = 0; i < scriptCount; i++ ) {
loadScript( scriptUrl[ i ] );
}
function removeListeners( script ) {
// Once the script loaded or failed to load, remove listeners as this might lead to memory leaks (#589).
script.$.onload = null;
script.$.onerror = null;
}
},

  1. In the beginning of the scripts array processing
    scriptCount = 2;
    i = 0;
  2. After checking the first file
    scriptsCount = 1;
    i = 1;
  3. Next element of the array isn't checked and isn't executed "doCallback( success );"

Expected result

What is the expected result of the above steps?

All the script array elements must be checked and "doCallback( success );" must be executed.

Actual result

What is the actual result of the above steps?

During the uploaded scripts check the scriptLoader checks only 50 per cent of them and doesn’t execute callback.

Other details

  • Browser: all
  • OS: all
  • CKEditor version: all
  • Installed CKEditor plugins: default
@aldoom aldoom added the type:bug A bug. label Sep 16, 2020
@aldoom aldoom mentioned this issue Sep 16, 2020
Merged
3 tasks
@f1ames
Copy link
Contributor

f1ames commented Sep 16, 2020 •
edited
Loading

Thanks for reporting @aldoom, very interesting finding 🤔

How did you notice this issue? Are you using scriptLoader somewhere in your code?

Btw. I wonder if it's not related to our PFW tests failing from time to time because of some filter files not loaded correctly (c @Comandeer)... These tests use scriptLoader from what I see:

ckeditor4/tests/plugins/pastetools/_helpers/ptTools.js

Lines 34 to 57 in aa1e0e7

asyncLoadFilters: function( filters, referrence ) {
return new CKEDITOR.tools.promise( function( resolve, reject ) {
var loaded = 0;
if ( typeof filters === 'string' ) {
filters = [ filters ];
}
for ( var i = 0; i < filters.length; i++ ) {
( function( currentFilter ) {
CKEDITOR.scriptLoader.queue( currentFilter, function( status ) {
if ( !status ) {
reject( 'Couldn\'t load filter: ' + currentFilter );
}
loaded++;
if ( loaded === filters.length ) {
resolve( getFilterByName( referrence ) );
}
} );
}( filters[ i ] ) );
}
} );
},

@f1ames f1ames added core The issue is caused by the editor core code. status:confirmed An issue confirmed by the development team. workload:medium labels Sep 16, 2020
@f1ames
Copy link
Contributor

f1ames commented Sep 25, 2020

Fixed in #4283.

@f1ames f1ames closed this as completed Sep 25, 2020
@f1ames f1ames added this to the 4.15.1 milestone Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
core The issue is caused by the editor core code. status:confirmed An issue confirmed by the development team. type:bug A bug.
Projects
None yet
Milestone
4.15.1
2 participants
@aldoom @f1ames