A proposal for adding a "function-like" import() module loading syntactic form to JavaScript. It is currently in stage 3 of the TC39 process.

Chromium folks try to ship this feature: https://groups.google.com/a/chromium.org/d/msg/blink-dev/ut-Mr0jt5X8/Q8B4F3wxBQAJ

Chrome 63.0.3228.0 now has the enable-module-scripts-dynamic-import enabled by default.

Jon, can you comment on how much effort it'd be to implement this? In bug 1427610 comment 4 Justin Fagnani from the Polymer project is asking about when we're planning to ship this.

(In reply to Till Schneidereit [:till] from comment #3) Sorry for the slow reply. I'm planning on working on this after import.meta, but I'm pretty busy right now.

I am very excited to see spurs of progress on this, but honestly, it is very confusing to see how JavaScript Modules shows up as "Complete" in Platform Status and JavaScript Dynamic Imports not even listed and not draw the conclusion that there is little follow up to this enormous barrier that has left people discouraged from using the now 4 years old ES modules. Please give this some urgency, the momentum that helped drive people to join the JavaScript community is now over because of such oversights (IMHO).

I also think it's important to move forward with this issue. Also because a dynamic import breaks the whole code of your app with "SyntaxError: import declarations may only appear at top level of a module"

Pushed by jcoppeard@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/41ded240a012 Add OOM tests for shell dynamic module import and fix bugs r=jandem

Backed out for wrong bug number in commit message: https://hg.mozilla.org/integration/mozilla-inbound/rev/84f29739c1da074c184d5e21e73b59d083a0e29d

This patch adds support for script and module private values containing a private pointer to a cycle collected C++ object. It adds a script finalize hook so the embedding can register code to run when a script or module with a private value is finalized (in reality its ScriptSourceObject). These are finalized on the main thread. The cycle collector is updated to interpret these private values as nsISupports pointers when tracing through the JS heap, by means of js::MaybeGetScriptPrivate() which it calls on JSObjects it encounters. I'll request additional review for this.

Patch to add support for dynamic import from modules only. This adds the following to ModuleLoadRequest: - A flag to indicate whether it's a dynamic import. - New fields that are used for dynamic imports. These are passed in from the JS engine passed and back when a dynamic import is complete. - A new constructor for use during dynamic import that sets up the above. ModuleScript (which represents a compiled module) gets a ScriptFetchOptions pointer. This is shared between all module scripts in a single module graph and is used to remember fetch options that are necessary for later dynamic import. When a ModuleScript is associated with a JS module object, the latter's private value is set to point to the ModuleScript and the reference count incremented. The script private finalize hook is used to decrement this when the module object dies. A pref is added for dynamic import which installs/clears the JS engine dynamic import hook, enabling/disabling this feature in the JS engine. The dynamic import hook StartDynamicImport attempts to resolve the module specifier, create new module load request and start loading it. The ScriptLoader gets a new list of dynamic import request, mDynamicImportRequests, to track these. Eventually FinishDynaimcImport is called which calls back into the JS engine to complete the import.

We will need to preserve information about classic scripts to allow future dynamic import from them. This patch refactors ModuleScript into LoadedScript (a new base class), ClassicScript and ModuleScript classes. The mLoader, mFetchOptions and mBaseURL fields are moved to the base class. The file is renamed to LoadedScript.h in the next patch.

Rename ModuleScript source files to LoadedScript.

Refactor nsJSUtils::ExecutionContext to separate out compilation and executution into different methods and add a way to extract the compiled JSScript in between. We need to extract the JSScript in some cases so we can associate a LoadedScript object with it. r?ing you since you wrote this. I'll request additional review from a DOM peer afterwards.

This allows dynamic import from classic scripts as well as module scripts. We provide a way of associating classic scripts with with loader metadata so we can support dynamic import from them. We do this by setting a private value for a JSScript and we add a reference to the LoadedScript every time we do this. These are not circular references in the case of classic scripts, unlike the module script case where the ModuleScript object contains a pointer to the JS module object which has a pointer back to th e ModuleScript (via the internal ScriptSourceObject). mBaseURL moves to ScriptLoadRequest (the base class) from ModuleLoadRequest as it's needed for import. ResolveModulesSpecifier is updated to work if the referencing thing is a classic script. Finally we have to create ClassicScript objects for classic scripts we load in case they do dynamic import later. These are associated with the compiled JSScript as described above.

Provide a JS API to get the script or module private value associated with the currently executing script. This is needed to get the correct import context for timeout handlers, which can compile code passed as a string.

Comment on attachment 9019704 [details] [diff] [review] bug1342012-4-refactor-exec-context Review of attachment 9019704 [details] [diff] [review]: ----------------------------------------------------------------- Nice! ::: dom/base/nsJSUtils.cpp @@ +138,5 @@ > , mEncodeBytecode(false) > #ifdef DEBUG > , mWantsReturnValue(false) > , mExpectScopeChain(false) > + , mScriptUsed(false) nit: fix indentation. ::: dom/base/nsJSUtils.h @@ +121,5 @@ > ~ExecutionContext() { > + // This flag is reset when the returned value is extracted. > + MOZ_ASSERT_IF(!mSkip, !mWantsReturnValue); > + > + MOZ_ASSERT_IF(mEncodeBytecode && mScript, mScriptUsed); nit: … && mRv == NS_OK, mScriptUsed); nit: Add the similar comment above: // If the encoding got started, we expect the script to be used later on for ending the encoding. ::: dom/script/ScriptLoader.cpp @@ +2578,5 @@ > } else { > // Main thread parsing (inline and small scripts) > LOG(("ScriptLoadRequest (%p): Compile And Exec", aRequest)); > if (aRequest->IsBinASTSource()) { > + exec.DecodeBinAST(options, nit: rv = exec.DecodeBinAST(…)

Comment on attachment 9019713 [details] [diff] [review] bug1342012-6-get-caller-private Review of attachment 9019713 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nit addressed. ::: js/src/jsapi.cpp @@ +4212,5 @@ > + if (iter.done()) { > + return UndefinedValue(); > + } > + > + return FindScriptOrModulePrivateForScript(iter.script()); This will assert if the caller is a Wasm frame. Maybe release-assert hasScript() or add `|| !iter.hasScript()` to the previous if-statement?

Comment on attachment 9019683 [details] [diff] [review] bug1342012-0-script-private-support Review of attachment 9019683 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the js/src changes ::: js/src/vm/JSScript.cpp @@ +1450,5 @@ > + Value value = sso->getPrivate(); > + if (!value.isUndefined()) { > + // The embedding may need to dispose of its private data. > + JS::AutoSuppressGCAnalysis suppressGC; > + if (JS::ScriptPrivateFinalizeHook hook = fop->runtime()->scriptPrivateFinalizeHook) Nit: {}

Comment on attachment 9019701 [details] [diff] [review] bug1342012-2-refactor-module-script >+////////////////////////////////////////////////////////////// >+// ClassicScript >+////////////////////////////////////////////////////////////// >+ >+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(ClassicScript) >+NS_INTERFACE_MAP_END_INHERITING(LoadedScript) >+ >+NS_IMPL_CYCLE_COLLECTION_CLASS(ClassicScript) >+ >+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(ClassicScript, >+ LoadedScript) >+NS_IMPL_CYCLE_COLLECTION_UNLINK_END >+ >+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(ClassicScript, >+ LoadedScript) >+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END >+ >+NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN_INHERITED(ClassicScript, >+ LoadedScript) >+NS_IMPL_CYCLE_COLLECTION_TRACE_END >+ >+NS_IMPL_ADDREF_INHERITED(ClassicScript, LoadedScript) >+NS_IMPL_RELEASE_INHERITED(ClassicScript, LoadedScript) Since ClassicScript doesn't add anything to the cycle collection, no need for all this stuff. > NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(ModuleScript) >-NS_INTERFACE_MAP_END >+NS_INTERFACE_MAP_END_INHERITING(LoadedScript) > > NS_IMPL_CYCLE_COLLECTION_CLASS(ModuleScript) > >-NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(ModuleScript) >- NS_IMPL_CYCLE_COLLECTION_UNLINK(mLoader) >- NS_IMPL_CYCLE_COLLECTION_UNLINK(mFetchOptions) >- NS_IMPL_CYCLE_COLLECTION_UNLINK(mBaseURL) >+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(ModuleScript, >+ LoadedScript) > tmp->UnlinkModuleRecord(); > tmp->mParseError.setUndefined(); > tmp->mErrorToRethrow.setUndefined(); > NS_IMPL_CYCLE_COLLECTION_UNLINK_END > >-NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(ModuleScript) >- NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mLoader) >- NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mFetchOptions) >+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(ModuleScript, >+ LoadedScript) > NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END > >-NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(ModuleScript) >+NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN_INHERITED(ModuleScript, >+ LoadedScript) > NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMBER_CALLBACK(mModuleRecord) > NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMBER_CALLBACK(mParseError) > NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMBER_CALLBACK(mErrorToRethrow) > NS_IMPL_CYCLE_COLLECTION_TRACE_END > >-NS_IMPL_CYCLE_COLLECTING_ADDREF(ModuleScript) >-NS_IMPL_CYCLE_COLLECTING_RELEASE(ModuleScript) >+NS_IMPL_ADDREF_INHERITED(ModuleScript, LoadedScript) >+NS_IMPL_RELEASE_INHERITED(ModuleScript, LoadedScript) Same here. If nothing is added to cycle collection, no need to add all the macros. >+class ClassicScript final: public LoadedScript >+{ >+ ~ClassicScript() = default; >+ >+ public: >+ NS_DECL_ISUPPORTS_INHERITED >+ NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(ClassicScript, >+ LoadedScript) I don't see why these are needed. >+class ModuleScript final : public LoadedScript >+{ > JS::Heap<JSObject*> mModuleRecord; > JS::Heap<JS::Value> mParseError; > JS::Heap<JS::Value> mErrorToRethrow; > bool mSourceElementAssociated; > > ~ModuleScript(); > > public: >- NS_DECL_CYCLE_COLLECTING_ISUPPORTS >- NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(ModuleScript) >+ NS_DECL_ISUPPORTS_INHERITED >+ NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(ModuleScript, >+ LoadedScript) same here. LoadedScript implements refcounting and traverses/unlinks the needed stuff. r- because I think the patch could be quite a bit simpler :)

Comment on attachment 9019712 [details] [diff] [review] bug1342012-5-import-from-classic-scripts >+LoadedScript::AssociateWithScript(JSScript* aScript) >+{ >+ MOZ_ASSERT(JS::GetScriptPrivate(aScript).isUndefined()); >+ JS::SetScriptPrivate(aScript, JS::PrivateValue(this)); >+ AddRef(); >+} >+ >+void >+HostFinalizeTopLevelScript(JSFreeOp* aFop, const JS::Value& aPrivate) >+{ >+ auto script = static_cast<LoadedScript*>(aPrivate.toPrivate()); >+ >+#ifdef DEBUG >+ if (script->IsModuleScript()) { >+ JSObject* module = script->AsModuleScript()->mModuleRecord.unbarrieredGet(); >+ MOZ_ASSERT(JS::GetModulePrivate(module) == aPrivate); >+ } >+#endif >+ >+ script->Release(); >+} >+ I don't understand this lifetime management. JSScript has strong reference to LoadedScript. What breaks the cycle if C++ keeps JSScript alive? > NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(ScriptLoadRequest) >- NS_IMPL_CYCLE_COLLECTION_UNLINK(mFetchOptions) >- NS_IMPL_CYCLE_COLLECTION_UNLINK(mCacheInfo) >+ NS_IMPL_CYCLE_COLLECTION_UNLINK(mFetchOptions, >+ mCacheInfo, >+ mBaseURL) > tmp->mScript = nullptr; > tmp->DropBytecodeCacheReferences(); > NS_IMPL_CYCLE_COLLECTION_UNLINK_END > > NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(ScriptLoadRequest) >- NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mFetchOptions) >- NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mCacheInfo) >+ NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mFetchOptions, >+ mCacheInfo, >+ mBaseURL) Base URL isn't anything cycle collectable, so no need to add it to traverse/unlink > if (rv == NS_OK) { > script = exec.GetScript(); >+ >+ // Create a ClassicScript object and associate it with the JSScript. >+ RefPtr<ClassicScript> classicScript = >+ new ClassicScript(this, aRequest->mFetchOptions, aRequest->mBaseURL); >+ classicScript->AssociateWithScript(exec.GetScript()); Why two exec.GetScript() calls? Couldn't the latter use 'script' I'd like to see some explanation for the lifetime management here. Please also document that around AssociateWithScript and HostFinalizeTopLevelScript.

Ah, I see there is some cycle collection stuff happening in patch 0.

Comment on attachment 9019712 [details] [diff] [review] bug1342012-5-import-from-classic-scripts ok, just fix the nits. After looking patch 0, memory management looks reasonable.

Comment on attachment 9019700 [details] [diff] [review] bug1342012-1-browser-import-from-modules >+ModuleLoadRequest::ModuleLoadRequest(nsIURI* aURI, >+ ModuleScript* aScript, >+ JS::Handle<JS::Value> aReferencingPrivate, >+ JS::Handle<JSString*> aSpecifier, >+ JS::Handle<JSObject*> aPromise) >+ : ScriptLoadRequest(ScriptKind::eModule, >+ aURI, >+ aScript->FetchOptions(), >+ SRIMetadata(), >+ aScript->BaseURL()), >+ mIsTopLevel(true), >+ mIsDynamicImport(true), It is rather magical for the reader to know which of the ModuleLoadRequest constructors create an object for dynamic import. Could there be inheriting classes DynamicModuleLoadRequest and StaticModuleLoadRequest or some such and constructors on ModuleLoadRequest would be protected. Or enum type variable telling which kind of request is created and then assert the type is passed. Anyhow, something which tells the reader of new ModuleLoadRequest(...) code what thing actually is created. > void > ModuleScript::UnlinkModuleRecord() > { > // Remove module's back reference to this object request if present. > if (mModuleRecord) { > MOZ_ASSERT(JS::GetModulePrivate(mModuleRecord).toPrivate() == this); > JS::SetModulePrivate(mModuleRecord, JS::UndefinedValue()); > mModuleRecord = nullptr; >+ Release(); Please document where the relevant AddRef is happening. > ModuleScript::SetModuleRecord(JS::Handle<JSObject*> aModuleRecord) > { > MOZ_ASSERT(!mModuleRecord); > MOZ_ASSERT(!HasParseError()); > MOZ_ASSERT(!HasErrorToRethrow()); > > mModuleRecord = aModuleRecord; > >- // Make module's host defined field point to this module script object. >- // This is cleared in the UnlinkModuleRecord(). >+ // Make module's host defined field point to this module script object and >+ // increment our reference count. > JS::SetModulePrivate(mModuleRecord, JS::PrivateValue(this)); > HoldJSObjects(this); >+ AddRef(); Please document where the relevant Release is happening. >+class MOZ_RAII AutoSetProcessingScriptTag >+{ >+ nsCOMPtr<nsIScriptContext> mContext; >+ bool mOldTag; >+ >+public: >+ explicit AutoSetProcessingScriptTag(nsIScriptContext* aContext) >+ : mContext(aContext) >+ , mOldTag(mContext->GetProcessingScriptTag()) >+ { >+ mContext->SetProcessingScriptTag(true); >+ } >+ >+ ~AutoSetProcessingScriptTag() >+ { >+ mContext->SetProcessingScriptTag(mOldTag); >+ } >+}; >+ This isn't used in the patch at all. Remove or explain where it will be used. (and member variables should be after methods in a class) > // Streams API > pref("javascript.options.streams", false); > >+// Dynamic module import. >+pref("javascript.options.dynamicImport", false); I guess there will be separate patch to enable this on Nightly or so? Rather minor things, but I'd like to read this code again anyhow. I could have missed something.

Comment on attachment 9019683 [details] [diff] [review] bug1342012-0-script-private-support Looks like you spotted this already, but formally requesting review for cycle collector changes.

> It is rather magical for the reader to know which of the ModuleLoadRequest constructors create an object for dynamic import Agreed. I added static methods to create requests for different purposes. > I guess there will be separate patch to enable this on Nightly or so? Yes, I'll land it preffed off start start with and enable later.

I removed the CC macros for ClassicScript, but as far as I can tell they are needed for ModuleScript because this adds new JS GC things to trace. I find the different macros pretty confusing though so maybe this is unnecessary?

Comment on attachment 9019704 [details] [diff] [review] bug1342012-4-refactor-exec-context Requesting additional review since these are browser changes.

Updated patch with nits fixed, carrying r+.

Patch to support import() in timeout handlers by associating the initiating script with the compiled JSScript. This adds a LoadedScript member to nsJSScriptTimeoutHandler to track the data necessary to perform the import.

Currently we have an nsIScriptElement field in ScriptFetchOptions. This is the script element that a fetch is for, and is used when opening a network channel to fetch the script and is also made available to the debugger. Dynamic module import will allow a script fetch to happen from inline event handlers and so a fetch can be associated with many kinds of element. This patch changes the type of this field to the more general nsIContent to allow this. I'm not sure whether nsIContent is the right type or if there is something more specific that would be better.

Dynamic module imports may happen in places where there is no active script (e.g. in inline event handlers). This changes ResolveImportedModule, HostImportModuleDynamically and associated functions to allow this situation and fall back on defaults based on the current document.

Similar to patch 7, add a LoadedScript field to event listeners. When compiling an event handler associate this with the compiled JSScript to provide the required context for any import() calls.

Finally, enable a bunch of WPT tests that are now passing.

(review is coming. Sorry about the delay)

(In reply to Jon Coppeard (:jonco) from comment #33) > Created attachment 9023337 [details] [diff] [review] > bug1342012-10-associate-event-handlers > > Similar to patch 7, add a LoadedScript field to event listeners. When > compiling an event handler associate this with the compiled JSScript to > provide the required context for any import() calls. So what happens when a web page is loaded. There isn't any script running at that point, yet most of the event handlers are set at that point. And which script should be associated if, for example script in page B sets event handler on page A? Or If page A runs a script which calls a script in page B which sets an event handler on page A?

Comment on attachment 9023302 [details] [diff] [review] bug1342012-7-associate-timeout-handlers >+LoadedScript* >+ScriptLoader::GetActiveScript(JSContext* aCx) >+{ >+ JS::Value value = JS::GetScriptedCallerPrivate(aCx); >+ if (value.isUndefined()) { >+ return nullptr; >+ } >+ >+ RefPtr<LoadedScript> script = static_cast<LoadedScript*>(value.toPrivate()); >+ if (script->Loader() != this) { Why this? What if a script in page A is calling windowB.setTimeout or so? Per spec "Assert: initiating script is not null, since this algorithm is always called from some script." for setTimeout. What am I missing here? >+void >+ScriptLoader::AssociateTimeoutHandlerWithScript(JSContext* aCx, >+ JS::Handle<JSScript*> aScript, >+ LoadedScript* aInitiatingScript) >+{ >+ if (aInitiatingScript) { >+ aInitiatingScript->AssociateWithScript(aScript); >+ } >+} This looks really odd method. It calls a method on an argument if non-null. Doesn't seem to depend on ScriptLoader itself, and doesn't use aCx. Perhaps the method should be static? wpt doesn't seem to have tests for multiple windows/iframes, as far as I see :/

Comment on attachment 9023337 [details] [diff] [review] bug1342012-10-associate-event-handlers The same questions which applies to timers apply here too >+ScriptLoader::AssociateEventHandlerWithScript(JSContext* aCx, >+ nsIContent* aElement, >+ JS::Handle<JSObject*> aHandler, >+ LoadedScript* aInitiatingScript) >+{ >+ MOZ_ASSERT(JS_ObjectIsFunction(aCx, aHandler)); >+ >+ if (aInitiatingScript) { >+ JS::Rooted<JSFunction*> func(aCx, reinterpret_cast<JSFunction*>(aHandler.get())); >+ JS::Rooted<JSScript*> script(aCx, JS_GetFunctionScript(aCx, func)); >+ aInitiatingScript->AssociateWithScript(script); >+ } This is also weird. The method doesn't do anything with 'this'. Perhaps the method should be static? I'd like to understand how this all should work when calls cross window boundaries.

Comment on attachment 9023335 [details] [diff] [review] bug1342012-9-allow-no-referring-script >+++ b/dom/script/ScriptLoader.h >@@ -341,16 +341,21 @@ public: > * any references to the JSScript or to the Request which might be used for > * caching the encoded bytecode. > */ > void Destroy() > { > GiveUpBytecodeEncoding(); > } > >+ static void ResolveImportedModule(JSContext* aCx, >+ JS::Handle<JS::Value> aReferencingPrivate, >+ JS::Handle<JSString*> aSpecifier, >+ JS::MutableHandle<JSObject*> aModuleOut); >+ the 2nd and 3rd argument names aren't too clear. At least document this method > void AssociateTimeoutHandlerWithScript(JSContext* aCx, > JS::Handle<JSScript*> aScript, > LoadedScript* aInitiatingScript); > >+ nsIDocument* GetDocument() const { nit, { should be on its own line

Comment on attachment 9023304 [details] [diff] [review] bug1342012-8-make-request-element-generic I'd prefer to see Element not nsIContent... except, how is this supposed to work with <body onclick="...">? That event handler is added to window object. But nsIContent feels definitely a bit weird.

Comment on attachment 9023295 [details] [diff] [review] bug1342012-1-browser-import-from-modules v2 >+ >+ for (ScriptLoadRequest* req = mDynamicImportRequests.getFirst(); req; >+ req = req->getNext()) { nit, align req

Btw, once all the parts have r+, I'd like to still see a patch which has all the changes. That way it is easier to see the big picture.

Really exciting to see this work in progress! I just want to make sure that everyone participating in this implementation is aware that Test262 has 577 dynamic imports tests: https://github.com/tc39/test262/tree/master/test/language/expressions/dynamic-import

Thanks for reminding about those. wpt is clearly missing tests, as I said in comment 37, and I don't expect tc39 tests to cover those cases.

(In reply to Olli Pettay [:smaug] (high review load) from comment #36) Thanks for all the reviews. > So what happens when a web page is loaded. There isn't any script running at > that point, yet most of the event handlers are set at that point. In that case there is no active script and subsequent import should use the document's base URL etc. I couldn't find the part of the spec that describes the behaviour I've implemented, and it turns out that it's not yet there (this is related to https://github.com/whatwg/html/issues/3295 ), although there are web platform tests for it. I think the best thing to do is to implement the current spec until the changes are finalised. I'll look into making this patch implement what's currently there and splitting the rest out into a separate bug. > And which > script should be associated if, for example script in page B sets event > handler on page A? Or If page A runs a script which calls a script in page B > which sets an event handler on page A? I think the idea would be that the script in page B would be associated in both cases.

Updated patch. This allows setTimeout calls between windows as per spec and makes ScriptLoader::AssociateTimeoutHandlerWithScript static, moving the if-statement to the caller.

Patch to allow the element associated with a load request to be null. This can happen when a event handler fires as we don't have a referencing script or module in that case, and also some situations with promises. This is different to the previous patch which made the type of the element more general and used the root element in this case. This seemed like overkill just for this. We may need to do this later if the spec changes to make setting an event handler work more like setTimeout. The patch adds a separate flag to tell if a request is a preload request as this previously used the element being null to indicate this.

Updated patch.

Comment on attachment 9023337 [details] [diff] [review] bug1342012-10-associate-event-handlers This patch is no longer required.

I'll pop in to say: Jon, the issues we discussed over IRC a while back around event listeners should be more properly specced now. https://github.com/whatwg/html/pull/4181 has the spec changes, and https://github.com/web-platform-tests/wpt/pull/14157 has some for-now-manual tests. Making the tests non-manual is tricky, because the spec was actually already well-defined if there was any JS on the stack at all, e.g. if you call el.onclick() or el.click(). It's specifically the case of the user clicking on something, and thus triggering the event listener with no JS code on the stack, that was poorly-defined. And that's hard to automate in WPT as testharness.js seems to at least sometimes put JS on the stack. Hope this helps!

Comment on attachment 9026787 [details] [diff] [review] bug1342012-7-associate-timeout-handlers v2 >+void >+ScriptLoader::AssociateTimeoutHandlerWithScript(JS::Handle<JSScript*> aScript, >+ LoadedScript* aInitiatingScript) >+{ >+ MOZ_ASSERT(aInitiatingScript); >+ aInitiatingScript->AssociateWithScript(aScript); >+} A bit odd method name when it doesn't do anything with timeout handler. But why we even need this method. This just calls a method on LoadedScript. (I'm still trying to understand why we associate LoadedScript with JSScript coming from timeout and not vice versa. But I think I need to see the full patch.)

Combined patch for the above changes. Sorry, I meant to upload this sooner!

(In reply to Olli Pettay [:smaug] (high review load) from comment #51) > A bit odd method name when it doesn't do anything with timeout handler. That's a fair comment. > But why we even need this method. This just calls a method on LoadedScript. I wanted to minimise the exposure of LoadedScript and have everything interact with ScriptLoader. But yes, I could get rid of this just have the client call the method on the LoadedScript. > (I'm still trying to understand why we associate LoadedScript with JSScript > coming from timeout and not vice versa. But I think I need to see the full > patch.) The LoadedScript pointer is the data that the JS engine passes back to the embedding when asking it to load a dynamic import, so this needs to be associated with the JSScript. Is that what you meant?

Comment on attachment 9026789 [details] [diff] [review] bug1342012-8-make-request-element-optional >+ void UnsetPreload(nsIScriptElement* aElement) > { > // Called when a preload request is later used for an actual request. > MOZ_ASSERT(aElement); > MOZ_ASSERT(!Element()); >+ MOZ_ASSERT(IsPreload()); > mFetchOptions->mElement = aElement; >+ mFetchOptions->mIsPreload = false; >+ } I'm having great trouble to understand this method. Ok, so we mark fetch options as not-preload, but what is that mElement = aElement about? (I think it is about re-associating with aElement, but this API is hard to understand) >+ >+ FromParser GetParserCreated() const { { should be on its own line > if (aRequest->mIsInline) { > AccumulateCategorical(LABELS_DOM_SCRIPT_LOADING_SOURCE::Inline); >- nsAutoString inlineData; >- aRequest->Element()->GetScriptText(inlineData); hups :) I'd like to see some clarification to UnsetPreload. Perhaps it is just renaming the method

(In reply to Olli Pettay [:smaug] (high review load) from comment #54) > I'm having great trouble to understand this method. Ok, so we mark fetch > options as not-preload, but > what is that mElement = aElement about? (I think it is about re-associating > with aElement, but this API is hard to understand) When we create a preload request there is not yet any element to associate it with, so the element is null. If we later go to load a script for an element and find that we already have a preload request that satisfies it, we take that preload request and set its element and make it no longer a preload request. That's what this method is doing. Maybe this should be called 'TransformFromPreloadToActualRequest' but that's not very pithy... suggestions welcome.

Comment on attachment 9026791 [details] [diff] [review] bug1342012-9-allow-no-referring-script v2 >+static ScriptLoader* >+GetCurrentScriptLoader() >+{ >+ nsIGlobalObject* global = GetCurrentGlobal(); Why can't we get the ScriptLoader from the global of the current realm or something like that? Looks like all the callers have JSContext around. Something like https://searchfox.org/mozilla-central/rev/876022232b15425bb9efde189caf747823b39567/dom/script/ScriptSettings.cpp#256-261 (In general I'd prefer us to use less these 'get whatever was pushed to stack' APIs) Or have we entered into wrong realm already in case we're dealing multiple windows? I sure hope we have tests covering using multiple windows. >+ MOZ_ASSERT(global); >+ >+ nsCOMPtr<nsPIDOMWindowInner> win = do_QueryInterface(global); >+ nsGlobalWindowInner* innerWindow = nsGlobalWindowInner::Cast(win); >+ >+ nsIDocument* document = innerWindow->GetDocument(); ...and I don't know what actually guarantees we have window object here. (bear with me. This is all is rather large change, so I need to process this in couple of steps.)

(In reply to Olli Pettay [:smaug] (high review load) from comment #56) > Why can't we get the ScriptLoader from the global of the current realm or > something like that? Yes that would make sense, I just didn't think of it. > I sure hope we have tests covering using multiple windows. I'll write some tests to cover this, although it's not totally clear to me what the behaviour should be.

Hi, glad to see work is in progress. May I ask what is an estimate time this lands in FF (by default, not behind the flag)? I am very eager to see this :).

Updated patch with SetPreload/UnsetPreload renamed to SetIsPreloadRequest/SetIsLoadRequest and some comments added. Hopefully this is clearer.

Updated patch to get the loader object from the current context. This removes the mLoader field from LoadedScript.

Comment on attachment 9031222 [details] [diff] [review] bug1342012-9-allow-no-referring-script r+, but we really need tests for multiple window objects. Hopefully wpt tests so that it is easy to run them in multiple browsers.

Fix a bug where we associate classic scripts we parse, but not ones we decode from cached bytecode.

Fix the error message that's used for all import() failures where there's no JS exception pending.

To make import() between multiple windows work, we need to be able to find the introduction script's SSO when it's in another global. This is a problem right now because we don't store cross-global introduction scripts in the SSO. This patch adds a new slot to store a CCW to the introduction script's SSO I considered making the existing introduction script slot store either the script or the SSO if it was in another compartment but this seems clearer.

Comment on attachment 9032170 [details] [diff] [review] bug1342012-14-add-introduction-sso-slot Review of attachment 9032170 [details] [diff] [review]: ----------------------------------------------------------------- Makes sense. ::: js/src/vm/JSScript.h @@ +1169,5 @@ > return nullptr; > } > return value.toGCThing()->as<JSScript>(); > } > + JSObject* introductionSourceObject() const { I was going to suggest unwrappedIntroductionSourceObject: it's a long name but it matches the other unwrapped* methods in this class. However the caller has to unwrap too.. I wonder if we should move that UncheckedUnwrap into this code and call it unwrappedIntroductionSourceObject.

Hi all! Happy to see progress on this. May I ask: Is this already available for JS Developers to test in FF Nightly? Maybe behind a Flag/about:config/etc.? If yes, it's appearently too well hidden - care to point it out? If no: Any idea when it will be? Thanks! :)

Associate event handlers with the active script when they are compiled, so that import() in event handlers works. I checked with Domenic that this is the correct interpretation of the spec, although it was surprising to me.

Finally, remove a check that prevents import() working when the active script is in another document and add tests for this.

(In reply to Daniel Bruckhaus from comment #66) > Hi all! Happy to see progress on this. > > May I ask: > Is this already available for JS Developers to test in FF Nightly? Maybe > behind a Flag/about:config/etc.? Features are generally not available in nightlies until the bugs in question are marked as FIXED.

Comment on attachment 9032655 [details] [diff] [review] bug1342012-16-allow-cross-document-import >+ const evaluators = { >+ eval: otherWindow.eval, >+ setTimeout: otherWindow.setTimeout, >+ "the Function constructor"(x) { >+ otherWindow.Function(x)(); >+ }, >+ "reflected inline event handlers"(x) { >+ otherDiv.setAttribute("onclick", x); >+ otherDiv.onclick(); >+ }, >+ "inline event handlers triggered via UA code"(x) { >+ otherDiv.setAttribute("onclick", x); >+ otherDiv.click(); // different from .**on**click() >+ } This isn't really the same as triggered via UA. When click is triggered via UA, there isn't any JS on stack. (IIRC existing wpt tests have the same issue, but isn't there some way to actually dispatch proper click in wpt. jgraham would know) So, at least change the name of that evaluator

Comment on attachment 9032654 [details] [diff] [review] bug1342012-15-associate-event-handlers > >+ JS::Rooted<JSFunction*> func(cx, JS_GetObjectFunction(handler)); >+ MOZ_ASSERT(func); >+ JS::Rooted<JSScript*> jsScript(cx, JS_GetFunctionScript(cx, func)); >+ MOZ_ASSERT(jsScript); >+ RefPtr<ScriptLoader> loader = aElement->OwnerDoc()->ScriptLoader(); >+ RefPtr<LoadedScript> loaderScript = loader->GetActiveScript(cx); Is this really right? What if some script running in another window (B) does something like windowA.document.body.firstElementChild.setAttribute("onclick", "/*js code*/"); >+ if (loaderScript) { >+ loaderScript->AssociateWithScript(jsScript); >+ } >+ Also, how does this work with the defer == true in EventListenerManager::SetEventHandler? Should we rather take the LoadedScript in EventListenerManager::SetEventHandler?

(In reply to Jan de Mooij [:jandem] from comment #65) > I wonder if we should move that > UncheckedUnwrap into this code and call it unwrappedIntroductionSourceObject. Good idea. This works out simpler.

(In reply to Olli Pettay [:smaug] (away-ish Dec 21-30) from comment #71) > >+ RefPtr<ScriptLoader> loader = aElement->OwnerDoc()->ScriptLoader(); > >+ RefPtr<LoadedScript> loaderScript = loader->GetActiveScript(cx); > Is this really right? What if some script running in another window (B) > does something like > windowA.document.body.firstElementChild.setAttribute("onclick", "/*js > code*/"); GetActiveScript() is static and uses the stack to find the current active script, so this should get the script running in the other window. If I understand your comment correctly the tests from patch 16 exercise this. I've taken out the unnecessary use of |loader|. > Also, how does this work with the defer == true in > EventListenerManager::SetEventHandler? > Should we rather take the LoadedScript in > EventListenerManager::SetEventHandler? The spec says to use the active script when the function is compiled (this happens inside FunctionCreate in step 10) not when the event hander is set. I agree this is a bit weird. https://html.spec.whatwg.org/#getting-the-current-value-of-the-event-handler

(In reply to Olli Pettay [:smaug] (away-ish Dec 21-30) from comment #70) > This isn't really the same as triggered via UA. When click is triggered via > UA, there isn't > any JS on stack. (IIRC existing wpt tests have the same issue, but isn't > there some way to actually dispatch proper click in wpt. jgraham would know) > So, at least change the name of that evaluator Ah good point. I just copied the existing test code, but I'll change this one to be correct at least.

Comment on attachment 9032897 [details] [diff] [review] bug1342012-15-associate-event-handlers v2 ok, this is crazy, but the spec is :/ Please ensure there is a spec bug open for this.

(In reply to Olli Pettay [:smaug] from comment #75) Raised https://github.com/whatwg/html/issues/4267

Pushed by jcoppeard@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2f619be44798 Support script and module private values which contain pointers to cycle-collected C++ objects r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/8e6f86cd811a Initial browser support for dynamic import from module scripts r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/c73b0641fc8e Refactor ModuleScript into ClassicScript class and LoadedScript base class so we can represent all scripts that can perform dynamic import r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/400145308204 Rename ModuleScript source files to LoadedScript r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/92863bfc60c0 Refactor nsJSUtils::ExecutionContext to separate compilation and execution steps and allow extraction of compiled JSScript r=nbp r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/7b1979745763 Support dynamic import from classic scripts by creating ClassicScript objects and associating them with the compiled JSScriptsr r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/0833244e1e01 Add a JS API to get the private value for the calling script or module r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/65742a003296 Support import from timeout handlers by associating the initiating script with the compiled JSScript r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/248ed24187a2 Make load request element optional r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/be8cd28e487c Allow dynamic import in cases where there's no referencing script or module r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/c89629ed81de Fix WPT expectatons for dynamic import tests r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/34e605a49937 Also associate classic scripts from the bytecode cache r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/43a79c5e38f4 Fix error message that covers all import() failures that don't throw a JS exception r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/85c9dc639077 Store a CCW to the introuction script's script source object r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/83937d7bd335 Associate event handler with active script when they are compiled r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/5ad9c5b505d3 Make import() work when the active script is in another document r=smaug

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14698 for changes under testing/web-platform/tests

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

https://hg.mozilla.org/mozilla-central/rev/2f619be44798 https://hg.mozilla.org/mozilla-central/rev/8e6f86cd811a https://hg.mozilla.org/mozilla-central/rev/c73b0641fc8e https://hg.mozilla.org/mozilla-central/rev/400145308204 https://hg.mozilla.org/mozilla-central/rev/92863bfc60c0 https://hg.mozilla.org/mozilla-central/rev/7b1979745763 https://hg.mozilla.org/mozilla-central/rev/0833244e1e01 https://hg.mozilla.org/mozilla-central/rev/65742a003296 https://hg.mozilla.org/mozilla-central/rev/248ed24187a2 https://hg.mozilla.org/mozilla-central/rev/be8cd28e487c https://hg.mozilla.org/mozilla-central/rev/c89629ed81de https://hg.mozilla.org/mozilla-central/rev/34e605a49937 https://hg.mozilla.org/mozilla-central/rev/43a79c5e38f4 https://hg.mozilla.org/mozilla-central/rev/85c9dc639077 https://hg.mozilla.org/mozilla-central/rev/83937d7bd335 https://hg.mozilla.org/mozilla-central/rev/5ad9c5b505d3

Note that this feature was landed as disabled by default behind the javascript.options.dynamicImport pref. See bug 1517546.

Upstream PR merged