njs: setImmediate() is broken inside module

@drsm

Fixed modules reset in accumulative mode.

# HG changeset patch
# User hongzhidao <hongzhidao@gmail.com>
# Date 1553421943 -28800
# Node ID e1e6c0ca6b50b592a77880c3e1a7f004eb3d30a9
# Parent  81303df0fd67647cada20e23f60b9aaf9e7e5e92
Fixed modules reset in accumulative mode.

diff -r 81303df0fd67 -r e1e6c0ca6b50 njs/njs.c
--- a/njs/njs.c	Sat Mar 23 16:39:40 2019 +0300
+++ b/njs/njs.c	Sun Mar 24 18:05:43 2019 +0800
@@ -226,6 +226,10 @@ njs_vm_compile(njs_vm_t *vm, u_char **st
         return NJS_ERROR;
     }

+    if (vm->modules != NULL && vm->options.accumulative) {
+        njs_modules_reset(vm);
+    }
+
     parser = nxt_mp_zalloc(vm->mem_pool, sizeof(njs_parser_t));
     if (nxt_slow_path(parser == NULL)) {
         return NJS_ERROR;
diff -r 81303df0fd67 -r e1e6c0ca6b50 njs/njs_module.c
--- a/njs/njs_module.c	Sat Mar 23 16:39:40 2019 +0300
+++ b/njs/njs_module.c	Sun Mar 24 18:05:43 2019 +0800
@@ -37,6 +37,37 @@ static njs_module_t *njs_module_add(njs_
 static nxt_int_t njs_module_insert(njs_vm_t *vm, njs_module_t *module);


+void
+njs_modules_reset(njs_vm_t *vm)
+{
+    nxt_str_t           *name;
+    nxt_uint_t          i;
+    njs_module_t        **item, *module;
+    nxt_lvlhsh_query_t  lhq;
+
+    item = vm->modules->start;
+
+    for (i = 0; i < vm->modules->items; i++) {
+        module = *item;
+
+        if (!module->function.native) {
+            name = &module->name;
+
+            lhq.key = *name;
+            lhq.key_hash = nxt_djb_hash(name->start, name->length);
+            lhq.proto = &njs_modules_hash_proto;
+            lhq.pool = vm->mem_pool;
+
+            (void) nxt_lvlhsh_delete(&vm->modules_hash, &lhq);
+        }
+
+        item++;
+    }
+
+    nxt_array_reset(vm->modules);
+}
+
+
 nxt_int_t
 njs_module_load(njs_vm_t *vm)
 {
@@ -63,22 +94,14 @@ njs_module_load(njs_vm_t *vm)
         } else {
             ret = njs_vm_invoke(vm, &module->function, NULL, 0, module->index);
             if (ret == NXT_ERROR) {
-                goto done;
+                return ret;
             }
         }

         item++;
     }

-    ret = NXT_OK;
-
-done:
-
-    if (vm->options.accumulative) {
-        nxt_array_reset(vm->modules);
-    }
-
-    return ret;
+    return NXT_OK;
 }


diff -r 81303df0fd67 -r e1e6c0ca6b50 njs/njs_module.h
--- a/njs/njs_module.h	Sat Mar 23 16:39:40 2019 +0300
+++ b/njs/njs_module.h	Sun Mar 24 18:05:43 2019 +0800
@@ -16,6 +16,7 @@ typedef struct {
 } njs_module_t;


