Last Comment Bug 384168 - C.u.import doesn't prevent recursion in the presence of circular dependencies
: C.u.import doesn't prevent recursion in the presence of circular dependencies
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
-- normal (vote)
: ---
Assigned To: Robert Sayre
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-06-12 10:39 PDT by Mike Kaply [:mkaply]
Modified: 2007-06-20 11:31 PDT (History)
7 users (show)
sayrer: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Module that goes in the bin\modules directory (137 bytes, text/plain)
2007-06-12 10:39 PDT, Mike Kaply [:mkaply]
no flags Details
Extension (3.17 KB, application/octet-stream)
2007-06-12 10:40 PDT, Mike Kaply [:mkaply]
no flags Details
keep track of currently importing files (13.24 KB, patch)
2007-06-17 11:51 PDT, Robert Sayre
brendan: review+
Details | Diff | Splinter Review
address brendan's comments (13.24 KB, patch)
2007-06-17 15:33 PDT, Robert Sayre
benjamin: superreview+
Details | Diff | Splinter Review

Description User image Mike Kaply [:mkaply] 2007-06-12 10:39:48 PDT
Created attachment 268100 [details]
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 User image Mike Kaply [:mkaply] 2007-06-12 10:40:48 PDT
Created attachment 268101 [details]
Extension
Comment 2 User image Simon Bünzli 2007-06-12 10:45:47 PDT
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 User image Ben Turner (not reading bugmail, use the needinfo flag!) 2007-06-12 11:16:01 PDT
Correct, the module is only parsed/evaluated once and all exported objects are copied each time the module gets imported.
Comment 4 User image Mike Kaply [:mkaply] 2007-06-12 11:20:55 PDT
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 User image Mike Kaply [:mkaply] 2007-06-12 11:31:52 PDT
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 User image Nickolay_Ponomarev 2007-06-13 12:51:57 PDT
mkaply, you wanted to resummarize / file a new bug for the issue in comment 5. This bug, as currently summarized, is invalid.
Comment 7 User image Mike Kaply [:mkaply] 2007-06-13 13:29:57 PDT
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 User image Robert Sayre 2007-06-13 16:39:32 PDT
(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 User image Nickolay_Ponomarev 2007-06-13 18:41:03 PDT
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 User image Robert Sayre 2007-06-17 11:51:01 PDT
Created attachment 268692 [details] [diff] [review]
keep track of currently importing files
Comment 11 User image Brendan Eich [:brendan] 2007-06-17 13:40:49 PDT
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 User image Robert Sayre 2007-06-17 14:49:22 PDT
(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 User image Brendan Eich [:brendan] 2007-06-17 14:58:57 PDT
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 User image Robert Sayre 2007-06-17 15:33:58 PDT
Created attachment 268717 [details] [diff] [review]
address brendan's comments
Comment 15 User image Benjamin Smedberg [:bsmedberg] 2007-06-19 12:35:27 PDT
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 User image David Baron :dbaron: ⌚️UTC-8 2007-06-20 09:19:26 PDT
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 User image Robert Sayre 2007-06-20 10:44:43 PDT
(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

Note You need to log in before you can comment on or make changes to this bug.