To implement dynamic module imports we'll need a way for the script loader to associate data with top-level (global) scripts as well as modules. (In the spec this is Script Record's [[HostDefined]] field.)

This removes ModuleObject's HostDefined slot and adds a void* pointer to top level scopes, adding a base class for global and module scopes. I did it like this to avoid adding an extra field to every JSScript since most of these are not top-level. After I did this I realised that it might make more sense to put this in the script's non-shared data. I had a go at doing this but it turned out to be a pain (alignment is important, you don't always know whether the script requires a private when this is set up, you have to calculate a pointer to it...). What do you think, is this OK or shall I have a another go?

Update the script loader to use the new API for storing a private pointer on a JSScript.

Comment on attachment 8998906 [details] [diff] [review] bug1482153-host-defined-field-1 Review of attachment 8998906 [details] [diff] [review]: ----------------------------------------------------------------- I think this is fine, it's just annoying that JSScript::isTopLevel() also returns true for eval scripts. However I think in that case we're guaranteed to MOZ_CRASH.

(In reply to Jan de Mooij [:jandem] from comment #3) > I think this is fine, it's just annoying that JSScript::isTopLevel() also > returns true for eval scripts. However I think in that case we're guaranteed > to MOZ_CRASH. Ok thanks, I can add some comments to make this clearer.

Pushed by jcoppeard@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4129380f8318 Replace ModuleObject's host defined field with one on top-level JSScripts r=jandem r=hsivonen

Backed out for assertion failures at ModuleScript.cpp:59. backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/09c51359bb20f1ebf9adfcc893f3199c7389d101 push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=8289b70dc0dbbe144f0081c7d99add92707efe79 failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=193674065&repo=mozilla-inbound&lineNumber=35756 [task 2018-08-13T16:36:20.056Z] 16:36:20 INFO - REFTEST TEST-START | file:///builds/worker/workspace/build/tests/jsreftest/tests/jsreftest.html?test=test262/language/module-code/namespace/internals/set-prototype-of.js;module [task 2018-08-13T16:36:20.057Z] 16:36:20 INFO - REFTEST TEST-LOAD | file:///builds/worker/workspace/build/tests/jsreftest/tests/jsreftest.html?test=test262/language/module-code/namespace/internals/set-prototype-of.js;module | 3870 / 10837 (35%) [task 2018-08-13T16:36:20.092Z] 16:36:20 INFO - Assertion failure: JS::GetTopLevelScriptPrivate(mScript) == this, at /builds/worker/workspace/build/src/dom/script/ModuleScript.cpp:59 [task 2018-08-13T16:36:42.772Z] 16:36:42 INFO - #01: mozilla::dom::ModuleScript::cycleCollection::Unlink(void*) [dom/script/ModuleScript.cpp:25] [task 2018-08-13T16:36:42.781Z] 16:36:42 INFO - [task 2018-08-13T16:36:42.782Z] 16:36:42 INFO - #02: nsCycleCollector::CollectWhite() [xpcom/base/nsCycleCollector.cpp:3467] [task 2018-08-13T16:36:42.784Z] 16:36:42 INFO - [task 2018-08-13T16:36:42.785Z] 16:36:42 INFO - #03: nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*, bool) [xpcom/base/nsCycleCollector.cpp:3836] [task 2018-08-13T16:36:42.786Z] 16:36:42 INFO - [task 2018-08-13T16:36:42.787Z] 16:36:42 INFO - #04: nsCycleCollector_collectSlice(js::SliceBudget&, bool) [xpcom/base/nsCycleCollector.cpp:4420] [task 2018-08-13T16:36:42.787Z] 16:36:42 INFO - [task 2018-08-13T16:36:42.787Z] 16:36:42 INFO - #05: nsJSContext::RunCycleCollectorSlice(mozilla::TimeStamp) [dom/base/nsJSEnvironment.cpp:1568] [task 2018-08-13T16:36:42.788Z] 16:36:42 INFO - [task 2018-08-13T16:36:42.789Z] 16:36:42 INFO - #06: ICCRunnerFired [dom/base/nsJSEnvironment.cpp:1623] [task 2018-08-13T16:36:42.789Z] 16:36:42 INFO - [task 2018-08-13T16:36:42.790Z] 16:36:42 INFO - #07: mozilla::IdleTaskRunner::Run() [gcc/include/c++/6.4.0/functional:2127] [task 2018-08-13T16:36:42.790Z] 16:36:42 INFO - [task 2018-08-13T16:36:42.790Z] 16:36:42 INFO - #08: nsJSContext::RunNextCollectorTimer(JS::gcreason::Reason, mozilla::TimeStamp) [dom/base/nsJSEnvironment.cpp:2042] [task 2018-08-13T16:36:42.790Z] 16:36:42 INFO - [task 2018-08-13T16:36:42.791Z] 16:36:42 INFO - #09: nsDOMWindowUtils::RunNextCollectorTimer() [dom/base/nsDOMWindowUtils.cpp:1264] [task 2018-08-13T16:36:42.792Z] 16:36:42 INFO - [task 2018-08-13T16:36:42.793Z] 16:36:42 INFO - #10: NS_InvokeByIndex [task 2018-08-13T16:36:42.793Z] 16:36:42 INFO - [task 2018-08-13T16:36:42.794Z] 16:36:42 INFO - #11: CallMethodHelper::Call() [js/xpconnect/src/xpcprivate.h:712] [task 2018-08-13T16:36:42.794Z] 16:36:42 INFO - [task 2018-08-13T16:36:42.795Z] 16:36:42 INFO - #12: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) [js/public/RootingAPI.h:1011] [task 2018-08-13T16:36:42.796Z] 16:36:42 INFO - [task 2018-08-13T16:36:42.797Z] 16:36:42 INFO - #13: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) [js/xpconnect/src/XPCWrappedNativeJSOps.cpp:893] [task 2018-08-13T16:36:42.797Z] 16:36:42 INFO - [task 2018-08-13T16:36:42.798Z] 16:36:42 INFO - #14: ??? (???:???) [task 2018-08-13T16:36:42.799Z] 16:36:42 INFO - [Parent 977, Gecko_IOThread] WARNING: pipe error (131): Connection reset by peer: file /builds/worker/workspace/build/src/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 356 [task 2018-08-13T16:36:42.800Z] 16:36:42 INFO - ###!!! [Parent][MessageChannel] Error: (msgtype=0x17007C,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv [task 2018-08-13T16:36:42.800Z] 16:36:42 INFO - A content process crashed and MOZ_CRASHREPORTER_SHUTDOWN is set, shutting down [task 2018-08-13T16:36:42.801Z] 16:36:42 INFO - JavaScript error: chrome://reftest/content/reftest.jsm, line 1558: NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIPropertyBag2.getPropertyAsAString]

Pushed by jcoppeard@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/da82ea6820ee Replace ModuleObject's host defined field with one on top-level JSScripts r=jandem r=hsivonen

As explained in bug 1481196 comment 6 I'm going to back this out. Just running this past you for cursory review as it's not a straight backout due to intervening changes.

Comment on attachment 9014065 [details] [diff] [review] backout-bug1482153-host-defined-field Review of attachment 9014065 [details] [diff] [review]: ----------------------------------------------------------------- Good find.

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/92181c289ffa370c103d333f6cc3b0c620a897b4

Here's a new approach that uses a new reserved slot to ScriptSourceObject and uses that to store the host-defined field. This uses a JS::Value so that we can use a JSObject for the shell and a pointer inside a PrivateValue for the browser. Using ScriptSourceObject is really helpful for dynamic import because it makes it easy to find this private given a call to import() that may be an a deeply nested scope. The patch also changes the module hooks to pass the private value rather than the module object. We don't have an equivalent object for scripts to pass and it makes more sense not to expose this. I changed the terminology to 'private' from 'host defined field' because the latter makes for unwieldy function names and we already use 'private' for the in our other APIs. This does push up the size of ScriptSource object but I think it might be possible to move some of the existing data on this (DOM element and attribute name) to the browser's private data in the future. I'll request separate review for the browser changes.

Comment on attachment 9017255 [details] [diff] [review] bug1482153-script-or-module-private Review of attachment 9017255 [details] [diff] [review]: ----------------------------------------------------------------- Nice.

Comment on attachment 9017255 [details] [diff] [review] bug1482153-script-or-module-private Requesting additional review for browser changes. This is changing the module hooks to pass the private JS::Value directly rather than passing a ModuleObject and renaming the APIs for getting/setting the private value.

Comment on attachment 9017255 [details] [diff] [review] bug1482153-script-or-module-private Review of attachment 9017255 [details] [diff] [review]: ----------------------------------------------------------------- Really an rs+ rather than an r+.

Pushed by jcoppeard@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/180eb0ea89bc Provide a way of associating a private value with a script or module r=jandem rs=hsivonen

This appears to have regressed JS memory usage in the base AWSY test by 12k. Jon, is that expected? Is there anything we can do to avoid that?

(In reply to Eric Rahm [:erahm] from comment #18) It was expected that it might increase memory usage a little. How significant is 12K? I'll file a bug for the possible mitigation mentioned in comment 12, but this is something that would have to be done after bug 1342012.

(In reply to Jon Coppeard (:jonco) from comment #19) > (In reply to Eric Rahm [:erahm] from comment #18) > It was expected that it might increase memory usage a little. How > significant is 12K? I'll file a bug for the possible mitigation mentioned > in comment 12, but this is something that would have to be done after bug > 1342012. We consider 12K somewhat significant for the JS base measurement (essentially JS memory usage for a content process with about:blank loaded), thanks for filing the follow up.