Fix dangling raw pointer bug in BaseTable/SourceCatalog

raw pointer->weak pointer and make sure to initialize it when the table
is copied, particularly in SourceCatalog, which is what uses the
AliasMap.
Move BaseTable constructor out of header, now that it's more
complicated.

include/lsst/afw/table/AliasMap.h

@@ -38,14 +38,14 @@ class AliasMap final {
 
 public:
     // Create an empty AliasMap
-    AliasMap() : _internal(), _table(nullptr) {}
+    AliasMap() : _internal(), _table() {}
 
     /**
      *  Deep-copy an AliasMap
      *
      *  The new AliasMap will not be linked to any tables, even if other is.
      */
-    AliasMap(AliasMap const& other) : _internal(other._internal), _table(nullptr) {}
+    AliasMap(AliasMap const& other) : _internal(other._internal), _table() {}
     // Delegate to copy-constructor for backwards compatibility
     AliasMap(AliasMap&& other) : AliasMap(other) {}
 
@@ -124,7 +124,6 @@ class AliasMap final {
 private:
     friend class Schema;
     friend class SubSchema;
-    friend class BaseTable;
 
     // Internal in-place implementation of apply()
     void _apply(std::string& name) const;
@@ -134,7 +133,7 @@ class AliasMap final {
     // Table to notify of any changes.  We can't use a shared_ptr here because the Table needs to set
     // this in its own constructor, but the Table does guarantee that this pointer is either valid or
     // null.
-    BaseTable* _table;
+    std::weak_ptr<BaseTable> _table;
 };
 }  // namespace table
 }  // namespace afw

include/lsst/afw/table/BaseTable.h

