From fadc27f068680456db5aa4d1434a94d042d9450c Mon Sep 17 00:00:00 2001 From: mathiasscheffe <62892503+mathiasscheffe@users.noreply.github.com> Date: Mon, 30 Mar 2020 16:37:57 +0200 Subject: [PATCH 1/6] Stable error handling for loaded Cordova plugins Platforms affected All Motivation and Context When developing a large Cordova app in a distributed team occasionally a developer commits a Cordova plugin which contains an issue which directly results in a javaScript exception on plugin load. Before the fix the Cordova startup crashes and the app results in a whitescreen. Description The fix makes handlePluginsObject more stable. The function keeps track of the loaded modules. If a load error occurs for a module the module is removed from the modules list. Additionally an alert is displayed. This makes it way easier for developers to track and fix the error. --- src/common/pluginloader.js | 52 +++++++++++++++++++++++++++++++++++--- 1 file changed, 49 insertions(+), 3 deletions(-) diff --git a/src/common/pluginloader.js b/src/common/pluginloader.js index 3cc55a6cc..9a9fc86ae 100644 --- a/src/common/pluginloader.js +++ b/src/common/pluginloader.js @@ -42,7 +42,7 @@ function injectIfNecessary (id, url, onload, onerror) { if (id in define.moduleMap) { onload(); } else { - onerror(); + onerror("Module not inserted to module map."); } }, onerror); } @@ -79,19 +79,65 @@ function onScriptLoadingComplete (moduleList, finishPluginLoading) { function handlePluginsObject (path, moduleList, finishPluginLoading) { // Now inject the scripts. var scriptCounter = moduleList.length; + var modulesWithProblems = []; if (!scriptCounter) { finishPluginLoading(); return; } + + function callOnScriptLoadingComplete(){ + modulesWithProblems.forEach(function(elem){ + + var foundIndex = -1; + var modulesLength = moduleList.length; + var i = 0; + // find module with load problems in module list. Use while loop to exit iteration early. + while(i < modulesLength && foundIndex === -1) { + if(moduleList[i].id === elem) { + foundIndex = i; + } + i++; + } + if(foundIndex !== -1) { + // delete module with load problem from module list + moduleList.splice(foundIndex, 1); + } + + } ); + onScriptLoadingComplete(moduleList, finishPluginLoading); + } + function scriptLoadedCallback () { if (!--scriptCounter) { - onScriptLoadingComplete(moduleList, finishPluginLoading); + callOnScriptLoadingComplete(); + } + } + + function scriptLoadedErrorCallback(id, message, source, lineno, colno, error) { + modulesWithProblems.push(id); + if(typeof message !== "undefined") { + var messageString = message; + if(typeof message !== "string") { + messageString = JSON.stringify(message); + } + messageString = "Could not load all functions. Please confirm or restart your application. \n \n"+ + "Details: Error while loading module: '"+id+"'. Module will be skipped. " + messageString; + console.error(messageString); + alert(messageString); + } + if (!--scriptCounter) { + callOnScriptLoadingComplete(); } } for (var i = 0; i < moduleList.length; i++) { - injectIfNecessary(moduleList[i].id, path + moduleList[i].file, scriptLoadedCallback); + var moduleId = moduleList[i].id; + var me = this; + // bound function to have the module id when the error occurs. + var boundErrorCallback = scriptLoadedErrorCallback.bind(me, moduleId); + injectIfNecessary(moduleId, path + moduleList[i].file, + scriptLoadedCallback, boundErrorCallback); } } From de8dab89ecaa2575fbdbf3b9edea7debb23d29db Mon Sep 17 00:00:00 2001 From: mathiasscheffe <62892503+mathiasscheffe@users.noreply.github.com> Date: Mon, 30 Mar 2020 17:24:22 +0200 Subject: [PATCH 2/6] Fixed overlooked linter errors --- src/common/pluginloader.js | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/src/common/pluginloader.js b/src/common/pluginloader.js index 9a9fc86ae..083edc883 100644 --- a/src/common/pluginloader.js +++ b/src/common/pluginloader.js @@ -42,7 +42,7 @@ function injectIfNecessary (id, url, onload, onerror) { if (id in define.moduleMap) { onload(); } else { - onerror("Module not inserted to module map."); + onerror('Module not inserted to module map.'); } }, onerror); } @@ -86,25 +86,23 @@ function handlePluginsObject (path, moduleList, finishPluginLoading) { return; } - function callOnScriptLoadingComplete(){ - modulesWithProblems.forEach(function(elem){ - + function callOnScriptLoadingComplete () { + modulesWithProblems.forEach(function (elem) { var foundIndex = -1; var modulesLength = moduleList.length; var i = 0; // find module with load problems in module list. Use while loop to exit iteration early. - while(i < modulesLength && foundIndex === -1) { - if(moduleList[i].id === elem) { + while (i < modulesLength && foundIndex === -1) { + if (moduleList[i].id === elem) { foundIndex = i; } i++; } - if(foundIndex !== -1) { + if (foundIndex !== -1) { // delete module with load problem from module list moduleList.splice(foundIndex, 1); } - - } ); + }); onScriptLoadingComplete(moduleList, finishPluginLoading); } @@ -114,15 +112,15 @@ function handlePluginsObject (path, moduleList, finishPluginLoading) { } } - function scriptLoadedErrorCallback(id, message, source, lineno, colno, error) { + function scriptLoadedErrorCallback (id, message, source, lineno, colno, error) { modulesWithProblems.push(id); - if(typeof message !== "undefined") { + if (typeof message !== 'undefined') { var messageString = message; - if(typeof message !== "string") { + if (typeof message !== 'string') { messageString = JSON.stringify(message); } - messageString = "Could not load all functions. Please confirm or restart your application. \n \n"+ - "Details: Error while loading module: '"+id+"'. Module will be skipped. " + messageString; + messageString = 'Could not load all functions. Please confirm or restart your application. \n \n' + + 'Details: Error while loading module: "' + id + '". Module will be skipped. ' + messageString; console.error(messageString); alert(messageString); } @@ -136,7 +134,7 @@ function handlePluginsObject (path, moduleList, finishPluginLoading) { var me = this; // bound function to have the module id when the error occurs. var boundErrorCallback = scriptLoadedErrorCallback.bind(me, moduleId); - injectIfNecessary(moduleId, path + moduleList[i].file, + injectIfNecessary(moduleId, path + moduleList[i].file, scriptLoadedCallback, boundErrorCallback); } } From 55918455ae76e4dcaa5bffbd85b06dbb2a2c3046 Mon Sep 17 00:00:00 2001 From: mathiasscheffe <62892503+mathiasscheffe@users.noreply.github.com> Date: Thu, 9 Apr 2020 13:19:45 +0200 Subject: [PATCH 3/6] Update src/common/pluginloader.js MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This pointer not needed. Therefore not bound. Co-Authored-By: Raphael von der Grün --- src/common/pluginloader.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/common/pluginloader.js b/src/common/pluginloader.js index 083edc883..53a98d992 100644 --- a/src/common/pluginloader.js +++ b/src/common/pluginloader.js @@ -131,9 +131,8 @@ function handlePluginsObject (path, moduleList, finishPluginLoading) { for (var i = 0; i < moduleList.length; i++) { var moduleId = moduleList[i].id; - var me = this; // bound function to have the module id when the error occurs. - var boundErrorCallback = scriptLoadedErrorCallback.bind(me, moduleId); + var boundErrorCallback = scriptLoadedErrorCallback.bind(null, moduleId); injectIfNecessary(moduleId, path + moduleList[i].file, scriptLoadedCallback, boundErrorCallback); } From d24ee061210078ae4d79ad55ab2e36946a66abfb Mon Sep 17 00:00:00 2001 From: mathiasscheffe <62892503+mathiasscheffe@users.noreply.github.com> Date: Thu, 9 Apr 2020 13:38:35 +0200 Subject: [PATCH 4/6] Update src/common/pluginloader.js MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Raphael von der Grün --- src/common/pluginloader.js | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-) diff --git a/src/common/pluginloader.js b/src/common/pluginloader.js index 53a98d992..663c6eec5 100644 --- a/src/common/pluginloader.js +++ b/src/common/pluginloader.js @@ -87,23 +87,10 @@ function handlePluginsObject (path, moduleList, finishPluginLoading) { } function callOnScriptLoadingComplete () { - modulesWithProblems.forEach(function (elem) { - var foundIndex = -1; - var modulesLength = moduleList.length; - var i = 0; - // find module with load problems in module list. Use while loop to exit iteration early. - while (i < modulesLength && foundIndex === -1) { - if (moduleList[i].id === elem) { - foundIndex = i; - } - i++; - } - if (foundIndex !== -1) { - // delete module with load problem from module list - moduleList.splice(foundIndex, 1); - } + var loadedModules = moduleList.filter(function (m) { + return modulesWithProblems.indexOf(m.id) === -1; }); - onScriptLoadingComplete(moduleList, finishPluginLoading); + onScriptLoadingComplete(loadedModules, finishPluginLoading); } function scriptLoadedCallback () { From 8893e55214d4aaf75c967611407b5228319a4e3c Mon Sep 17 00:00:00 2001 From: "Scheffe, Mathias" Date: Mon, 20 Apr 2020 09:05:17 +0200 Subject: [PATCH 5/6] Removed alert --- src/common/pluginloader.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/common/pluginloader.js b/src/common/pluginloader.js index 663c6eec5..e618a5ebf 100644 --- a/src/common/pluginloader.js +++ b/src/common/pluginloader.js @@ -109,7 +109,8 @@ function handlePluginsObject (path, moduleList, finishPluginLoading) { messageString = 'Could not load all functions. Please confirm or restart your application. \n \n' + 'Details: Error while loading module: "' + id + '". Module will be skipped. ' + messageString; console.error(messageString); - alert(messageString); + // use this comment as search & replace marker to insert a more app specific error handling in your after_platform_add hook. + // Decide if the app can start even if plugin loading of a specific plugin has failed. } if (!--scriptCounter) { callOnScriptLoadingComplete(); From e0add7baf6957b383867bbf3151ea3b027d125a5 Mon Sep 17 00:00:00 2001 From: "Scheffe, Mathias" Date: Thu, 23 Apr 2020 13:30:03 +0200 Subject: [PATCH 6/6] implemented unit tests for improved error handling --- src/common/pluginloader.js | 3 +-- test/test.pluginloader.js | 28 ++++++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/src/common/pluginloader.js b/src/common/pluginloader.js index e618a5ebf..45df21407 100644 --- a/src/common/pluginloader.js +++ b/src/common/pluginloader.js @@ -106,8 +106,7 @@ function handlePluginsObject (path, moduleList, finishPluginLoading) { if (typeof message !== 'string') { messageString = JSON.stringify(message); } - messageString = 'Could not load all functions. Please confirm or restart your application. \n \n' + - 'Details: Error while loading module: "' + id + '". Module will be skipped. ' + messageString; + messageString = 'Could not load all functions. Error while loading module: "' + id + '". Module will be skipped. ' + messageString; console.error(messageString); // use this comment as search & replace marker to insert a more app specific error handling in your after_platform_add hook. // Decide if the app can start even if plugin loading of a specific plugin has failed. diff --git a/test/test.pluginloader.js b/test/test.pluginloader.js index 72c37876e..80d3fa02d 100644 --- a/test/test.pluginloader.js +++ b/test/test.pluginloader.js @@ -105,4 +105,32 @@ describe('pluginloader', function () { done(); }); }); + it('Test#005 : If injecting a module causes an error then the module is skipped and plugin loading continues', function (done) { + define('cordova/plugin_list', function (require, exports, module) { + module.exports = [ + { file: 'some/pathToScriptWithErrors.js', id: 'some.id.whichWillNotLoad' }, + { file: 'some/path.js', id: 'some.id' } + ]; + }); + spyOn(console, 'error'); + injectScript.and.callFake(function (url, onload, onerror) { + if (url.indexOf('some/pathToScriptWithErrors.js') > -1) { + // fake a load error for one module. + onerror('An error message.'); + } else { + // modules need to be inserted to the modulemap otherwise plugin loading will fail + define('some.id', function (require, exports, module) { + }); + onload(); + } + }); + + pluginloader.load(() => { + expect(console.error).toHaveBeenCalledWith('Could not load all functions. Error while loading module: "some.id.whichWillNotLoad". Module will be skipped. An error message.'); + var moduleMap = define.moduleMap; + expect(moduleMap['some.id']).toBeDefined(); + expect(moduleMap['some.id.whichWillNotLoad']).not.toBeDefined(); + done(); + }); + }); });