Closed Bug 1534882 Opened 9 months ago Closed 7 months ago

Crash in [@ mozilla::ipc::FatalError | mozilla::ipc::IProtocol::HandleFatalError | mozilla::dom::PBackgroundLSDatabaseChild::SendPBackgroundLSSnapshotConstructor]

Categories

(Core :: Storage: localStorage & sessionStorage, defect, P2, critical)

Unspecified
Android
defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox-esr60 --- unaffected
firefox67 --- wontfix
firefox68 --- fixed

People

(Reporter: janv, Assigned: janv)

References

(Blocks 1 open bug)

Details

(Keywords: crash)

Crash Data

Attachments

(2 files, 1 obsolete file)

This bug is for crash report bp-947d1811-6965-48e8-97ff-356080190313.

Top 10 frames of crashing thread:

0 libxul.so MOZ_Crash mfbt/Assertions.h:314
1 libxul.so mozilla::ipc::FatalError ipc/glue/ProtocolUtils.cpp:264
2 libxul.so mozilla::ipc::IProtocol::HandleFatalError const ipc/glue/ProtocolUtils.cpp:440
3 libxul.so mozilla::dom::PBackgroundLSDatabaseChild::SendPBackgroundLSSnapshotConstructor ipc/glue/ProtocolUtils.cpp:431
4 libxul.so mozilla::dom::LSDatabase::EnsureSnapshot dom/localstorage/LSDatabase.cpp:308
5 libxul.so mozilla::dom::LSObject::SetItem dom/localstorage/LSDatabase.cpp:203
6 libxul.so mozilla::dom::Storage_Binding::setItem dom/bindings/StorageBinding.cpp:217
7 libxul.so bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions> dom/bindings/BindingUtils.cpp:3144
8 libxul.so js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:442
9 libxul.so Interpret js/src/vm/Interpreter.cpp:593

Blocks: 1517090
Priority: -- → P2

Maybe a similar problem as in bug 1533651. SendPBackgroundLSSnapshotConstructor fails because IPC transport is not alive anymore ? The question is, why ActorDestroy wasn't called.

No longer blocks: 1517090
Blocks: 1540402
Crash Signature: [@ mozilla::ipc::FatalError | mozilla::ipc::IProtocol::HandleFatalError | mozilla::dom::PBackgroundLSDatabaseChild::SendPBackgroundLSSnapshotConstructor] → [@ mozilla::ipc::FatalError | mozilla::ipc::IProtocol::HandleFatalError | mozilla::dom::PBackgroundLSDatabaseChild::SendPBackgroundLSSnapshotConstructor] [@ mozilla::dom::PBackgroundLSDatabaseChild::SendPBackgroundLSSnapshotConstructor]

These are all Fennec crashes. Eden, can you help out here, too?

Flags: needinfo?(echuang)
Assignee: nobody → echuang
Status: NEW → ASSIGNED
Flags: needinfo?(echuang)

As a first step, I think we should convert MOZ_ASSERT to MOZ_DIAGNOSTIC_ASSERT here:
https://searchfox.org/mozilla-central/rev/11cfa0462a6b5d8c5e2111b8cfddcf78098f0141/dom/localstorage/LSDatabase.cpp#296

Hm, but if mActor is null, we should crash and not call SendPBackgroundLSSnapshotConstructor. mActor can't be a dangling pointer because mActor is set to null when LSDatabaseChild::ActorDestroy is called.

So I can only think of a case when the IPC channel is already disconnected but ActorDestroy hasn't been called yet, so SendPBackgroundLSSnapshotConstructor fails to send the message and triggers a fatal error.

I agree that the fatal error seems problematic. At the very least, the channel's LastSendError() should be propagated to the call to FatalError() so we have more info in the MOZ_CRASH reason.

Also, it's not clear to me that the problem is MessageChannel::Connected() returning false. This is same-process PBackground and we don't appear to be in shutdown, and MessageChannel::Send[1] seems to have a zillion reasons to return false ( and for which it sets mLastSendError, so it would be great to propagate that).

redirecting NI to :nika for the higher level IPC question (and general IPC mega-expertise :).

1: https://searchfox.org/mozilla-central/rev/11cfa0462a6b5d8c5e2111b8cfddcf78098f0141/ipc/glue/MessageChannel.cpp#1404

Flags: needinfo?(bugmail) → needinfo?(nika)

This was changed for async actor constructors in bug 1509362 for the same "we can't actually do anything about this" issue. It wasn't changed for sync actors to try to be conservative, but that might have been the wrong decision.

Perhaps we should take the same approach here.

Depends on: 1509362
Flags: needinfo?(nika)

This patch changes the way how we handle sync ctor send errors. They are now
ignored and treated like messages which successfully were queued to send, but
got lost due to the other side hanging up.
For more details, see bug 1509362 which originally did it for async ctors.
The main difference here is that we return null when the send fails.

Nika, thanks for the info!
I hope you are ok with my mostly "copy and paste" patch :)

Attachment #9065511 - Attachment is obsolete: true
Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/87325e358912
Don't crash when synchronously constructing actor during content shutdown, r=nika
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.