@@ -215,10 +215,7 @@ class BaseTable : public std::enable_shared_from_this<BaseTable> {
     /// Construct from a schema.
     explicit BaseTable(Schema const& schema, std::shared_ptr<daf::base::PropertyList> metadata = nullptr);
 
-    /// Copy construct.
-    BaseTable(BaseTable const& other) : _schema(other._schema), _metadata(other._metadata) {
-        if (_metadata) _metadata = std::static_pointer_cast<daf::base::PropertyList>(_metadata->deepCopy());
-    }
+    BaseTable(BaseTable const& other);
     // Delegate to copy-constructor for backwards compatibility
     BaseTable(BaseTable&& other) : BaseTable(other) {}
 

src/table/AliasMap.cc

@@ -81,15 +81,17 @@ std::string AliasMap::get(std::string const& name) const {
 
 void AliasMap::set(std::string const& alias, std::string const& target) {
     _internal[alias] = target;
-    if (_table) {
-        _table->handleAliasChange(alias);
+    auto table = _table.lock();
+    if (table) {
+        table->handleAliasChange(alias);
     }
 }
 
 bool AliasMap::erase(std::string const& alias) {
     bool result = _internal.erase(alias);
-    if (_table) {
-        _table->handleAliasChange(alias);
+    auto table = _table.lock();
+    if (table) {
+        table->handleAliasChange(alias);
     }
     return result;
 }

src/table/BaseTable.cc

@@ -118,7 +118,9 @@ std::size_t BaseTable::getBufferSize() const {
 }
 
 std::shared_ptr<BaseTable> BaseTable::make(Schema const &schema) {
-    return std::shared_ptr<BaseTable>(new BaseTable(schema));
+    std::shared_ptr<BaseTable> table(new BaseTable(schema));
+    table->getSchema().getAliasMap()->setTable(table);
+    return table;
 }
 
 Schema BaseTable::makeMinimalSchema() { return Schema(); }
@@ -140,7 +142,9 @@ std::shared_ptr<io::FitsWriter> BaseTable::makeFitsWriter(fits::Fits *fitsfile,
 }
 
 std::shared_ptr<BaseTable> BaseTable::_clone() const {
-    return std::shared_ptr<BaseTable>(new BaseTable(*this));
+    std::shared_ptr<BaseTable> table(new BaseTable(*this));
+    table->getSchema().getAliasMap()->setTable(table);
+    return table;
 }
 
 std::shared_ptr<BaseRecord> BaseTable::_makeRecord() { return constructRecord<BaseRecord>(); }
@@ -149,10 +153,16 @@ BaseTable::BaseTable(Schema const &schema, std::shared_ptr<daf::base::PropertyLi
         : _schema(schema), _metadata(metadata) {
     Block::padSchema(_schema);
     _schema.disconnectAliases();
-    _schema.getAliasMap()->_table = this;
+    _schema.getAliasMap()->getTable().reset();
 }
 
-BaseTable::~BaseTable() { _schema.getAliasMap()->_table = nullptr; }
+BaseTable::BaseTable(BaseTable const &other) : _schema(other._schema), _metadata(other._metadata) {
+    _schema.disconnectAliases();
+    _schema.getAliasMap()->getTable().reset();
+    if (_metadata) _metadata = std::static_pointer_cast<daf::base::PropertyList>(_metadata->deepCopy());
+}
+
+BaseTable::~BaseTable() { _schema.getAliasMap()->getTable().reset(); }
 
 namespace {
 

src/table/Exposure.cc

@@ -397,7 +397,9 @@ std::shared_ptr<ExposureTable> ExposureTable::make(Schema const &schema) {
                 lsst::pex::exceptions::InvalidParameterError,
                 "Schema for Exposure must contain at least the keys defined by makeMinimalSchema().");
     }
-    return std::shared_ptr<ExposureTable>(new ExposureTable(schema));
+    std::shared_ptr<ExposureTable> table(new ExposureTable(schema));
+    table->getSchema().getAliasMap()->setTable(table);
+    return table;
 }
 
 ExposureTable::ExposureTable(Schema const &schema) : BaseTable(schema) {}
@@ -429,7 +431,9 @@ std::shared_ptr<io::FitsWriter> ExposureTable::makeFitsWriter(fits::Fits *fitsfi
 }
 
 std::shared_ptr<BaseTable> ExposureTable::_clone() const {
-    return std::shared_ptr<ExposureTable>(new ExposureTable(*this));
+    std::shared_ptr<ExposureTable> table(new ExposureTable(*this));
+    table->getSchema().getAliasMap()->setTable(table);
+    return table;
 }
 
 std::shared_ptr<BaseRecord> ExposureTable::_makeRecord() {

src/table/Simple.cc

@@ -78,7 +78,9 @@ std::shared_ptr<SimpleTable> SimpleTable::make(Schema const& schema,
         throw LSST_EXCEPT(lsst::pex::exceptions::InvalidParameterError,
                           "Schema for Simple must contain at least the keys defined by makeMinimalSchema().");
     }
-    return std::shared_ptr<SimpleTable>(new SimpleTable(schema, idFactory));
+    std::shared_ptr<SimpleTable> table(new SimpleTable(schema, idFactory));
+    table->getSchema().getAliasMap()->setTable(table);
+    return table;
 }
 
 SimpleTable::SimpleTable(Schema const& schema, std::shared_ptr<IdFactory> const& idFactory)
@@ -106,7 +108,9 @@ std::shared_ptr<io::FitsWriter> SimpleTable::makeFitsWriter(fits::Fits* fitsfile
 }
 
 std::shared_ptr<BaseTable> SimpleTable::_clone() const {
-    return std::shared_ptr<SimpleTable>(new SimpleTable(*this));
+    std::shared_ptr<SimpleTable> table(new SimpleTable(*this));
+    table->getSchema().getAliasMap()->setTable(table);
+    return table;
 }
 
 std::shared_ptr<BaseRecord> SimpleTable::_makeRecord() {

src/table/Source.cc

@@ -403,7 +403,9 @@ std::shared_ptr<SourceTable> SourceTable::make(Schema const &schema,
         throw LSST_EXCEPT(lsst::pex::exceptions::InvalidParameterError,
                           "Schema for Source must contain at least the keys defined by getMinimalSchema().");
     }
-    return std::shared_ptr<SourceTable>(new SourceTable(schema, idFactory));
+    std::shared_ptr<SourceTable> table(new SourceTable(schema, idFactory));
+    table->getSchema().getAliasMap()->setTable(table);
+    return table;
 }
 
 SourceTable::SourceTable(Schema const &schema, std::shared_ptr<IdFactory> const &idFactory)
@@ -435,7 +437,9 @@ std::shared_ptr<io::FitsWriter> SourceTable::makeFitsWriter(fits::Fits *fitsfile
 }
 
 std::shared_ptr<BaseTable> SourceTable::_clone() const {
-    return std::shared_ptr<SourceTable>(new SourceTable(*this));
+    std::shared_ptr<SourceTable> table(new SourceTable(*this));
+    table->getSchema().getAliasMap()->setTable(table);
+    return table;
 }
 
 std::shared_ptr<BaseRecord> SourceTable::_makeRecord() {

tests/test_tableAliases.cc

@@ -31,7 +31,9 @@ class TestTable : public lsst::afw::table::BaseTable {
     mutable std::string lastAliasChanged;
 
     static std::shared_ptr<TestTable> make(lsst::afw::table::Schema const& schema) {
-        return std::make_shared<TestTable>(schema);
+        auto table = std::make_shared<TestTable>(schema);
+        table->getSchema().getAliasMap()->setTable(table);
+        return table;
     }
 
     explicit TestTable(lsst::afw::table::Schema const& schema) : lsst::afw::table::BaseTable(schema) {}
@@ -44,7 +46,9 @@ class TestTable : public lsst::afw::table::BaseTable {
     void handleAliasChange(std::string const& alias) override { lastAliasChanged = alias; }
 
     std::shared_ptr<lsst::afw::table::BaseTable> _clone() const override {
-        return std::make_shared<TestTable>(*this);
+        auto table = std::make_shared<TestTable>(*this);
+        table->getSchema().getAliasMap()->setTable(table);
+        return table;
     }
 
     std::shared_ptr<lsst::afw::table::BaseRecord> _makeRecord() override {