Now, we propagate the symbols instead of networking errors. We should propagate the errors in the promise pipeline. And we should explore the other way to suppress duplicate error reporting.
Created attachment 300037 [details] Patch
Created attachment 300038 [details] Patch
Comment on attachment 300038 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=300038&action=review > Source/WebCore/bindings/js/HostErrorKind.h:30 > +enum class HostErrorKind { Can't we just call this ModuleFetchFailureKind? > Source/WebCore/bindings/js/HostErrorKind.h:32 > + ModuleFetchWasErrored, > + ModuleFetchWasCanceled, And remove "ModuelFetch" prefix from these? > Source/WebCore/bindings/js/ScriptController.cpp:395 > + JSValue errorKindValue = object->getDirect(vm, static_cast<JSVMClientData&>(*vm.clientData).builtinNames().errorKindPrivateName()); > + if (errorKindValue) { Please define errorKindValue inside if (~). > Source/WebCore/bindings/js/ScriptController.cpp:404 > + case HostErrorKind::ModuleFetchWasErrored: { > + moduleScript->notifyLoadFailed(LoadableScript::Error { > + LoadableScript::ErrorType::CachedScript, > + std::nullopt > + }); > + break; > + } We don't need curly braces here since there is no variable declaration. > Source/WebCore/bindings/js/ScriptController.cpp:408 > + case HostErrorKind::ModuleFetchWasCanceled: { > + moduleScript->notifyLoadWasCanceled(); > + break; > + } Ditto. > Source/WebCore/bindings/js/ScriptModuleLoader.cpp:158 > + // Reject a promise to propagate the error back all the way through module promise chains to the script element. Instead of adding a comment, why don't we add a helper function with a descriptive name like: rejectToPropagateNetworkError(deferred, HostErrorKind::ModuleFetchWasErrored); return jsPromise. > LayoutTests/http/tests/security/mixedContent/import-insecure-script-in-iframe.html:7 > +trigger a mixed content callback even though the main frame is HTTP because the > +HTTPS frame's origin is contaminated with an insecure module script.`); I think it's quite irrelevant whether the main frame is HTTP or no. I think it's better to just state that since iframe's content is of a secure origin, importing a module script via HTTP should result in a mixed content error.
Comment on attachment 300038 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=300038&action=review Thanks! >> Source/WebCore/bindings/js/HostErrorKind.h:30 >> +enum class HostErrorKind { > > Can't we just call this ModuleFetchFailureKind? OK, changed. >> Source/WebCore/bindings/js/HostErrorKind.h:32 >> + ModuleFetchWasCanceled, > > And remove "ModuelFetch" prefix from these? Done. >> Source/WebCore/bindings/js/ScriptController.cpp:395 >> + if (errorKindValue) { > > Please define errorKindValue inside if (~). Done. >> Source/WebCore/bindings/js/ScriptController.cpp:404 >> + } > > We don't need curly braces here since there is no variable declaration. OK, done. >> Source/WebCore/bindings/js/ScriptController.cpp:408 >> + } > > Ditto. Done. >> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:158 >> + // Reject a promise to propagate the error back all the way through module promise chains to the script element. > > Instead of adding a comment, why don't we add a helper function with a descriptive name like: > rejectToPropagateNetworkError(deferred, HostErrorKind::ModuleFetchWasErrored); > return jsPromise. Nice. I've extracted these part to a static function named "rejectToPropagateNetworkError". >> LayoutTests/http/tests/security/mixedContent/import-insecure-script-in-iframe.html:7 >> +HTTPS frame's origin is contaminated with an insecure module script.`); > > I think it's quite irrelevant whether the main frame is HTTP or no. > I think it's better to just state that since iframe's content is of a secure origin, > importing a module script via HTTP should result in a mixed content error. OK, the description is changed.
Committed r211473: <http://trac.webkit.org/changeset/211473>
(In reply to comment #5) > Committed r211473: <http://trac.webkit.org/changeset/211473> It broke the build everywhere, see build.webkit.org for details.
(In reply to comment #6) > (In reply to comment #5) > > Committed r211473: <http://trac.webkit.org/changeset/211473> > > It broke the build everywhere, see build.webkit.org for details. Fixed with the follow-up patches.