384168 - C.u.import doesn't prevent recursion in the presence of circular dependencies

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Mike Kaply [:mkaply]]

Attachment: Module that goes in the bin\modules directory

If I'm doing something horribly wrong, please let me know.

I've created a simple module that increments a counter every time it is loaded/inited and an extension that checks that counter.

What I'm finding is that the counter is always 1.

I thought that since my JS object was global, the counter should be incremented each time the module is loaded.

Comment 1

[Mike Kaply [:mkaply]]

Attachment: Extension

Comment 2

[Simon Bünzli]

AFAICT the module is initialized on first load and reused from that point (i.e. you always get the exact same objects to deal with) - so there's simply no way to tell how many time a module has been imported.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Correct, the module is only parsed/evaluated once and all exported objects are copied each time the module gets imported.

Comment 4

[Mike Kaply [:mkaply]]

OK, so I understand my confusion now. But there is still question.

If JS is in the global scope in a module, like:

dump("HELLO")

does that get executed everytime the module is imported?

I'm trying to figure out how to give a module the ability to init itself...

Comment 5

[Mike Kaply [:mkaply]]

OK, the problem is much more interesting. Here's the problem.

When microformats.js inits, it loads another module, adr.js. adr.js needs microformats it does a component import on microformats.js and so on and so on.

So even though when the Microformats global object inits and sets an internal variable to "true" to prevent reiniting itself, when I load the adr.js module which loads the Microformats module again, the internal variable is not set to true so we init again.

So I'm ending up with recursion loading my modules.

Comment 6

[Nickolay_Ponomarev]

mkaply, you wanted to resummarize / file a new bug for the issue in comment 5. This bug, as currently summarized, is invalid.

Comment 7

[Mike Kaply [:mkaply]]

OK, so basically what the real problem is here is that you can't prevent recursion in JS code modules. This might or might not be a bug.

Picture this scenario:

Microformats.js at init imports all the files that define microformats, like for instance adr.js. It does this by having a .init function at the end of the JS file.

But adr.js uses the Microformats API, so it needs to import Microformats.js 

So Microformats.js is imported again and init is invoked again.

There is no way to set a variable in Microformats.js to prevent init from being called again because the original import was never finalized.

Comment 8

[Robert Sayre]

