Skip to content

Commit

Permalink
Fix crash in rendering on proj 6 builds
Browse files Browse the repository at this point in the history 
Ensure PJ* objects are never reused across threads.

Fixes #33902
  • Loading branch information
@nyalldawson
nyalldawson committed Jan 22, 2020
1 parent db32236 commit 8a71091
Show file tree
Hide file tree
Showing 4 changed files with 186 additions and 18 deletions.
131 changes: 117 additions & 14 deletions src/core/qgscoordinatereferencesystem.cpp
View file
Expand Up @@ -692,12 +692,113 @@ bool QgsCoordinateReferenceSystem::loadFromDatabase( const QString &db, const QS
return d->mIsValid;
}

#if PROJ_VERSION_MAJOR>=6
void QgsCoordinateReferenceSystem::removeFromCacheObjectsBelongingToCurrentThread( void *pj_context )
{
// Not completely sure about object order destruction after main() has
// exited. So it is safer to check sDisableCache before using sCacheLock
// in case sCacheLock would have been destroyed before the current TLS
// QgsProjContext object that has called us...

if ( !sDisableSrIdCache )
{
QgsReadWriteLocker locker( *sSrIdCacheLock(), QgsReadWriteLocker::Write );
if ( !sDisableSrIdCache )
{
for ( auto it = sSrIdCache()->begin(); it != sSrIdCache()->end(); )
{
auto &v = it.value();
if ( v.d->removeObjectsBelongingToCurrentThread( pj_context ) )
it = sSrIdCache()->erase( it );
else
++it;
}
}
}
if ( !sDisableOgcCache )
{
QgsReadWriteLocker locker( *sOgcLock(), QgsReadWriteLocker::Write );
if ( !sDisableOgcCache )
{
for ( auto it = sOgcCache()->begin(); it != sOgcCache()->end(); )
{
auto &v = it.value();
if ( v.d->removeObjectsBelongingToCurrentThread( pj_context ) )
it = sOgcCache()->erase( it );
else
++it;
}
}
}
if ( !sDisableProjCache )
{
QgsReadWriteLocker locker( *sProj4CacheLock(), QgsReadWriteLocker::Write );
if ( !sDisableProjCache )
{
for ( auto it = sProj4Cache()->begin(); it != sProj4Cache()->end(); )
{
auto &v = it.value();
if ( v.d->removeObjectsBelongingToCurrentThread( pj_context ) )
it = sProj4Cache()->erase( it );
else
++it;
}
}
}
if ( !sDisableWktCache )
{
QgsReadWriteLocker locker( *sCRSWktLock(), QgsReadWriteLocker::Write );
if ( !sDisableWktCache )
{
for ( auto it = sWktCache()->begin(); it != sWktCache()->end(); )
{
auto &v = it.value();
if ( v.d->removeObjectsBelongingToCurrentThread( pj_context ) )
it = sWktCache()->erase( it );
else
++it;
}
}
}
if ( !sDisableSrsIdCache )
{
QgsReadWriteLocker locker( *sCRSSrsIdLock(), QgsReadWriteLocker::Write );
if ( !sDisableSrsIdCache )
{
for ( auto it = sSrsIdCache()->begin(); it != sSrsIdCache()->end(); )
{
auto &v = it.value();
if ( v.d->removeObjectsBelongingToCurrentThread( pj_context ) )
it = sSrsIdCache()->erase( it );
else
++it;
}
}
}
if ( !sDisableStringCache )
{
QgsReadWriteLocker locker( *sCrsStringLock(), QgsReadWriteLocker::Write );
if ( !sDisableStringCache )
{
for ( auto it = sStringCache()->begin(); it != sStringCache()->end(); )
{
auto &v = it.value();
if ( v.d->removeObjectsBelongingToCurrentThread( pj_context ) )
it = sStringCache()->erase( it );
else
++it;
}
}
}
}
#endif

