Uploaded image for project: 'Apache Cordova'
  1. Apache Cordova
  2. CB-13804

Always log a message when catching an exception

    XMLWordPrintableJSON

    Details

      Description

      Looking at cordova.js in cordova-windows along with the other platforms I noticed that the JavaScript logs a message in most but not all catch blocks. Here are a few exceptions I spotted so far:

      In cordova-js src/common/builder.js:

      function include (parent, objects, clobber, merge) {
          each(objects, function (obj, key) {
              try {
                  var result = obj.path ? require(obj.path) : {};
      
                  // ...
              } catch (e) {
                  utils.alert('Exception building Cordova JS globals: ' + e + ' for key "' + key + '"');
              }
          });
      }
      

      utils.alert would show an alert if possible, otherwise log a message. I would rather to see the JavaScript log the message regardless of whether or not it is possible to show the alert (ideally log first). Possible fix (NOT TESTED):

      diff --git a/src/common/utils.js b/src/common/utils.js
      index febfd91..7244b8f 100644
      --- a/src/common/utils.js
      +++ b/src/common/utils.js
      @@ -170,12 +170,9 @@ utils.extend = (function () {
       }());
       
       /**
      - * Alerts a message in any available way: alert or console.log.
      + * Alerts a message in any possible way: alert / console.log
        */
       utils.alert = function (msg) {
      -    if (window.alert) {
      -        window.alert(msg);
      -    } else if (console && console.log) {
      -        console.log(msg);
      -    }
      +    console && console.log && console.log(msg);
      +    window.alert && window.alert(msg);
       };
      

      Possible fixes in cordova-windows (NOT TESTED):

      diff --git a/cordova-js-src/confighelper.js b/cordova-js-src/confighelper.js
      index c166052..faa65e3 100644
      --- a/cordova-js-src/confighelper.js
      +++ b/cordova-js-src/confighelper.js
      @@ -89,7 +89,11 @@ function requestFile(filePath, success, error) {
               xhr.open("get", filePath, true);
               xhr.send();
           } catch (e) {
      -        fail('[Windows][cordova.js][xhrFile] Could not XHR ' + filePath + ': ' + JSON.stringify(e));
      +        var msg =
      +            '[Windows][cordova.js][xhrFile] Could not XHR ' + filePath + ': ' +
      +                JSON.stringify(e);
      +        console.error(msg);
      +        fail(msg);
           }
       }
       
      diff --git a/cordova-js-src/platform.js b/cordova-js-src/platform.js
      index 4bc4025..dbc1c75 100644
      --- a/cordova-js-src/platform.js
      +++ b/cordova-js-src/platform.js
      @@ -99,7 +99,9 @@ module.exports = {
                   try {
                       Windows.UI.ViewManagement.ApplicationView.getForCurrentView();
                       isCoreWindowAvailable = true;
      -            } catch (e) { }
      +            } catch (e) {
      +                console && console.log && console.log('NOTICE: CoreWindow functionality is not available');
      +            }
       
                   if (isCoreWindowAvailable) {
                       app.addEventListener("checkpoint", checkpointHandler);
      @@ -160,6 +162,7 @@ function injectBackButtonHandler() {
                       return true;
                   }
                   catch (e) {
      +                console && console.log && console.log('NOTICE: backbutton handler not available, ignored');
                       return false;
                   }
               }
      

      I would need some time to check for unlogged exceptions on the other platforms.

      A case where I think logging should NOT be done in release build is in the clobber funciton in cordova-js/src/common/builder.js:

      function clobber (obj, key, value) {
          exports.replaceHookForTesting(obj, key);
          var needsProperty = false;
          try {
              obj[key] = value;
          } catch (e) {
              needsProperty = true;
          }
          // ...
      

      I originally spotted this issue on Windows when looking at the ApplicationView calls related to the changes in CB-12238, CB-12784, and CB-13641 along with suggested changes I raised in CB-13802.

        Attachments

          Activity

            People

            • Assignee:
              bowserj Joey Robert Bowser
              Reporter:
              brodybits Chris Brody
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated: