Change reset to disconnect and clarify comments

I decided that it's actually fine to copy a direct object to another QPDF. Even if we eventually prevent a QPDFObject from having multiple parents, this could happen if an object is moved.

Change reset to disconnect and clarify comments
I decided that it's actually fine to copy a direct object to another QPDF. Even if we eventually prevent a QPDFObject from having multiple parents, this could happen if an object is moved.
Jay Berkenbilt
1 parent dba61da1
Showing 17 changed files with 44 additions and 64 deletions
TODO
include/qpdf/QPDFObjectHandle.hh
libqpdf/QPDF.cc
libqpdf/QPDFObjectHandle.cc
libqpdf/QPDFValueProxy.cc
libqpdf/QPDFWriter.cc
libqpdf/QPDF_Array.cc
libqpdf/QPDF_Dictionary.cc
libqpdf/QPDF_Stream.cc
libqpdf/SparseOHArray.cc
libqpdf/qpdf/QPDFValue.hh
libqpdf/qpdf/QPDFValueProxy.hh
libqpdf/qpdf/QPDF_Array.hh
libqpdf/qpdf/QPDF_Dictionary.hh
libqpdf/qpdf/QPDF_Stream.hh
libqpdf/qpdf/SparseOHArray.hh
qpdf/test_driver.cc
@@ -785,7 +785,7 @@ Rejected Ideas
   and too much toil for library users to be worth the small benefit of
   not having to call resetObjGen in QPDF's destructor.
-* Fix Multiple Direct Object Owner Issue
+* Fix Multiple Direct Object Parent Issue
   These are some ideas I had before m-holger's changes to split
   QPDFValue from QPDFObject. These notes were written prior to the
@@ -811,12 +811,3 @@ Rejected Ideas
   Note that arrays and dictionaries still need to contain
   QPDFObjectHandle because of indirect objects. This only pertains to
   direct objects, which are always "resolved" in QPDFObjectHandle.
-
-  If this is addressed, read comments in the following places:
-  * QPDFWriter.cc::enqueueObject near the call to getOwningQPDF
-  * QPDFValueProxy::reset and QPDFValueProxy::destroy
-  * QPDF::~QPDF()
-  * test 92 in test_driver.cc
-  * QPDFObjectHandle.hh near isDestroyed
-  All these references were from the release of qpdf 11 (in case they
-  have moved by such time as this might be resurrected).
@@ -392,7 +392,8 @@ class QPDFObjectHandle
     inline bool isIndirect() const;
     // This returns true for indirect objects from a QPDF that has
-    // been destroyed.
+    // been destroyed. Trying unparse such an object will throw a
+    // logic_error.
     QPDF_DLL
     bool isDestroyed();
@@ -1540,8 +1541,8 @@ class QPDFObjectHandle
     friend class ObjAccessor;
     // Provide access to specific classes for recursive
-    // reset().
-    class Resetter
+    // disconnected().
+    class DisconnectAccess
     {
         friend class QPDF_Dictionary;
         friend class QPDF_Stream;
@@ -1549,9 +1550,9 @@ class QPDFObjectHandle
       private:
         static void
-        reset(QPDFObjectHandle& o)
+        disconnect(QPDFObjectHandle& o)
         {
-            o.reset();
+            o.disconnect();
         }
     };
     friend class Resetter;
@@ -1653,7 +1654,7 @@ class QPDFObjectHandle
         bool first_level_only,
         bool stop_at_streams);
     void shallowCopyInternal(QPDFObjectHandle& oh, bool first_level_only);
-    void reset();
+    void disconnect();
     void setParsedOffset(qpdf_offset_t offset);
     void parseContentStream_internal(
         std::string const& description, ParserCallbacks* callbacks);
@@ -252,9 +252,11 @@ QPDF::~QPDF()
     // resolved indirect references by replacing them with an internal
     // object type representing that they have been destroyed. Note
     // that we can't break references like this at any time when the
-    // QPDF object is active. The call to reset also causes all
+    // QPDF object is active. The call to reset also causes all direct
     // QPDFObjectHandle objects that are reachable from this object to
-    // release their association with this QPDF.
+    // release their association with this QPDF. Direct objects are
+    // not destroyed since they can be moved to other QPDF objects
+    // safely.
     // At this point, obviously no one is still using the QPDF object,
     // but we'll explicitly clear the xref table anyway just to
@@ -262,9 +264,7 @@ QPDF::~QPDF()
     this->m->xref_table.clear();
     auto null_obj = QPDF_Null::create();
     for (auto const& iter: this->m->obj_cache) {
-        iter.second.object->reset();
-        // It would be better if reset() could call destroy(), but it
-        // can't -- see comments in QPDFValueProxy::reset().
+        iter.second.object->disconnect();
         iter.second.object->destroy();
     }
 }
@@ -249,7 +249,7 @@ QPDFObjectHandle::operator!=(QPDFObjectHandle const&amp; rhs) const
 }
 void
-QPDFObjectHandle::reset()
+QPDFObjectHandle::disconnect()
 {
     // Recursively remove association with any QPDF object. This
     // method may only be called during final destruction.
@@ -257,7 +257,7 @@ QPDFObjectHandle::reset()
     // pointer itself, so we don't do that here. Other objects call it
     // through this method.
     if (!isIndirect()) {
-        this->obj->reset();
+        this->obj->disconnect();
     }
 }
@@ -13,6 +13,5 @@ QPDFValueProxy::doResolve()
 void
 QPDFValueProxy::destroy()
 {
-    // See comments in reset() for why this isn't part of reset.
     value = QPDF_Destroyed::getInstance();
 }
@@ -1198,14 +1198,12 @@ void
 QPDFWriter::enqueueObject(QPDFObjectHandle object)
 {
     if (object.isIndirect()) {
-        // This owner check should really be done for all objects, not
-        // just indirect objects. As of the time of the release of
-        // qpdf 11, it is known that there are cases of direct objects
-        // from other files getting copied into multiple QPDF objects.
-        // This definitely happens in the page splitting code. If we
-        // were to implement strong checks to prevent objects from
-        // having multiple owners, once that was complete phased in,
-        // this check could be moved outside the if statement.
+        // This owner check can only be done for indirect objects. It
+        // is possible for a direct object to have an owning QPDF that
+        // is from another file if a direct QPDFObjectHandle from one
+        // file was insert into another file without copying. Doing
+        // that is safe even if the original QPDF gets destroyed,
+        // which just disconnects the QPDFObjectHandle from its owner.
         if (object.getOwningQPDF() != &(this->m->pdf)) {
             QTC::TC("qpdf", "QPDFWriter foreign object");
             throw std::logic_error(
@@ -35,9 +35,9 @@ QPDF_Array::shallowCopy()
 }
 void
-QPDF_Array::reset()
+QPDF_Array::disconnect()
 {
-    elements.reset();
+    elements.disconnect();
 }
 std::string
@@ -22,10 +22,10 @@ QPDF_Dictionary::shallowCopy()
 }
 void
-QPDF_Dictionary::reset()
+QPDF_Dictionary::disconnect()
 {
     for (auto& iter: this->items) {
-        QPDFObjectHandle::Resetter::reset(iter.second);
+        QPDFObjectHandle::DisconnectAccess::disconnect(iter.second);
     }
 }
@@ -168,10 +168,10 @@ QPDF_Stream::getFilterOnWrite() const
 }
 void
-QPDF_Stream::reset()
+QPDF_Stream::disconnect()
 {
     this->stream_provider = nullptr;
-    QPDFObjectHandle::Resetter::reset(this->stream_dict);
+    QPDFObjectHandle::DisconnectAccess::disconnect(this->stream_dict);
 }
 void
@@ -49,10 +49,10 @@ SparseOHArray::remove_last()
 }
 void
