Bug 167501 – Propagate networking errors correctly for import() operator

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 300037 [details]
Patch

[Yusuke Suzuki]

Created attachment 300038 [details]
Patch

[Ryosuke Niwa]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r211473: <http://trac.webkit.org/changeset/211473>

[Csaba Osztrogonác]

(In reply to comment #5)
> Committed r211473: <http://trac.webkit.org/changeset/211473>

It broke the build everywhere, see build.webkit.org for details.

[Yusuke Suzuki]

(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.