Closed Bug 898034 Opened 6 years ago Closed 6 years ago

[Chrome worker] module.exports only works once

Categories

(Toolkit :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: Yoric, Assigned: Yoric)

Details

Attachments

(1 file, 1 obsolete file)

i.e.
// module a.js
module.exports = { foo: 1 };

// module b.js
let A1 = require("a");
let A2 = require("a");
dump(A1.foo); // 1
dump(A1.foo); // undefined
Attached patch module.exports (obsolete) — Splinter Review
Attachment #781077 - Flags: review?(rFobic)
Comment on attachment 781077 [details] [diff] [review]
module.exports

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

r+ with my comments addressed, since they're very small changes I don't think we need another round.

Thanks!

::: toolkit/components/workerloader/require.js
@@ +167,5 @@
>  
>        // Make module available immediately
>        // (necessary in case of circular dependencies)
>        if (modules.has(path)) {
> +        return modules.get(path).exports;

This means that module.exports can be changed at later ticks which I'd rather forbid. For example

module.exports = { a: 1 }
setTimeout(function() { module.exports = { a: 2 })


This will cause two different requires to get a different exports if they don't happen on the same tick.

@@ +172,1 @@
>        }

One the second thought I think this makes sense, I would just make sure to make `module.export`  non configurable & non writable after you freeze exports:

https://github.com/mozilla/mozilla-central/blob/master/toolkit/components/workerloader/require.js#L218

::: toolkit/components/workerloader/tests/worker_test_loading.js
@@ +84,5 @@
> +add_test(function test_module_dot_exports() {
> +  let H = require(PATH + "moduleH-module-dot-exports.js");
> +  is(H.key, "value", "module.exports worked");
> +  let H2 = require(PATH + "moduleH-module-dot-exports.js");
> +  is(H2.key, H.key, "module.exports worked twice");

Can you please compare `H2.key` to "value" instead of H.key to make sure `H.key` did not change. I would also add another assertion to make sure `H === H2`.
Attachment #781077 - Flags: review?(rFobic) → review+
In the end, I decided to freeze everything (both module and module.exports).
If we realize at a later stage that we should have left them unfrozen, there will be time for that.
Attachment #781077 - Attachment is obsolete: true
Attachment #781704 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/7cdac016f94f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.