[gjs/wip/ptomato/mozjs38: 2/22] js: Be careful about undefined property accesses

[Philip Chimento <pchimento src gnome org>]

commit ab399bf143bcacc9ba597166e5beb82ef547ac8c
Author: Philip Chimento <philip chimento gmail com>
Date:   Sun Jan 29 22:38:41 2017 -0800

    js: Be careful about undefined property accesses
    
    SpiderMonkey 38 warns rather more loudly if you access an undefined
    property. This adds checks to all the places where a normal run of the
    test suite would generate the warning.
    
    On the C++ side, the checks are done with JS_AlreadyHasOwnProperty() in
    order not to re-enter the whole resolve -> get machinery. This also adds
    a comment to gjs_object_require_property() stating not to use it if the
    property is not actually required, which is now the best practice.
    
    On the JS side, we simply write our JS code a bit more carefully.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=776966

 gi/repo.cpp                  |   11 +++++---
 gjs/importer.cpp             |   57 ++++++++++++++++++++++++++++-------------
 gjs/jsapi-util.cpp           |    3 ++
 gjs/jsapi-util.h             |   54 +++++++++++++++++++++------------------
 modules/coverage.js          |   13 ++-------
 modules/overrides/GObject.js |   24 +++++++++---------
 6 files changed, 93 insertions(+), 69 deletions(-)
---
diff --git a/gi/repo.cpp b/gi/repo.cpp
index ccb845e..d026c13 100644
--- a/gi/repo.cpp
+++ b/gi/repo.cpp
@@ -64,19 +64,22 @@ get_version_for_ns (JSContext       *context,
                     char           **version)
 {
     JS::RootedObject versions(context);
+    bool found;
 
     if (!gjs_object_require_property(context, repo_obj,
                                      "GI repository object",
                                      GJS_STRING_GI_VERSIONS, &versions))
         return false;
 
-    if (!gjs_object_require_property(context, versions, NULL, ns_id, version)) {
-        /* Property not actually required, so clear an exception */
-        JS_ClearPendingException(context);
+    if (!JS_AlreadyHasOwnPropertyById(context, versions, ns_id, &found))
+        return false;
+
+    if (!found) {
         *version = NULL;
+        return true;
     }
 
-    return true;
+    return gjs_object_require_property(context, versions, NULL, ns_id, version);
 }
 
 static bool
diff --git a/gjs/importer.cpp b/gjs/importer.cpp
index 4c93041..bc5b882 100644
--- a/gjs/importer.cpp
+++ b/gjs/importer.cpp
@@ -423,6 +423,42 @@ load_module_elements(JSContext        *cx,
     }
 }
 
