Circular module exports throw InternalError instead of SyntaxError

RESOLVED DUPLICATE of bug 1420420

Status

()

Core
DOM: Core & HTML
P2
normal
RESOLVED DUPLICATE of bug 1420420
11 months ago
10 days ago

People

(Reporter: anba, Assigned: jdai)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---

Firefox Tracking Flags

(firefox54 affected)

Details

Attachments

(2 attachments)

(Reporter)

Description

11 months ago
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`

Comment 1

11 months ago
InternalError shouldn't be exposed. John, have time to take a quick look at of what goes wrong here?
Flags: needinfo?(jdai)
(Assignee)

Comment 2

11 months ago
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)

Updated

11 months ago
Priority: -- → P2
(Assignee)

Updated

10 months ago
See Also: → bug 1341298
(Assignee)

Comment 3

10 months ago
Created attachment 8851466 [details] [diff] [review]
patch, v1

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 4

10 months ago
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)

Comment 5

10 months ago
Created attachment 8852045 [details] [diff] [review]
bug1340304-circular-dependency-test

Here's a web platform test that reproduces the issue.
(Assignee)

Comment 6

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

Comment 7

10 days ago
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
Last Resolved: 10 days 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.