Crash/Deadlock fix: Web3DOverlay could be destroyed on wrong thread.

When script calls Entities.getChildrenIDs*() it is possible to deadlock the main thread, and also invoke
the destructor of Entities and Overlays, which is very NOT thread safe.

The fix is to use a pattern already in use in several places in our codebase.
Use the custom deleter parameter of std::shared_ptr to call deleteLater() instead of destroying the object in place.
This allows any thread to use shared_ptrs of SpatiallyNestables without fear.
This commit is contained in:
Anthony J. Thibault 2017-10-11 17:32:34 -07:00
parent a4462056df
commit 0a943fbe7b
14 changed files with 31 additions and 30 deletions

View file

@ -53,7 +53,7 @@ const QUuid MY_AVATAR_KEY; // NULL key
AvatarManager::AvatarManager(QObject* parent) : AvatarManager::AvatarManager(QObject* parent) :
_avatarsToFade(), _avatarsToFade(),
_myAvatar(std::make_shared<MyAvatar>(qApp->thread())) _myAvatar(new MyAvatar(qApp->thread()), [](MyAvatar* ptr) { ptr->deleteLater(); })
{ {
// register a meta type for the weak pointer we'll use for the owning avatar mixer for each avatar // register a meta type for the weak pointer we'll use for the owning avatar mixer for each avatar
qRegisterMetaType<QWeakPointer<Node> >("NodeWeakPointer"); qRegisterMetaType<QWeakPointer<Node> >("NodeWeakPointer");
@ -297,7 +297,7 @@ void AvatarManager::simulateAvatarFades(float deltaTime) {
} }
AvatarSharedPointer AvatarManager::newSharedAvatar() { AvatarSharedPointer AvatarManager::newSharedAvatar() {
return std::make_shared<OtherAvatar>(qApp->thread()); return AvatarSharedPointer(new OtherAvatar(qApp->thread()), [](OtherAvatar* ptr) { ptr->deleteLater(); });
} }
void AvatarManager::handleRemovedAvatar(const AvatarSharedPointer& removedAvatar, KillAvatarReason removalReason) { void AvatarManager::handleRemovedAvatar(const AvatarSharedPointer& removedAvatar, KillAvatarReason removalReason) {

View file

@ -161,33 +161,33 @@ OverlayID Overlays::addOverlay(const QString& type, const QVariant& properties)
Overlay::Pointer thisOverlay = nullptr; Overlay::Pointer thisOverlay = nullptr;
if (type == ImageOverlay::TYPE) { if (type == ImageOverlay::TYPE) {
thisOverlay = std::make_shared<ImageOverlay>(); thisOverlay = Overlay::Pointer(new ImageOverlay(), [](Overlay* ptr) { ptr->deleteLater(); });
} else if (type == Image3DOverlay::TYPE || type == "billboard") { // "billboard" for backwards compatibility } else if (type == Image3DOverlay::TYPE || type == "billboard") { // "billboard" for backwards compatibility
thisOverlay = std::make_shared<Image3DOverlay>(); thisOverlay = Overlay::Pointer(new Image3DOverlay(), [](Overlay* ptr) { ptr->deleteLater(); });
} else if (type == TextOverlay::TYPE) { } else if (type == TextOverlay::TYPE) {
thisOverlay = std::make_shared<TextOverlay>(); thisOverlay = Overlay::Pointer(new TextOverlay(), [](Overlay* ptr) { ptr->deleteLater(); });
} else if (type == Text3DOverlay::TYPE) { } else if (type == Text3DOverlay::TYPE) {
thisOverlay = std::make_shared<Text3DOverlay>(); thisOverlay = Overlay::Pointer(new Text3DOverlay(), [](Overlay* ptr) { ptr->deleteLater(); });
} else if (type == Shape3DOverlay::TYPE) { } else if (type == Shape3DOverlay::TYPE) {
thisOverlay = std::make_shared<Shape3DOverlay>(); thisOverlay = Overlay::Pointer(new Shape3DOverlay(), [](Overlay* ptr) { ptr->deleteLater(); });
} else if (type == Cube3DOverlay::TYPE) { } else if (type == Cube3DOverlay::TYPE) {
thisOverlay = std::make_shared<Cube3DOverlay>(); thisOverlay = Overlay::Pointer(new Cube3DOverlay(), [](Overlay* ptr) { ptr->deleteLater(); });
} else if (type == Sphere3DOverlay::TYPE) { } else if (type == Sphere3DOverlay::TYPE) {
thisOverlay = std::make_shared<Sphere3DOverlay>(); thisOverlay = Overlay::Pointer(new Sphere3DOverlay(), [](Overlay* ptr) { ptr->deleteLater(); });
} else if (type == Circle3DOverlay::TYPE) { } else if (type == Circle3DOverlay::TYPE) {
thisOverlay = std::make_shared<Circle3DOverlay>(); thisOverlay = Overlay::Pointer(new Circle3DOverlay(), [](Overlay* ptr) { ptr->deleteLater(); });
} else if (type == Rectangle3DOverlay::TYPE) { } else if (type == Rectangle3DOverlay::TYPE) {
thisOverlay = std::make_shared<Rectangle3DOverlay>(); thisOverlay = Overlay::Pointer(new Rectangle3DOverlay(), [](Overlay* ptr) { ptr->deleteLater(); });
} else if (type == Line3DOverlay::TYPE) { } else if (type == Line3DOverlay::TYPE) {
thisOverlay = std::make_shared<Line3DOverlay>(); thisOverlay = Overlay::Pointer(new Line3DOverlay(), [](Overlay* ptr) { ptr->deleteLater(); });
} else if (type == Grid3DOverlay::TYPE) { } else if (type == Grid3DOverlay::TYPE) {
thisOverlay = std::make_shared<Grid3DOverlay>(); thisOverlay = Overlay::Pointer(new Grid3DOverlay(), [](Overlay* ptr) { ptr->deleteLater(); });
} else if (type == ModelOverlay::TYPE) { } else if (type == ModelOverlay::TYPE) {
thisOverlay = std::make_shared<ModelOverlay>(); thisOverlay = Overlay::Pointer(new ModelOverlay(), [](Overlay* ptr) { ptr->deleteLater(); });
} else if (type == Web3DOverlay::TYPE) { } else if (type == Web3DOverlay::TYPE) {
thisOverlay = std::make_shared<Web3DOverlay>(); thisOverlay = Overlay::Pointer(new Web3DOverlay(), [](Overlay* ptr) { ptr->deleteLater(); });
} else if (type == RectangleOverlay::TYPE) { } else if (type == RectangleOverlay::TYPE) {
thisOverlay = std::make_shared<RectangleOverlay>(); thisOverlay = Overlay::Pointer(new RectangleOverlay(), [](Overlay* ptr) { ptr->deleteLater(); });
} }
if (thisOverlay) { if (thisOverlay) {
@ -230,7 +230,7 @@ OverlayID Overlays::cloneOverlay(OverlayID id) {
Overlay::Pointer thisOverlay = getOverlay(id); Overlay::Pointer thisOverlay = getOverlay(id);
if (thisOverlay) { if (thisOverlay) {
OverlayID cloneId = addOverlay(Overlay::Pointer(thisOverlay->createClone())); OverlayID cloneId = addOverlay(Overlay::Pointer(thisOverlay->createClone(), [](Overlay* ptr) { ptr->deleteLater(); }));
#if OVERLAY_PANELS #if OVERLAY_PANELS
auto attachable = std::dynamic_pointer_cast<PanelAttachable>(thisOverlay); auto attachable = std::dynamic_pointer_cast<PanelAttachable>(thisOverlay);
if (attachable && attachable->getParentPanel()) { if (attachable && attachable->getParentPanel()) {

View file

@ -60,7 +60,8 @@ bool ModelEntityWrapper::isModelLoaded() const {
} }
EntityItemPointer RenderableModelEntityItem::factory(const EntityItemID& entityID, const EntityItemProperties& properties) { EntityItemPointer RenderableModelEntityItem::factory(const EntityItemID& entityID, const EntityItemProperties& properties) {
EntityItemPointer entity{ new RenderableModelEntityItem(entityID, properties.getDimensionsInitialized()) }; EntityItemPointer entity(new RenderableModelEntityItem(entityID, properties.getDimensionsInitialized()),
[](EntityItem* ptr) { ptr->deleteLater(); });
entity->setProperties(properties); entity->setProperties(properties);
return entity; return entity;
} }

View file

@ -138,7 +138,7 @@ void loop3(const T& start, const T& end, F f) {
} }
EntityItemPointer RenderablePolyVoxEntityItem::factory(const EntityItemID& entityID, const EntityItemProperties& properties) { EntityItemPointer RenderablePolyVoxEntityItem::factory(const EntityItemID& entityID, const EntityItemProperties& properties) {
std::shared_ptr<RenderablePolyVoxEntityItem> entity{ new RenderablePolyVoxEntityItem(entityID) }; std::shared_ptr<RenderablePolyVoxEntityItem> entity(new RenderablePolyVoxEntityItem(entityID), [](EntityItem* ptr) { ptr->deleteLater(); });
entity->setProperties(properties); entity->setProperties(properties);
entity->initializePolyVox(); entity->initializePolyVox();
return entity; return entity;

View file

@ -30,7 +30,7 @@ const float LightEntityItem::DEFAULT_CUTOFF = PI / 2.0f;
bool LightEntityItem::_lightsArePickable = false; bool LightEntityItem::_lightsArePickable = false;
EntityItemPointer LightEntityItem::factory(const EntityItemID& entityID, const EntityItemProperties& properties) { EntityItemPointer LightEntityItem::factory(const EntityItemID& entityID, const EntityItemProperties& properties) {
EntityItemPointer entity { new LightEntityItem(entityID) }; EntityItemPointer entity(new LightEntityItem(entityID), [](EntityItem* ptr) { ptr->deleteLater(); });
entity->setProperties(properties); entity->setProperties(properties);
return entity; return entity;
} }

View file

@ -26,7 +26,7 @@ const int LineEntityItem::MAX_POINTS_PER_LINE = 70;
EntityItemPointer LineEntityItem::factory(const EntityItemID& entityID, const EntityItemProperties& properties) { EntityItemPointer LineEntityItem::factory(const EntityItemID& entityID, const EntityItemProperties& properties) {
EntityItemPointer entity { new LineEntityItem(entityID) }; EntityItemPointer entity(new LineEntityItem(entityID), [](EntityItem* ptr) { ptr->deleteLater(); });
entity->setProperties(properties); entity->setProperties(properties);
return entity; return entity;
} }
@ -214,4 +214,4 @@ void LineEntityItem::resetPointsChanged() {
withWriteLock([&] { withWriteLock([&] {
_pointsChanged = false; _pointsChanged = false;
}); });
} }

View file

@ -26,7 +26,7 @@ const QString ModelEntityItem::DEFAULT_MODEL_URL = QString("");
const QString ModelEntityItem::DEFAULT_COMPOUND_SHAPE_URL = QString(""); const QString ModelEntityItem::DEFAULT_COMPOUND_SHAPE_URL = QString("");
EntityItemPointer ModelEntityItem::factory(const EntityItemID& entityID, const EntityItemProperties& properties) { EntityItemPointer ModelEntityItem::factory(const EntityItemID& entityID, const EntityItemProperties& properties) {
EntityItemPointer entity { new ModelEntityItem(entityID) }; EntityItemPointer entity(new ModelEntityItem(entityID), [](EntityItem* ptr) { ptr->deleteLater(); });
entity->setProperties(properties); entity->setProperties(properties);
return entity; return entity;
} }

View file

@ -147,7 +147,7 @@ uint64_t Properties::emitIntervalUsecs() const {
EntityItemPointer ParticleEffectEntityItem::factory(const EntityItemID& entityID, const EntityItemProperties& properties) { EntityItemPointer ParticleEffectEntityItem::factory(const EntityItemID& entityID, const EntityItemProperties& properties) {
EntityItemPointer entity { new ParticleEffectEntityItem(entityID) }; EntityItemPointer entity(new ParticleEffectEntityItem(entityID), [](EntityItem* ptr) { ptr->deleteLater(); });
entity->setProperties(properties); entity->setProperties(properties);
return entity; return entity;
} }

View file

@ -26,7 +26,7 @@ const int PolyLineEntityItem::MAX_POINTS_PER_LINE = 70;
EntityItemPointer PolyLineEntityItem::factory(const EntityItemID& entityID, const EntityItemProperties& properties) { EntityItemPointer PolyLineEntityItem::factory(const EntityItemID& entityID, const EntityItemProperties& properties) {
EntityItemPointer entity{ new PolyLineEntityItem(entityID) }; EntityItemPointer entity(new PolyLineEntityItem(entityID), [](EntityItem* ptr) { ptr->deleteLater(); });
entity->setProperties(properties); entity->setProperties(properties);
return entity; return entity;
} }

View file

@ -47,7 +47,7 @@ const QString PolyVoxEntityItem::DEFAULT_Y_TEXTURE_URL = QString("");
const QString PolyVoxEntityItem::DEFAULT_Z_TEXTURE_URL = QString(""); const QString PolyVoxEntityItem::DEFAULT_Z_TEXTURE_URL = QString("");
EntityItemPointer PolyVoxEntityItem::factory(const EntityItemID& entityID, const EntityItemProperties& properties) { EntityItemPointer PolyVoxEntityItem::factory(const EntityItemID& entityID, const EntityItemProperties& properties) {
EntityItemPointer entity { new PolyVoxEntityItem(entityID) }; EntityItemPointer entity(new PolyVoxEntityItem(entityID), [](EntityItem* ptr) { ptr->deleteLater(); });
entity->setProperties(properties); entity->setProperties(properties);
return entity; return entity;
} }

View file

@ -52,7 +52,7 @@ namespace entity {
} }
ShapeEntityItem::Pointer ShapeEntityItem::baseFactory(const EntityItemID& entityID, const EntityItemProperties& properties) { ShapeEntityItem::Pointer ShapeEntityItem::baseFactory(const EntityItemID& entityID, const EntityItemProperties& properties) {
Pointer entity { new ShapeEntityItem(entityID) }; Pointer entity(new ShapeEntityItem(entityID), [](EntityItem* ptr) { ptr->deleteLater(); });
entity->setProperties(properties); entity->setProperties(properties);
return entity; return entity;
} }

View file

@ -30,7 +30,7 @@ const xColor TextEntityItem::DEFAULT_BACKGROUND_COLOR = { 0, 0, 0};
const bool TextEntityItem::DEFAULT_FACE_CAMERA = false; const bool TextEntityItem::DEFAULT_FACE_CAMERA = false;
EntityItemPointer TextEntityItem::factory(const EntityItemID& entityID, const EntityItemProperties& properties) { EntityItemPointer TextEntityItem::factory(const EntityItemID& entityID, const EntityItemProperties& properties) {
EntityItemPointer entity { new TextEntityItem(entityID) }; EntityItemPointer entity(new TextEntityItem(entityID), [](EntityItem* ptr) { ptr->deleteLater(); });
entity->setProperties(properties); entity->setProperties(properties);
return entity; return entity;
} }

View file

@ -24,7 +24,7 @@
const QString WebEntityItem::DEFAULT_SOURCE_URL("http://www.google.com"); const QString WebEntityItem::DEFAULT_SOURCE_URL("http://www.google.com");
EntityItemPointer WebEntityItem::factory(const EntityItemID& entityID, const EntityItemProperties& properties) { EntityItemPointer WebEntityItem::factory(const EntityItemID& entityID, const EntityItemProperties& properties) {
EntityItemPointer entity { new WebEntityItem(entityID) }; EntityItemPointer entity(new WebEntityItem(entityID), [](EntityItem* ptr) { ptr->deleteLater(); });
entity->setProperties(properties); entity->setProperties(properties);
return entity; return entity;
} }

View file

@ -32,7 +32,7 @@ const bool ZoneEntityItem::DEFAULT_GHOSTING_ALLOWED = true;
const QString ZoneEntityItem::DEFAULT_FILTER_URL = ""; const QString ZoneEntityItem::DEFAULT_FILTER_URL = "";
EntityItemPointer ZoneEntityItem::factory(const EntityItemID& entityID, const EntityItemProperties& properties) { EntityItemPointer ZoneEntityItem::factory(const EntityItemID& entityID, const EntityItemProperties& properties) {
EntityItemPointer entity { new ZoneEntityItem(entityID) }; EntityItemPointer entity(new ZoneEntityItem(entityID), [](EntityItem* ptr) { ptr->deleteLater(); });
entity->setProperties(properties); entity->setProperties(properties);
return entity; return entity;
} }