bool QgsCoordinateReferenceSystem::hasAxisInverted() const
{
if ( d->mAxisInvertedDirty )
{
#if PROJ_VERSION_MAJOR>=6
d->mAxisInverted = QgsProjUtils::axisOrderIsSwapped( d->mPj.get() );
d->mAxisInverted = QgsProjUtils::axisOrderIsSwapped( d->threadLocalProjObject() );
#else
OGRAxisOrientation orientation;
OSRGetAxis( d->mCRS, OSRIsGeographic( d->mCRS ) ? "GEOGCS" : "PROJCS", 0, &orientation );
Expand Down Expand Up @@ -1216,9 +1317,9 @@ QString QgsCoordinateReferenceSystem::ellipsoidAcronym() const
if ( d->mEllipsoidAcronym.isNull() )
{
#if PROJ_VERSION_MAJOR>=6
if ( d->mPj )
if ( PJ *obj = d->threadLocalProjObject() )
{
QgsProjUtils::proj_pj_unique_ptr ellipsoid( proj_get_ellipsoid( QgsProjContext::get(), d->mPj.get() ) );
QgsProjUtils::proj_pj_unique_ptr ellipsoid( proj_get_ellipsoid( QgsProjContext::get(), obj ) );
if ( ellipsoid )
{
const QString ellipsoidAuthName( proj_get_id_auth_name( ellipsoid.get(), 0 ) );
Expand Down Expand Up @@ -1266,9 +1367,9 @@ QString QgsCoordinateReferenceSystem::toProj() const
if ( d->mProj4.isEmpty() )
{
#if PROJ_VERSION_MAJOR>=6
if ( d->mPj )
if ( PJ *obj = d->threadLocalProjObject() )
{
d->mProj4 = getFullProjString( d->mPj.get() );
d->mProj4 = getFullProjString( obj );
}
#else
char *proj4src = nullptr;
Expand Down Expand Up @@ -1300,15 +1401,16 @@ QgsRectangle QgsCoordinateReferenceSystem::bounds() const
return QgsRectangle();

#if PROJ_VERSION_MAJOR>=6
if ( !d->mPj )
PJ *obj = d->threadLocalProjObject();
if ( !obj )
return QgsRectangle();

double westLon = 0;
double southLat = 0;
double eastLon = 0;
double northLat = 0;

if ( !proj_get_area_of_use( QgsProjContext::get(), d->mPj.get(),
if ( !proj_get_area_of_use( QgsProjContext::get(), obj,
&westLon, &southLat, &eastLon, &northLat, nullptr ) )
return QgsRectangle();

Expand Down Expand Up @@ -1822,7 +1924,7 @@ bool QgsCoordinateReferenceSystem::operator!=( const QgsCoordinateReferenceSyste
QString QgsCoordinateReferenceSystem::toWkt( WktVariant variant, bool multiline, int indentationWidth ) const
{
#if PROJ_VERSION_MAJOR>=6
if ( d->mPj )
if ( PJ *obj = d->threadLocalProjObject() )
{
PJ_WKT_TYPE type = PJ_WKT1_GDAL;
switch ( variant )
Expand Down Expand Up @@ -1850,7 +1952,7 @@ QString QgsCoordinateReferenceSystem::toWkt( WktVariant variant, bool multiline,
const QByteArray multiLineOption = QStringLiteral( "MULTILINE=%1" ).arg( multiline ? QStringLiteral( "YES" ) : QStringLiteral( "NO" ) ).toLocal8Bit();
const QByteArray indentatationWidthOption = QStringLiteral( "INDENTATION_WIDTH=%1" ).arg( multiline ? QString::number( indentationWidth ) : QStringLiteral( "0" ) ).toLocal8Bit();
const char *const options[] = {multiLineOption.constData(), indentatationWidthOption.constData(), nullptr};
return QString( proj_as_wkt( QgsProjContext::get(), d->mPj.get(), type, options ) );
return QString( proj_as_wkt( QgsProjContext::get(), obj, type, options ) );
}
return QString();
#else
Expand Down Expand Up @@ -2424,14 +2526,15 @@ QList<long> QgsCoordinateReferenceSystem::userSrsIds()

long QgsCoordinateReferenceSystem::matchToUserCrs() const
{
if ( !d->mPj )
PJ *obj = d->threadLocalProjObject();
if ( !obj )
return 0;

const QList< long > ids = userSrsIds();
for ( long id : ids )
{
QgsCoordinateReferenceSystem candidate = QgsCoordinateReferenceSystem::fromSrsId( id );
if ( candidate.projObject() && proj_is_equivalent_to( d->mPj.get(), candidate.projObject(), PJ_COMP_EQUIVALENT ) )
if ( candidate.projObject() && proj_is_equivalent_to( obj, candidate.projObject(), PJ_COMP_EQUIVALENT ) )
{
return id;
}
Expand Down Expand Up @@ -3341,9 +3444,9 @@ QString QgsCoordinateReferenceSystem::geographicCrsAuthId() const
return d->mAuthId;
}
#if PROJ_VERSION_MAJOR>=6
else if ( d->mPj )
else if ( PJ *obj = d->threadLocalProjObject() )
{
QgsProjUtils::proj_pj_unique_ptr geoCrs( proj_crs_get_geodetic_crs( QgsProjContext::get(), d->mPj.get() ) );
QgsProjUtils::proj_pj_unique_ptr geoCrs( proj_crs_get_geodetic_crs( QgsProjContext::get(), obj ) );
return geoCrs ? QStringLiteral( "%1:%2" ).arg( proj_get_id_auth_name( geoCrs.get(), 0 ), proj_get_id_code( geoCrs.get(), 0 ) ) : QString();
}
#else
Expand All @@ -3361,7 +3464,7 @@ QString QgsCoordinateReferenceSystem::geographicCrsAuthId() const
#if PROJ_VERSION_MAJOR>=6
PJ *QgsCoordinateReferenceSystem::projObject() const
{
return d->mPj.get();
return d->threadLocalProjObject();
}
#endif

Expand Down
8 changes: 7 additions & 1 deletion src/core/qgscoordinatereferencesystem.h
View file
Expand Up @@ -956,10 +956,16 @@ class CORE_EXPORT QgsCoordinateReferenceSystem

QExplicitlySharedDataPointer<QgsCoordinateReferenceSystemPrivate> d;

#if PROJ_VERSION_MAJOR>=6
friend class QgsProjContext;

// Only meant to be called by QgsProjContext::~QgsProjContext()
static void removeFromCacheObjectsBelongingToCurrentThread( void *pj_context );
#endif

//! Function for CRS validation. May be NULLPTR.
static CUSTOM_CRS_VALIDATION sCustomSrsValidation;


// cache

static bool sDisableSrIdCache;
Expand Down
64 changes: 61 additions & 3 deletions src/core/qgscoordinatereferencesystem_p.h
View file
Expand Up @@ -34,6 +34,7 @@
#if PROJ_VERSION_MAJOR>=6
#include <proj.h>
#include "qgsprojutils.h"
#include "qgsreadwritelocker.h"
#else
#include <ogr_srs_api.h>
#endif
Expand Down Expand Up @@ -75,8 +76,11 @@ class QgsCoordinateReferenceSystemPrivate : public QSharedData
, mAxisInverted( other.mAxisInverted )
{
#if PROJ_VERSION_MAJOR>=6
if ( mIsValid && other.mPj )
mPj.reset( proj_clone( QgsProjContext::get(), other.mPj.get() ) );
if ( mIsValid )
{
if ( PJ *obj = other.threadLocalProjObject() )
mPj.reset( proj_clone( QgsProjContext::get(), obj ) );
}
#else
if ( mIsValid )
{
Expand All @@ -91,7 +95,15 @@ class QgsCoordinateReferenceSystemPrivate : public QSharedData

~QgsCoordinateReferenceSystemPrivate()
{
#if PROJ_VERSION_MAJOR<6
#if PROJ_VERSION_MAJOR>=6
QgsReadWriteLocker locker( mProjLock, QgsReadWriteLocker::Write );

for ( auto it = mProjObjects.begin(); it != mProjObjects.end(); ++it )
{
proj_destroy( it.value() );
}
mProjObjects.clear();
#else
OSRDestroySpatialReference( mCRS );
#endif
}
Expand Down Expand Up @@ -124,6 +136,9 @@ class QgsCoordinateReferenceSystemPrivate : public QSharedData
bool mIsValid = false;

#if PROJ_VERSION_MAJOR>=6

// this is the "master" proj object, to be used as a template for new proj objects created on different threads ONLY.
// Always use threadLocalProjObject() instead of this.
QgsProjUtils::proj_pj_unique_ptr mPj;
#else
OGRSpatialReferenceH mCRS;
Expand All @@ -138,6 +153,49 @@ class QgsCoordinateReferenceSystemPrivate : public QSharedData
//! Whether this is a coordinate system has inverted axis
mutable bool mAxisInverted = false;

#if PROJ_VERSION_MAJOR>=6
mutable QReadWriteLock mProjLock;
mutable QMap < uintptr_t, PJ * > mProjObjects;

PJ *threadLocalProjObject() const
{
QgsReadWriteLocker locker( mProjLock, QgsReadWriteLocker::Read );
if ( !mPj )
return nullptr;

PJ_CONTEXT *context = QgsProjContext::get();
QMap < uintptr_t, PJ * >::const_iterator it = mProjObjects.constFind( reinterpret_cast< uintptr_t>( context ) );

if ( it != mProjObjects.constEnd() )
{
return it.value();
}

// proj object doesn't exist yet, so we need to create
locker.changeMode( QgsReadWriteLocker::Write );

PJ *res = proj_clone( context, mPj.get() );
mProjObjects.insert( reinterpret_cast< uintptr_t>( context ), res );
return res;
}

// Only meant to be called by QgsCoordinateReferenceSystem::removeFromCacheObjectsBelongingToCurrentThread()
bool removeObjectsBelongingToCurrentThread( void *pj_context )
{
QgsReadWriteLocker locker( mProjLock, QgsReadWriteLocker::Write );

QMap < uintptr_t, PJ * >::iterator it = mProjObjects.find( reinterpret_cast< uintptr_t>( pj_context ) );
if ( it != mProjObjects.end() )
{
proj_destroy( it.value() );
mProjObjects.erase( it );
}

return mProjObjects.isEmpty();
}
#endif


};

/// @endcond
Expand Down
1 change: 1 addition & 0 deletions src/core/qgsprojutils.cpp
View file
Expand Up @@ -50,6 +50,7 @@ QgsProjContext::~QgsProjContext()
// Call removeFromCacheObjectsBelongingToCurrentThread() before
// destroying the context
QgsCoordinateTransform::removeFromCacheObjectsBelongingToCurrentThread( mContext );
QgsCoordinateReferenceSystem::removeFromCacheObjectsBelongingToCurrentThread( mContext );
proj_context_destroy( mContext );
#else
pj_ctx_free( mContext );
Expand Down

0 comments on commit 8a71091

Please sign in to comment.