1880708 - ModuleLoaderBase's cleanup methods should expect CC unlinks pointers before calling them

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Tooru Fujisawa [:arai]]

bug 1874686 discovers that JS::loader::ModuleLoaderBase::CancelDynamicImport can be called on a ModuleLoadRequest which is already unlinked by CC,
that results in failing the following assertion, because mLoader is set to null by unlink:

https://searchfox.org/mozilla-central/rev/151afcc1c9c44b3634bc76e2bcd40b8e40eb0992/js/loader/ModuleLoaderBase.cpp#1058-1060

void ModuleLoaderBase::CancelDynamicImport(ModuleLoadRequest* aRequest,
                                           nsresult aResult) {
  MOZ_ASSERT(aRequest->mLoader == this);

that happens in the following route:

https://searchfox.org/mozilla-central/rev/151afcc1c9c44b3634bc76e2bcd40b8e40eb0992/dom/base/Document.cpp#2595,2621

NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(Document)
...
  tmp->SetScriptGlobalObject(nullptr);

https://searchfox.org/mozilla-central/rev/151afcc1c9c44b3634bc76e2bcd40b8e40eb0992/dom/base/Document.cpp#7722,7855-7856

void Document::SetScriptGlobalObject(
...
  if (mScriptLoader && !IsTemplateContentsOwner()) {
    mScriptLoader->SetGlobalObject(mScriptGlobalObject);

https://searchfox.org/mozilla-central/rev/151afcc1c9c44b3634bc76e2bcd40b8e40eb0992/dom/script/ScriptLoader.cpp#270-273

void ScriptLoader::SetGlobalObject(nsIGlobalObject* aGlobalObject) {
  if (!aGlobalObject) {
    // The document is being detached.
    CancelAndClearScriptLoadRequests();

https://searchfox.org/mozilla-central/rev/151afcc1c9c44b3634bc76e2bcd40b8e40eb0992/dom/script/ScriptLoader.cpp#1590,1605-1606

void ScriptLoader::CancelAndClearScriptLoadRequests() {
...
  if (mModuleLoader) {
    mModuleLoader->CancelAndClearDynamicImports();

https://searchfox.org/mozilla-central/rev/151afcc1c9c44b3634bc76e2bcd40b8e40eb0992/js/loader/ModuleLoaderBase.cpp#1326-1329

void ModuleLoaderBase::CancelAndClearDynamicImports() {
  while (ScriptLoadRequest* req = mDynamicImportRequests.getFirst()) {
    // This also removes the request from the list.
    CancelDynamicImport(req->AsModuleRequest(), NS_ERROR_ABORT);

https://searchfox.org/mozilla-central/rev/151afcc1c9c44b3634bc76e2bcd40b8e40eb0992/js/loader/ModuleLoaderBase.cpp#1058-1060

void ModuleLoaderBase::CancelDynamicImport(ModuleLoadRequest* aRequest,
                                           nsresult aResult) {
  MOZ_ASSERT(aRequest->mLoader == this);

So, methods which can be called while CC unlink should expect that the unlink already happened for the related objects.

Comment 1

[Tooru Fujisawa [:arai]]

setting to S4 given this is debug-only assertion failure.

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Bug 1880708 - Handle CC unlink when destroying ScriptLoader/ModuleLoader. r?jonco!

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e30a2fca300e
Handle CC unlink when destroying ScriptLoader/ModuleLoader. r=jonco

Comment 4

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/e30a2fca300e