+/* If error, returns false. If not found, returns true but does not touch
+ * the value at *result. If found, returns true and sets *result = true.
+ */
+static bool
+import_symbol_from_init_js(JSContext       *cx,
+                           JS::HandleObject importer,
+                           const char      *dirname,
+                           const char      *name,
+                           bool            *result)
+{
+    bool found;
+    g_autofree char *full_path = g_build_filename(dirname, MODULE_INIT_FILENAME,
+                                                  NULL);
+
+    JS::RootedObject module_obj(cx, load_module_init(cx, importer, full_path));
+    if (!module_obj || !JS_AlreadyHasOwnProperty(cx, module_obj, name, &found))
+        return false;
+
+    if (!found)
+        return true;
+
+    JS::RootedValue obj_val(cx);
+    if (!JS_GetProperty(cx, module_obj, name, &obj_val))
+        return false;
+
+    if (obj_val.isUndefined())
+        return true;
+
+    if (!JS_DefineProperty(cx, importer, name, obj_val,
+                           GJS_MODULE_PROP_FLAGS & ~JSPROP_PERMANENT))
+        return false;
+
+    *result = true;
+    return true;
+}
+
 static bool
 import_file_on_module(JSContext       *context,
                       JS::HandleObject obj,
@@ -499,7 +535,6 @@ do_import(JSContext       *context,
     directories = NULL;
 
     JS::RootedValue elem(context);
-    JS::RootedObject module_obj(context);
 
     /* First try importing an internal module like byteArray */
     if (priv->is_root &&
@@ -539,23 +574,9 @@ do_import(JSContext       *context,
             continue;
 
         /* Try importing __init__.js and loading the symbol from it */
-        if (full_path)
-            g_free(full_path);
-        full_path = g_build_filename(dirname, MODULE_INIT_FILENAME,
-                                     NULL);
-
-        module_obj.set(load_module_init(context, obj, full_path));
-        if (module_obj != NULL) {
-            JS::RootedValue obj_val(context);
-            if (JS_GetProperty(context, module_obj, name, &obj_val)) {
-                if (!obj_val.isUndefined() &&
-                    JS_DefineProperty(context, obj, name, obj_val,
-                                      GJS_MODULE_PROP_FLAGS & ~JSPROP_PERMANENT)) {
-                    result = true;
-                    goto out;
-                }
-            }
-        }
+        import_symbol_from_init_js(context, obj, dirname, name, &result);
+        if (result)
+            goto out;
 
         /* Second try importing a directory (a sub-importer) */
         if (full_path)
diff --git a/gjs/jsapi-util.cpp b/gjs/jsapi-util.cpp
index 4f7204f..0cc78de 100644
--- a/gjs/jsapi-util.cpp
+++ b/gjs/jsapi-util.cpp
@@ -217,6 +217,9 @@ throw_property_lookup_error(JSContext       *cx,
  * Guarantees that *value_p is set to something, if only JS::UndefinedValue(),
  * even if an exception is set and false is returned.
  *
+ * SpiderMonkey will emit a warning if the property is not present, so don't
+ * use this if you expect the property not to be present some of the time.
+ *
  * Requires request.
  */
 bool
diff --git a/gjs/jsapi-util.h b/gjs/jsapi-util.h
index f89060c..d41f45d 100644
--- a/gjs/jsapi-util.h
+++ b/gjs/jsapi-util.h
@@ -197,32 +197,36 @@ gjs_##cname##_create_proto(JSContext *context,                                 \
     JS::RootedObject global(context, gjs_get_import_global(context));          \
     JS::RootedId class_name(context,                                           \
         gjs_intern_string_to_id(context, gjs_##cname##_class.name));           \
-    if (!JS_GetPropertyById(context, global, class_name, &rval))               \
+    bool found = false;                                                        \
+    if (!JS_AlreadyHasOwnPropertyById(context, global, class_name, &found))    \
         return JS::NullValue(); \
-    if (rval.isUndefined()) { \
-        JS::RootedObject prototype(context,                                    \
-            JS_InitClass(context, global, parent, &gjs_##cname##_class, ctor,  \
-                         0, &gjs_##cname##_proto_props[0],                     \
-                         &gjs_##cname##_proto_funcs[0],                        \
-                         NULL, NULL));                                         \
-        if (prototype == NULL) { \
-            return JS::NullValue(); \
-        } \
-        if (!gjs_object_require_property( \
-                context, global, NULL, \
-                class_name, &rval)) { \
-            return JS::NullValue(); \
-        } \
-        if (!JS_DefineProperty(context, module, proto_name, \
-                               rval, GJS_MODULE_PROP_FLAGS))                   \
-            return JS::NullValue(); \
-        if (gtype != G_TYPE_NONE) { \
-            JS::RootedObject rval_obj(context, &rval.toObject());              \
-            JS::RootedObject gtype_obj(context,                                \
-                gjs_gtype_create_gtype_wrapper(context, gtype));               \
-            JS_DefineProperty(context, rval_obj, "$gtype", gtype_obj,          \
-                              JSPROP_PERMANENT);                               \
-        } \
+    if (found) {                                                               \
+        if (!JS_GetPropertyById(context, global, class_name, &rval))           \
+            return JS::NullValue();                                            \
+        return rval;                                                           \
+    }                                                                          \
+    JS::RootedObject prototype(context,                                        \
+        JS_InitClass(context, global, parent, &gjs_##cname##_class, ctor,      \
+                     0, &gjs_##cname##_proto_props[0],                         \
+                     &gjs_##cname##_proto_funcs[0],                            \
+                     NULL, NULL));                                             \
+    if (prototype == NULL) { \
+        return JS::NullValue(); \
+    } \
+    if (!gjs_object_require_property( \
+            context, global, NULL, \
+            class_name, &rval)) { \
+        return JS::NullValue(); \
+    } \
+    if (!JS_DefineProperty(context, module, proto_name, \
+                           rval, GJS_MODULE_PROP_FLAGS))                       \
+        return JS::NullValue(); \
+    if (gtype != G_TYPE_NONE) { \
+        JS::RootedObject rval_obj(context, &rval.toObject());                  \
+        JS::RootedObject gtype_obj(context,                                    \
+            gjs_gtype_create_gtype_wrapper(context, gtype));                   \
+        JS_DefineProperty(context, rval_obj, "$gtype", gtype_obj,              \
+                          JSPROP_PERMANENT);                                   \
     } \
     return rval; \
 }
diff --git a/modules/coverage.js b/modules/coverage.js
index 636ce1a..d01b378 100644
--- a/modules/coverage.js
+++ b/modules/coverage.js
@@ -417,6 +417,8 @@ function _expressionLinesToCounters(expressionLines, nLines) {
         return counters;
 
     for (let i = 1; i < counters.length; i++) {
+        if (!expressionLines.hasOwnProperty(expressionLinesIndex))
+            continue;
         if (expressionLines[expressionLinesIndex] == i) {
             counters[i] = 0;
             expressionLinesIndex++;
@@ -773,12 +775,6 @@ function CoverageStatisticsContainer(prefixes, cache) {
     let coveredFiles = {};
     let cacheMisses = 0;
 
-    function wantsStatisticsFor(filename) {
-        return prefixes.some(function(prefix) {
-            return filename.startsWith(prefix);
-        });
-    }
-
     function createStatisticsFor(filename) {
         let contents = getFileContents(filename);
         let nLines = _getNumberOfLinesForScript(contents);
@@ -799,10 +795,7 @@ function CoverageStatisticsContainer(prefixes, cache) {
     }
 
     function ensureStatisticsFor(filename) {
-        let wantStatistics = wantsStatisticsFor(filename);
-        let haveStatistics = !!coveredFiles[filename];
-
-        if (wantStatistics && !haveStatistics)
+        if (!coveredFiles[filename])
             coveredFiles[filename] = createStatisticsFor(filename);
         return coveredFiles[filename];
     }
diff --git a/modules/overrides/GObject.js b/modules/overrides/GObject.js
index 62c02d2..4fdf878 100644
--- a/modules/overrides/GObject.js
+++ b/modules/overrides/GObject.js
@@ -42,22 +42,22 @@ function _createSignals(gtype, signals) {
     }
 }
 
-function _createGTypeName(name, gtypename) {
-    if (gtypename)
-        return gtypename;
+function _createGTypeName(params) {
+    if (params.GTypeName)
+        return params.GTypeName;
     else
-        return 'Gjs_' + name;
+        return 'Gjs_' + params.Name;
 }
 
 function _getGObjectInterfaces(interfaces) {
     return interfaces.filter((iface) => iface.hasOwnProperty('$gtype'));
 }
 
-function _propertiesAsArray(propertyObj) {
+function _propertiesAsArray(params) {
     let propertiesArray = [];
-    if (propertyObj) {
-        for (let prop in propertyObj) {
-            propertiesArray.push(propertyObj[prop]);
+    if (params.Properties) {
+        for (let prop in params.Properties) {
+            propertiesArray.push(params.Properties[prop]);
         }
     }
     return propertiesArray;
@@ -124,7 +124,7 @@ const GObjectMeta = new Lang.Class({
             throw new TypeError("Classes require an explicit 'Name' parameter.");
         let name = params.Name;
 
-        let gtypename = _createGTypeName(params.Name, params.GTypeName);
+        let gtypename = _createGTypeName(params);
 
         if (!params.Extends)
             params.Extends = GObject.Object;
@@ -138,7 +138,7 @@ const GObjectMeta = new Lang.Class({
             interfaces = interfaces.filter((iface) => !parent.implements(iface));
         let gobjectInterfaces = _getGObjectInterfaces(interfaces);
 
-        let propertiesArray = _propertiesAsArray(params.Properties);
+        let propertiesArray = _propertiesAsArray(params);
         delete params.Properties;
 
         let newClassConstructor = Gi.register_type(parent.prototype, gtypename,
@@ -197,13 +197,13 @@ GObjectInterface.prototype._construct = function (params) {
         throw new TypeError("Interfaces require an explicit 'Name' parameter.");
     }
 
-    let gtypename = _createGTypeName(params.Name, params.GTypeName);
+    let gtypename = _createGTypeName(params);
     delete params.GTypeName;
 
     let interfaces = params.Requires || [];
     let gobjectInterfaces = _getGObjectInterfaces(interfaces);
 
-    let properties = _propertiesAsArray(params.Properties);
+    let properties = _propertiesAsArray(params);
     delete params.Properties;
 
     let newInterfaceConstructor = Gi.register_interface(gtypename,