(In reply to comment #7)
> OK, so basically what the real problem is here is that you can't prevent
> recursion in JS code modules. This might or might not be a bug.

A bug. I can see how this happens. With a fix, this should work fine as long EXPORTED_SYMBOLS appears before any calls to C.u.import.

Comment 9

[Nickolay_Ponomarev]

C.u.import() promises synchronous execution of the module (when necessary), afaics. So once it returns, the imported module is expected to be fully usable.

I don't think we should change that, but on the other hand, you can't do that when two modules import each other during the initial execution. I think we should warn (and maybe still do something semi-sensible) in that case - I don't see any use cases that might need the circular dependencies among the modules, so whenever we see that, it's probably a fault on the programmer's part.

Comment 10

[Robert Sayre]

Attachment: keep track of currently importing files

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 268692 [details] [diff] [review]
keep track of currently importing files


>+    *aGlobal = global;

Why move this up if it's not a GC root? It should not be necessary to worry about rooting given:

>     jsval retval;
>     if (!JS_ExecuteScript(cx, global, script, &retval)) {

JS_ExecuteScript protects global during its activation.

> #ifdef DEBUG_shaver_off
>         fprintf(stderr, "mJCL: failed to execute %s\n", nativePath.get());
> #endif
>+        *aGlobal = nsnull;
>         return NS_ERROR_FAILURE;
>     }
> 
>     /* Freed when we remove from the table. */
>     nsCAutoString path;
>     aComponent->GetNativePath(path);
>     *aLocation = ToNewCString(path);
>-    if (!*aLocation)
>+    if (!*aLocation) {
>+        *aGlobal = nsnull;
>         return NS_ERROR_OUT_OF_MEMORY;
>+    }
> 
>-    *aGlobal = global;

Nothing else to this point could cause a GC, and anyway holder protects global AFAICS.

>     JS_AddNamedRoot(cx, aGlobal, *aLocation);

And now global (or whatever the value of *aGlobal is) is protected.

>     ModuleEntry* mod;
>     nsAutoPtr<ModuleEntry> newEntry;
>-    if (!mImports.Get(lfhash, &mod)) {
>+    if (!mImports.Get(lfhash, &mod) && !mInProgressImports.Get(lfhash, &mod)) {
>         newEntry = new ModuleEntry;
>-        if (!newEntry)
>+        mod = newEntry;
>+        if (!newEntry || !mInProgressImports.Put(lfhash, mod))
>             return NS_ERROR_OUT_OF_MEMORY;
>-        
>+
>         rv = GlobalForLocation(componentFile, &newEntry->global,
>                                &newEntry->location);
>+
>+        mInProgressImports.Remove(lfhash);
>+
>         if (NS_FAILED(rv)) {
>+            mod = nsnull;
>             *_retval = nsnull;
>             return NS_ERROR_FILE_NOT_FOUND;
>         }
>-        
>-        mod = newEntry;

Just curious why mod's setting was moved up here.

/be

Comment 12

[Robert Sayre]

(In reply to comment #11)
> (From update of attachment 268692 [details] [diff] [review])
> 
> >+    *aGlobal = global;
> 
> Why move this up if it's not a GC root? It should not be necessary to worry
> about rooting given:

It's not rooting that's the problem. When A.js imports B.js, and B.js imports A.js, mod->global must be set when A.js comes around a second time. To ensure this state, aGlobal is assigned before JS_ExecuteScript is called.

> 
> >     ModuleEntry* mod;
> >     nsAutoPtr<ModuleEntry> newEntry;
> >-    if (!mImports.Get(lfhash, &mod)) {
> >+    if (!mImports.Get(lfhash, &mod) && !mInProgressImports.Get(lfhash, &mod)) {

This is where mod->global needs to be set if we're going to fail the conditional.

> >         newEntry = new ModuleEntry;
> >-        if (!newEntry)
> >+        mod = newEntry;
> >+        if (!newEntry || !mInProgressImports.Put(lfhash, mod))
> >             return NS_ERROR_OUT_OF_MEMORY;
> >-        
> >+
> >         rv = GlobalForLocation(componentFile, &newEntry->global,
> >                                &newEntry->location);
> >+
> >+        mInProgressImports.Remove(lfhash);
> >+
> >         if (NS_FAILED(rv)) {
> >+            mod = nsnull;
> >             *_retval = nsnull;
> >             return NS_ERROR_FILE_NOT_FOUND;
> >         }
> >-        
> >-        mod = newEntry;
> 
> Just curious why mod's setting was moved up here.

Because I stuck mod into mInProgressImports for some reason. I can move that back.

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 268692 [details] [diff] [review]
keep track of currently importing files

(In reply to comment #12)
> It's not rooting that's the problem. When A.js imports B.js, and B.js imports
> A.js, mod->global must be set when A.js comes around a second time.

Oh of course -- worth a comment before setting *aGlobal early, I say.

r=me with that and the reversion of that unneeded mod-moving hunk.

/be

Comment 14

[Robert Sayre]

Attachment: address brendan's comments

Comment 15

[Benjamin Smedberg]

Comment on attachment 268717 [details] [diff] [review]
address brendan's comments

>Index: js/src/xpconnect/tests/unit/recursive_importA.jsm

>\ No newline at end of file

please fix before checkin

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Might this have caused the orange that started 1 cycle after it landed?  Mano says the stack seems more related to this than brendan's checkin...

Comment 17

[Robert Sayre]

(In reply to comment #16)
> Might this have caused the orange that started 1 cycle after it landed?  Mano
> says the stack seems more related to this than brendan's checkin...
> 

Turned out to be a cycle collector bug triggered by brendan's checkin