Closed Bug 1340304 Opened 3 years ago Closed 2 years ago

Circular module exports throw InternalError instead of SyntaxError

Categories

(Core :: DOM: Core & HTML, defect, P2)

defect

Tracking

()

RESOLVED DUPLICATE of bug 1420420
Tracking Status
firefox54 --- affected

People

(Reporter: anba, Assigned: jdai)

References

Details

Attachments

(2 files)

Test case:
---
<!DOCTYPE html>
<html>
  <head>
   <meta charset="utf-8">
  </head>
  <body>
    <script>
    (function() {
      window.onerror = function() {
        console.warn("onerror with: ", ...arguments);
      };

      var script = document.createElement("script");
      script.charset = "utf-8";
      script.type = "module";
      script.src = "./m.js";

      document.documentElement.appendChild(script);
    })();
    </script>
  </body>
</html>
---

m.js:
---
export { x } from "./q.js";
---

q.js:
---
export { x } from "./m.js";
---

Steps to reproduce:
- Enable dom.moduleScripts.enabled pref in about:config
- Load the html test case

Expected:
- window.onerror is called with SyntaxError object

Actual:
- window.onerror is called with `InternalError: attempt to re-instantiate module after failure`
InternalError shouldn't be exposed. John, have time to take a quick look at of what goes wrong here?
Flags: needinfo?(jdai)
The problem is we reset any exception to MODULE_STATE_FAILED [1][2].
When we load another module, we check GetModuleEnvironment()[3], and it throws InternalError. 

[1] https://searchfox.org/mozilla-central/source/js/src/builtin/Module.js#279
[2] https://searchfox.org/mozilla-central/source/js/src/builtin/Module.js#211
[3] https://searchfox.org/mozilla-central/source/js/src/builtin/Module.js#196-197
Flags: needinfo?(jdai)
Priority: -- → P2
See Also: → 1341298
Attached patch patch, v1Splinter Review
I removed RecordInstantationFailure() in module.js. It's because every error throws in ModuleDeclarationInstantiation which will reset to MODULE_STATE_FAILED, then it'll be reported by AutoEntryScript[1] when it finished nsScriptLoader::EvaluateScript(). After I ran try, it didn't bring any side-effect. Moreover, the previous patch was landed by you.[3] I would like you to give me some feedback. Thank you.


[1] https://searchfox.org/mozilla-central/source/dom/base/nsScriptLoader.cpp#2217
[2] Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d19ed0c0ac1c97d6b3616716017cdff13a0afe39&filter-tier=1&group_state=expanded
[3] https://bugzilla.mozilla.org/show_bug.cgi?id=1284486
Assignee: nobody → jdai
Status: NEW → ASSIGNED
Attachment #8851466 - Flags: feedback?(jcoppeard)
Comment on attachment 8851466 [details] [diff] [review]
patch, v1

Review of attachment 8851466 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch, but I don't think this is the correct solution.

The code removed by the patch is designed to make sure that we don't end up attempting to instantiate a module again after a previous instantiation has failed.  This is still an important thing to do.  The test added in the original bug doesn't quite cover this case (although it covers a related case), and that should probably be fixed.

I haven't worked out what's wrong but it's probably something to do with the script loader not handling cleanup properly for circularly-dependent modules.
Attachment #8851466 - Flags: feedback?(jcoppeard)
Here's a web platform test that reproduces the issue.
(In reply to Jon Coppeard (:jonco) from comment #4)
> Comment on attachment 8851466 [details] [diff] [review]
> patch, v1
> 
> Review of attachment 8851466 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch, but I don't think this is the correct solution.
> 
> The code removed by the patch is designed to make sure that we don't end up
> attempting to instantiate a module again after a previous instantiation has
> failed.  This is still an important thing to do.  The test added in the
> original bug doesn't quite cover this case (although it covers a related
> case), and that should probably be fixed.
> 
> I haven't worked out what's wrong but it's probably something to do with the
> script loader not handling cleanup properly for circularly-dependent modules.

Thank you for your feedback and give me valuable direction. I'll provide a patch in next version.
I have to said sorry that this is my first bug jump into this area.
I can no longer reproduce this issue. Jon, do you think it was fixed by your recent module updates?
Flags: needinfo?(jcoppeard)
Yes, I expect this was fixed by bug 1420420.
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Flags: needinfo?(jcoppeard)
Resolution: --- → DUPLICATE
Duplicate of bug: 1420420
You need to log in before you can comment on or make changes to this bug.