module: fix re-entrant module importing #4574
Conversation
|
Fixes #4570 . Test cases will follow. /cc @rhuanjl @fatcerberus |
|
@rhuanjl would you like to contribute your test cases? Please let me know the commit hash |
| bool contains = this->parentModuleList->Contains(parentRecord); | ||
| if (!contains) | ||
|
|
||
| if (!this->parentModuleList->Contains(parentRecord)) |
There was a problem hiding this comment.
My branch wasn't updated hence there was an assert(!contains) here. That's why the change
There was a problem hiding this comment.
|
Prior to adding more test cases, I will introduce |
|
that would be great! In reply to: 358745237 [](ancestors = 358745237) |
|
The test case I had is here: Though I note that this issue would be covered by currently disabled test bug_issue_3257.js |
It would do that to some extend. I saw there are other issues need attention prior to enable it. See #4575 |
…names. Note - test coverage is actually already provided by more bug issue 3257 But that test is currently disabled
|
Also opened #4577 to track |
|
@akroshg @boingoing please review |
|
This seems to work for most cases now. The following seemingly related example (i.e. a circular re-entrant import) still crashes but I think it crashes with or without this fix - taking module0 out of the folder removes the crash so this is probably an error in ch's specifier normalising or something and not a CC error (a few printfs show that this case is somehow generating 5 module records): //test.js
WScript.LoadScriptFile("module0/module0.js","module");//module0/module0.js
import "module1.js";
import "module2.js";//module1.js
import "module2.js";//module2.js
import "module0/module0.js"; |
|
Yep there are more to come. Things to do on both ch and CC side. Thanks for the test cases and all the effort @rhuanjl ! |
boingoing
left a comment
boingoing
left a comment
There was a problem hiding this comment.
Looks good to me, thanks for fixing these! 👍
|
@akroshg @rhuanjl @boingoing Thanks for the reviews! |
Merge pull request #4574 from obastemur:fix_reent
…rting Merge pull request #4574 from obastemur:fix_reent
…t module importing Merge pull request #4574 from obastemur:fix_reent
|
@obastemur This fix has resurfaced #4482 in miniSphere. :( |
6 participants
No description provided.