-SparseOHArray::reset()
+SparseOHArray::disconnect()
 {
     for (auto& iter: this->elements) {
-        QPDFObjectHandle::Resetter::reset(iter.second);
+        QPDFObjectHandle::DisconnectAccess::disconnect(iter.second);
     }
 }
@@ -64,7 +64,7 @@ class QPDFValue
         return og;
     }
     virtual void
-    reset()
+    disconnect()
     {
     }
@@ -102,33 +102,24 @@ class QPDFValueProxy
         o->value->og = og;
     }
-    // The following two methods are for use by class QPDF only
     void
     setObjGen(QPDF* qpdf, QPDFObjGen const& og)
     {
+        // Intended for use by the QPDF class
         value->qpdf = qpdf;
         value->og = og;
     }
     void
-    reset()
-    {
-        value->reset();
-        // It would be better if, rather than clearing value->qpdf and
-        // value->og, we completely replaced value with
-        // QPDF_Destroyed. However, at the time of the release of qpdf
-        // 11, this causes test failures and would likely break a lot
-        // of code since it possible for a direct object that
-        // recursively contains no indirect objects to be copied into
-        // multiple QPDF objects. For that reason, we have to break
-        // the association with the owning QPDF but not otherwise
-        // mutate the object. For indirect objects, QPDF::~QPDF
-        // replaces indirect objects with QPDF_Destroyed, which clears
-        // circular references. If this code were able to do that,
-        // that code would not have to.
+    disconnect()
+    {
+        // Disconnect an object from its owning QPDF. This is called
+        // by QPDF's destructor.
+        value->disconnect();
         value->qpdf = nullptr;
         value->og = QPDFObjGen();
     }
-
+    // Mark an object as destroyed. Used by QPDF's destructor for its
+    // indirect objects.
     void destroy();
     bool
@@ -17,7 +17,7 @@ class QPDF_Array: public QPDFValue
     virtual std::shared_ptr<QPDFValueProxy> shallowCopy();
     virtual std::string unparse();
     virtual JSON getJSON(int json_version);
-    virtual void reset();
+    virtual void disconnect();
     int getNItems() const;
     QPDFObjectHandle getItem(int n) const;
@@ -17,7 +17,7 @@ class QPDF_Dictionary: public QPDFValue
     virtual std::shared_ptr<QPDFValueProxy> shallowCopy();
     virtual std::string unparse();
     virtual JSON getJSON(int json_version);
-    virtual void reset();
+    virtual void disconnect();
     // hasKey() and getKeys() treat keys with null values as if they
     // aren't there.  getKey() returns null for the value of a
@@ -27,7 +27,7 @@ class QPDF_Stream: public QPDFValue
     virtual std::string unparse();
     virtual JSON getJSON(int json_version);
     virtual void setDescription(QPDF*, std::string const&);
-    virtual void reset();
+    virtual void disconnect();
     QPDFObjectHandle getDict() const;
     bool isDataModified() const;
     void setFilterOnWrite(bool);
@@ -15,7 +15,7 @@ class SparseOHArray
     void setAt(size_t idx, QPDFObjectHandle oh);
     void erase(size_t idx);
     void insert(size_t idx, QPDFObjectHandle oh);
-    void reset();
+    void disconnect();
     typedef std::unordered_map<size_t, QPDFObjectHandle>::const_iterator
         const_iterator;
@@ -3316,8 +3316,8 @@ test_92(QPDF&amp; pdf, char const* arg2)
     check(contents);
     check(contents_dict);
     // Objects that were originally indirect should be destroyed.
-    // Otherwise, they should have retained their old values. See
-    // comments in QPDFValueProxy::reset for why this is the case.
+    // Otherwise, they should have retained their old values but just
+    // lost their connection to the owning QPDF.
     assert(root.isDestroyed());
     assert(page1.isDestroyed());
     assert(contents.isDestroyed());