Crash in [@ mozilla::ipc::FatalError | mozilla::ipc::IProtocol::HandleFatalError | mozilla::dom::PBackgroundLSDatabaseChild::SendPBackgroundLSSnapshotConstructor]
Categories
(Core :: Storage: localStorage & sessionStorage, defect, P2)
Tracking
()
| 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)
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
1.73 KB,
patch
|
Details | Diff | Splinter Review |
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
|
Assignee | |
Updated•5 years ago
|
|
|
Assignee | |
Comment 1•5 years ago
|
||
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.
|
||
Updated•5 years ago
|
|
|
Assignee | |
Updated•5 years ago
|
|
||
Comment 2•5 years ago
|
||
These are all Fennec crashes. Eden, can you help out here, too?
|
||
Updated•5 years ago
|
|
|
Assignee | |
Comment 3•5 years ago
|
||
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
|
|
Assignee | |
Comment 4•5 years ago
|
||
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.
|
|
Assignee | |
Comment 5•5 years ago
|
||
Ah, this is what we have to do:
https://searchfox.org/mozilla-central/rev/11cfa0462a6b5d8c5e2111b8cfddcf78098f0141/dom/media/ipc/RemoteAudioDecoder.cpp#51
|
|
Assignee | |
Comment 6•5 years ago
|
||
It seems that other synchronous constructors have this problem:
https://searchfox.org/mozilla-central/search?q=constructor+for+actor+failed&case=false®exp=false&path=
Especially these two constructors show in crash stats:
https://crash-stats.mozilla.org/search/?signature=~CallPStreamNotifyConstructor&date=%3E%3D2019-05-09T09%3A35%3A00.000Z&date=%3C2019-05-16T09%3A35%3A00.000Z&_facets=signature&_sort=-date&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature
We can call:
if (!mActor->GetIPCChannel()->CanSend()) {
return NS_ERROR_FAILURE;
}
before calling the snapshot constructor, but that's still not 100% safe, because channel can still close in between in theory
I wonder if we should reconsider this fatal error:
https://searchfox.org/mozilla-central/rev/11cfa0462a6b5d8c5e2111b8cfddcf78098f0141/ipc/ipdl/ipdl/lower.py#4186
since normal synchronous messages don't cause a fatal error when they fail to send
Andrew, what do you think ?
|
||
Comment 7•5 years ago
|
||
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 :).
|
||
Comment 8•5 years ago
|
||
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.
|
|
Assignee | |
Comment 9•5 years ago
|
||
|
|
Assignee | |
Comment 10•5 years ago
|
||
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.
|
|
Assignee | |
Comment 11•5 years ago
|
||
Nika, thanks for the info!
I hope you are ok with my mostly "copy and paste" patch :)
|
|
Assignee | |
Comment 12•5 years ago
|
||
|
|
Assignee | |
Comment 13•5 years ago
|
||
|
||
Comment 14•5 years ago
|
||
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
|
||
Comment 15•5 years ago
|
||
| bugherder | ||
|
|
||
Updated•5 years ago
|
|
||
Updated•5 years ago
|
Description
•