+void njs_modules_reset(njs_vm_t *vm);
 nxt_int_t njs_module_load(njs_vm_t *vm);
 nxt_int_t njs_parser_module(njs_vm_t *vm, njs_parser_t *parser);
 njs_ret_t njs_module_require(njs_vm_t *vm, njs_value_t *args,

Thanks again.

@drsm I need more work, the patch is still not ideal.

@hongzhidao with the patch above:

$ cat bug.js 
export default {
    f: function() {
        console.log('f: ', typeof gtest, gtest);
    }
};
$ build/njs
interactive njs 0.3.0

v.<Tab> -> the properties and prototype methods of v.
type console.help() for more information

>> import bug from 'bug.js';
ReferenceError: "gtest" is not defined in bug.js:3
>> bug
ReferenceError: "bug" is not defined in shell:1
>> import bug from 'bug.js';
undefined
>> bug
undefined
>>

deleted.

@xeioex @hongzhidao

$ cat bug.js 
export default {
    f: function() {
        console.log('f: ', typeof namespace_pollution);
    }
};

setImmediate(function() {
    console.log('immediate: ', typeof namespace_pollution);
});
$ build/njs
interactive njs 0.3.0

v.<Tab> -> the properties and prototype methods of v.
type console.help() for more information

>> var namespace_pollution = 123;
undefined
>> import bug from 'bug.js';
undefined
'immediate: ' 'number'
>> bug.f()
'f: ' 'number'
undefined
>>

and

$ cat bug.js 
export default {
    f: function() {
        console.log('f: ', typeof namespace_pollution, namespace_pollution);
    }
};
setImmediate(function() {
    console.log('immediate: ', typeof namespace_pollution, namespace_pollution);
});

$ build/njs
interactive njs 0.3.0

v.<Tab> -> the properties and prototype methods of v.
type console.help() for more information

>> import bug from 'bug.js';
ReferenceError: "namespace_pollution" is not defined in bug.js:8
>> var namespace_pollution = 123;
Segmentation fault

@xeioex

I think we can commit it first, then I’ll recommit the patch of arrow function. This ticket can be kept.

In next week.

1. Review the patch of arrow function.
2. Start the feature of template literal.
3. Improve modules feature.

@hongzhidao

It only says that the top level scope of variables in modules is a local scope.

Agree. So, let’s wait for @drsm response, otherwise I plan to commit https://gist.github.com/xeioex/83f8a91447f075e29d220e9bc54647b8

@xeioex

It seems only global this is unavailable, but everything else is available from global scope.

Yes. It does in chrome.

I think I misunderstand the usage of variable accessing in modules. See this. It seems global variables are allowed to access in modules. http://exploringjs.com/es6/ch_modules.html#_benefit-variable-checking

It only says that the top level scope of variables in modules is a local scope. If yes, your patch is right enough.

@drsm suggestions are welcome.

@xeioex

cat test.html (note that add type with ‘module’ value)

<script type="module" src="file2.js"></script>

cat file2.js

import './file1.js';

cat file1.js

console.log(isNaN());
console.log(Math);
console.log(this);

@xeioex

Tried in chrome.

// file2.js
import './file1.js';

// file1.js
console.log(isNaN());
console.log(Math);
console.log(this);

true
file1.js:3 Math {abs: ƒ, acos: ƒ, acosh: ƒ, asin: ƒ, asinh: ƒ, …}
file1.js:4 undefined

And normal global variables are not allowed to access in modules.

@hongzhidao, BTW it does not fail without the patch (will do), otherwise looks good.

+if (isNaN() === false) {
+    console.log("failed!");
+}
+
+if (isNaN(123) === true) {
+    console.log("failed!");
+}
+

@drsm thanks.

Try the patch please.

diff -r 53dc000e8c14 njs/njs_variable.c
--- a/njs/njs_variable.c	Sat Mar 02 20:31:10 2019 +0800
+++ b/njs/njs_variable.c	Sat Mar 23 15:16:55 2019 +0800
@@ -448,6 +448,7 @@ static njs_ret_t
 njs_variable_reference_resolve(njs_vm_t *vm, njs_variable_reference_t *vr,
     njs_parser_scope_t *node_scope)
 {
+    njs_variable_t      *var;
     nxt_lvlhsh_query_t  lhq;
     njs_parser_scope_t  *scope, *previous;

@@ -460,7 +461,15 @@ njs_variable_reference_resolve(njs_vm_t

     for ( ;; ) {
         if (nxt_lvlhsh_find(&scope->variables, &lhq) == NXT_OK) {
-            vr->variable = lhq.value;
+            var = lhq.value;
+
+            if (previous && previous->module
+                && var->value.type != NJS_FUNCTION)
+            {
+                goto not_found;
+            }
+
+            vr->variable = var;

             if (scope->type == NJS_SCOPE_SHIM) {
                 scope = previous;
@@ -488,16 +497,20 @@ njs_variable_reference_resolve(njs_vm_t
             return NXT_OK;
         }

-        if (scope->module || scope->parent == NULL) {
+        if (scope->parent == NULL) {
             /* A global scope. */
-            vr->scope = scope;
-
-            return NXT_DECLINED;
+            goto not_found;
         }

         previous = scope;
         scope = scope->parent;
     }
+
+not_found:
+
+    vr->scope = scope;
+
+    return NXT_DECLINED;
 }


diff -r 53dc000e8c14 njs/test/module/normal.js
--- a/njs/test/module/normal.js	Sat Mar 02 20:31:10 2019 +0800
+++ b/njs/test/module/normal.js	Sat Mar 23 15:16:55 2019 +0800
@@ -32,4 +32,12 @@ if (lib1_2.get() != 1) {
     console.log("failed!");
 }

+if (isNaN() === false) {
+    console.log("failed!");
+}
+
+if (isNaN(123) === true) {
+    console.log("failed!");
+}
+
 console.log("passed!");