1726711 - Crash in [@ mozilla::dom::cache::Manager::Factory::ShutdownAll]

Status: RESOLVED WORKSFORME

(Core :: Storage: Cache API, defect)

Description

[Jens Stutte [:jstutte]]

Maybe Fission related. (DOMFissionEnabled=1)

Crash report: https://crash-stats.mozilla.org/report/index/7f749705-6916-4ab3-a010-46cdf0210628

MOZ_CRASH Reason: MOZ_DIAGNOSTIC_ASSERT(!sFactory->mManagerList.IsEmpty())

Top 10 frames of crashing thread:

0 xul.dll static mozilla::dom::cache::Manager::Factory::ShutdownAll dom/cache/Manager.cpp:306
1 xul.dll mozilla::dom::quota::QuotaManager::Shutdown dom/quota/ActorsParent.cpp:3712
2 xul.dll static mozilla::dom::quota::QuotaManager::ShutdownInstance dom/quota/ActorsParent.cpp:3296
3 xul.dll mozilla::dom::quota::RecvShutdownQuotaManager dom/quota/ActorsParent.cpp:2681
4 xul.dll mozilla::ipc::BackgroundParentImpl::RecvShutdownQuotaManager ipc/glue/BackgroundParentImpl.cpp:1090
5 xul.dll mozilla::ipc::PBackgroundParent::OnMessageReceived ipc/ipdl/PBackgroundParent.cpp:4468
6 xul.dll mozilla::ipc::MessageChannel::DispatchMessage ipc/glue/MessageChannel.cpp:2012
7 xul.dll mozilla::ipc::MessageChannel::MessageTask::Run ipc/glue/MessageChannel.cpp:1891
8 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1146
9 xul.dll mozilla::ipc::MessagePumpForNonMainThreads::Run ipc/glue/MessagePump.cpp:330

Comment 1

[Jens Stutte [:jstutte]]

Not sure if this is indicating some unwanted re-entrance or just a situation where we shutdown without any Manager/Context yet created.

Comment 2

[Haiyang Xu]

(In reply to Jens Stutte [:jstutte] from comment #1)

Not sure if this is indicating some unwanted re-entrance or just a situation where we shutdown without any Manager/Context yet created.
It might be the situation of shutdown before creation, since from the crash-stats, the uptime is just 30 seconds.
How long does it take for the Manager/Context to be created?

Comment 3

[Jens Stutte [:jstutte]]

There are other reports with much longer up-times. Still these objects are created lazily on (first) demand, so this does not really exclude anything.

Comment 4

[Haiyang Xu]

Actually there are two categories in this crash. 81894d24-ebd5-4c12-a664-e8ad10210731 and 84add856-bfc3-400c-b651-037670210717 are crashing at the line of static void ShutdownAll()

However, 7f749705-6916-4ab3-a010-46cdf0210628 and 165bb46a-9216-4262-a8ba-4ae620210626 are crashing at the line of MOZ_DIAGNOSTIC_ASSERT(!sFactory->mManagerList.IsEmpty());

I guess there are different reasons. Still investigating...

Comment 5

[Jens Stutte [:jstutte]]

While looking at 81894d24-ebd5-4c12-a664-e8ad10210731, I noticed the following:

  if (gShutdown.exchange(true)) {
    NS_ERROR("Shutdown more than once?!");
  }

This reads as if we want to avoid re-entrance, but NS_ERROR seems to be defined to do something only in debug builds. We should at least add a return here (and might want to combine this with a QM_TRY to get telemetry, maybe).

Comment 6

[Haiyang Xu]

(In reply to Jens Stutte [:jstutte] from comment #5)

While looking at 81894d24-ebd5-4c12-a664-e8ad10210731, I noticed the following:

  if (gShutdown.exchange(true)) {
    NS_ERROR("Shutdown more than once?!");
  }

This reads as if we want to avoid re-entrance, but NS_ERROR seems to be defined to do something only in debug builds. We should at least add a return here (and might want to combine this with a QM_TRY to get telemetry, maybe).

I'll create a patch to address this.

Comment 7

[Haiyang Xu]

Attachment: Bug 1726711 - Replace NS_ERROR with QM_TRY and return immediately in case of shutdown re-entrance; r=#dom-storage-reviewers,jstutte

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eb73de629b26
Replace NS_ERROR with QM_TRY and return immediately in case of shutdown re-entrance; r=jstutte,dom-storage-reviewers

Comment 9

[Noemi Erli[:noemi_erli]]

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

Comment 10

[Haiyang Xu]

Attachment: Bug 1726711 - Return immediately in shutdown re-entrance; r=#dom-storage-reviewers

Comment 11

[Phabricator Automation]

Comment on attachment 9248331 [details]
Bug 1726711 - Return immediately in shutdown re-entrance; r=#dom-storage-reviewers

Revision D129877 was moved to bug 1726714. Setting attachment 9248331 [details] to obsolete.

Comment 12

[Jens Stutte [:jstutte]]

I think we are done here.