The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

()

Core
XPConnect
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: mkaply, Assigned: Robert Sayre)

Tracking

unspecified
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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.
(Reporter)

Comment 1

10 years ago
Created attachment 268101 [details]
Extension

Comment 2

10 years ago
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.
Correct, the module is only parsed/evaluated once and all exported objects are copied each time the module gets imported.
(Reporter)

Comment 4

10 years ago
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...
(Reporter)

Comment 5

10 years ago
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

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

Comment 7

10 years ago
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.
Summary: JavaScript code sharing not reusing global objects → Cannot prevent recursion when sharing javascript code
(Assignee)

Comment 8

10 years ago
(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.
Assignee: nobody → sayrer

Comment 9

10 years ago
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.
(Assignee)

Comment 10

10 years ago
Created attachment 268692 [details] [diff] [review]
keep track of currently importing files
Attachment #268692 - Flags: review?(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
(Assignee)

Comment 12

10 years ago
(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 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
Attachment #268692 - Flags: review?(brendan) → review+
(Assignee)

Comment 14

10 years ago
Created attachment 268717 [details] [diff] [review]
address brendan's comments
Attachment #268692 - Attachment is obsolete: true
Attachment #268717 - Flags: superreview?
(Assignee)

Updated

10 years ago
Attachment #268717 - Flags: superreview? → superreview?(benjamin)
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
Attachment #268717 - Flags: superreview?(benjamin) → superreview+
(Assignee)

Updated

10 years ago
Summary: Cannot prevent recursion when sharing javascript code → Cannot prevent recursion with recursive calls to C.u.import
(Assignee)

Updated

10 years ago
Summary: Cannot prevent recursion with recursive calls to C.u.import → C.u.import doesn't prevent recursion with circular dependencies
(Assignee)

Updated

10 years ago
Summary: C.u.import doesn't prevent recursion with circular dependencies → C.u.import doesn't prevent recursion in the presence of circular dependencies
(Assignee)

Updated

10 years ago
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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...
(Assignee)

Comment 17

10 years ago
(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
You need to log in before you can comment on or make changes to this bug.