1735695 - OOM for JS::CreateModuleRequest isn't handled correctly in ScriptLoader::FinishDynamicImport

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Tooru Fujisawa [:arai]]

ScriptLoader::FinishDynamicImport calls JS::CreateModuleRequest and reports OOM on error, but JS::CreateModuleRequest is supposed to set pending exception on error, so we shouldn't have to report error here.

Also, it falls through on error case, and JS::FinishDynamicModuleImport dereferences moduleRequest without null-checking.
we should early return, or handle null case.

https://searchfox.org/mozilla-central/rev/0998b61022a1ece43afe3c750077e5804c6c6392/dom/script/ScriptLoader.cpp#1244-1251

void ScriptLoader::FinishDynamicImport(
    JSContext* aCx, ModuleLoadRequest* aRequest, nsresult aResult,
    JS::Handle<JSObject*> aEvaluationPromise) {
...
  JS::Rooted<JSObject*> moduleRequest(aCx,
                                      JS::CreateModuleRequest(aCx, specifier));
  if (!moduleRequest) {
    JS_ReportOutOfMemory(aCx);
  }

  JS::FinishDynamicModuleImport(aCx, aEvaluationPromise, referencingScript,
                                moduleRequest, promise);

https://searchfox.org/mozilla-central/rev/0998b61022a1ece43afe3c750077e5804c6c6392/js/src/vm/Modules.cpp#75-85

JS_PUBLIC_API bool JS::FinishDynamicModuleImport(
    JSContext* cx, Handle<JSObject*> evaluationPromise,
    Handle<Value> referencingPrivate, Handle<JSObject*> moduleRequest,
    Handle<JSObject*> promise) {
...
  return js::FinishDynamicModuleImport(
      cx, evaluationPromise, referencingPrivate, moduleRequest, promise);

https://searchfox.org/mozilla-central/rev/0998b61022a1ece43afe3c750077e5804c6c6392/js/src/builtin/ModuleObject.cpp#2471-2494

bool js::FinishDynamicModuleImport(JSContext* cx,
                                   HandleObject evaluationPromise,
                                   HandleValue referencingPrivate,
                                   HandleObject moduleRequest,
                                   HandleObject promiseArg) {
...
  if (!evaluationPromise) {
...
  }
...
  if (!FinishDynamicModuleImport_impl(cx, evaluationPromise, referencingPrivate,
                                      moduleRequest, promiseArg)) {
    return false;
  }

https://searchfox.org/mozilla-central/rev/0998b61022a1ece43afe3c750077e5804c6c6392/js/src/builtin/ModuleObject.cpp#2436-2469

bool FinishDynamicModuleImport_impl(JSContext* cx,
                                    HandleObject evaluationPromise,
                                    HandleValue referencingPrivate,
                                    HandleObject moduleRequest,
                                    HandleObject promiseArg) {
  Rooted<ListObject*> resolutionArgs(cx, ListObject::create(cx));
  if (!resolutionArgs->append(cx, referencingPrivate)) {
    return false;
  }
  Rooted<Value> stringValue(
      cx, StringValue(moduleRequest->as<ModuleRequestObject>().specifier()));

Comment 1

[Jon Coppeard (:jonco)]

(In reply to Tooru Fujisawa [:arai] from comment #0)

Also, it falls through on error case, and JS::FinishDynamicModuleImport dereferences moduleRequest without null-checking.
we should early return, or handle null case.

It's been a while since I looked at this code, but we need to call FinishDynamicModuleImport regardless of success/failure, so we shouldn't return early.

Maybe we should handle this the same way as lack of evaluationPromise in FinishDynamicModuleImport: https://searchfox.org/mozilla-central/rev/0998b61022a1ece43afe3c750077e5804c6c6392/js/src/builtin/ModuleObject.cpp#2482

Comment 2

[Jonatan Klemets [:jon4t4n]]

Attachment: Bug 1735695 - Remove OOM error in FinishDynamicImport when CreateModuleRequest fails. r=arai

As stated in the bug we do not need to report OOM on error since this is handled
by CreateModuleRequest.

Comment 3

[Jonatan Klemets [:jon4t4n]]

Attachment: Bug 1735695 - Reject the dynamic import if we do not have a module request. r=arai

As stated by jonco in the bug, we always need to call FinishDynamicModuleImport,
so we can't null check and have an early return.

This patch adds a null check and rejects the dynamic import if we do not have a
module request. The rejection is handled the same as not having an evaluationPromise.

Depends on D134707

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cc4183106e2c
Remove OOM error in FinishDynamicImport when CreateModuleRequest fails. r=arai
https://hg.mozilla.org/integration/autoland/rev/1fe36f69d3e1
Reject the dynamic import if we do not have a module request. r=arai

Comment 5

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/cc4183106e2c
https://hg.mozilla.org/mozilla-central/rev/1fe36f69d3e1