Closed
Bug 1340304
Opened 8 years ago
Closed 7 years ago
Circular module exports throw InternalError instead of SyntaxError
Categories
(Core :: DOM: Core & HTML, defect, P2)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
DUPLICATE
of bug 1420420
| Tracking | Status | |
|---|---|---|
| firefox54 | --- | affected |
People
(Reporter: anba, Assigned: jdai)
References
Details
Attachments
(2 files)
|
(deleted),
patch
|
Details | Diff | Splinter Review | |
|
(deleted),
patch
|
Details | Diff | Splinter Review |
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•8 years 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•8 years 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•8 years ago
|
Priority: -- → P2
|
|
Assignee | |
Updated•8 years ago
|
|
|
Assignee | |
Comment 3•8 years ago
|
||
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
|
||
Comment 4•8 years 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•8 years ago
|
||
Here's a web platform test that reproduces the issue.
|
|
Assignee | |
Comment 6•8 years 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•7 years ago
|
||
I can no longer reproduce this issue. Jon, do you think it was fixed by your recent module updates?
Flags: needinfo?(jcoppeard)
|
|
||
Comment 8•7 years ago
|
||
Yes, I expect this was fixed by bug 1420420.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: needinfo?(jcoppeard)
Resolution: --- → DUPLICATE
You need to log in
before you can comment on or make changes to this bug.
Description
•