1628279 - JS shell module loader is not initialized for additional globals

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

The JS shell module loader is only initialized once for the main global by an InitModuleLoader call. Any additional globals created e.g. with newGlobal() lack the module loader and this seems to cause leaks/lifetime issues when running code in another global that would require the module loader.

This can be fixed easily by adding a global to the InitModuleLoader parameters and calling it again in NewGlobal. I have a patch for this coming up later.

Comment 1

[Christian Holler (:decoder)]

Attachment: Bug 1628279 - Initialize the JS shell module loader for each new global. r?jonco

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Christian Holler (:decoder) from comment #0)

Any additional globals created e.g. with newGlobal() lack the module loader and this seems to cause leaks/lifetime issues when running code in another global that would require the module loader.

We can add the module loader for new globals but I don't think this will address the root case of the leak.

Comment 3

[Christian Holler (:decoder)]

(In reply to Jon Coppeard (:jonco) from comment #2)

(In reply to Christian Holler (:decoder) from comment #0)

Any additional globals created e.g. with newGlobal() lack the module loader and this seems to cause leaks/lifetime issues when running code in another global that would require the module loader.

We can add the module loader for new globals but I don't think this will address the root case of the leak.

I totally agree, but so far, the module loader seems to be the only source causing this to happen at all. This also means that the root cause might be shell-only, making it even less likely to be tracked down in a timely manner. With the attached patch, testing is unblocked.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/928ee6869299
Initialize the JS shell module loader for each new global. r=jonco

Comment 5

[Cristian Brindusan [:cbrindusan]]

Backed out changeset 928ee6869299 (Bug 1628279) for sm bustages at script-filename-validation-1.js.
https://hg.mozilla.org/integration/autoland/rev/b824a66c9fe6ee51eda56e98b3729f3fdbc5f961

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=297888160&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=928ee6869299a81bc293056d566ce5127f4bd8f7

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=297888160&repo=autoland&lineNumber=15598

Comment 6

[Christian Holler (:decoder)]

Jon, I don't understand exactly what these failures mean or how these tests interact with my newGlobal change. Could you take a look and let me know how I should fix these? Thanks!

Comment 7

[Jon Coppeard (:jonco)]

Looks like this is falling foul of a script filename validation check that Jan added. I think you can add options.setSkipFilenameValidation(true) when setting the compile options in InitModuleLoader to fix this.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:decoder, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 9

[Christian Holler (:decoder)]

Jon, I saw that you made significant changes to the module loader a while ago already in bug 1637529. With these new changes, do we still need this bug or is the module loader now working in all globals in the JS shell? If not, is it easy for me to make the change using the new C++ module loader?

Comment 10

[Jon Coppeard (:jonco)]

Oh, I forgot to comment here. Yes, with the changes the module loader should now work in all globals. Can you check it's working as you expect?

Comment 11

[Caroline Cullen [:caroline]]

Wanted to see if we could close this bug out, have we confirmed that with Jon's rewrite fixed the globals issue?

Comment 12

[Christian Holler (:decoder)]

Yes, so far I think everything is working as expected, I'll open a new bug if anything related becomes visible again in parser fuzzing. Thanks!