Hi all,
I have attached a revised form of my proposed change to this
e-mail as a patch. It addresses the issues raised (below), no
new warnings are caused by it (with GCC 5.3.1), with one exception:
I haven't changed the check, but documented it can't be deleted at
its source anymore. If that's too impractical (which it may be), please
say so and I'll change the check. So please, ifyou are a committer, test
(I build-tested only, circumstanceshaven't changed) and if that's going OK,
please accept/committhe change attached separatelyto the public repository.
Thanks in advance.
Best regards, mabri
----- Original Message -----
From: zyx <z...@litepdf.cz>
To: podofo-users@lists.sourceforge.net
Sent: Wednesday, 13 July 2016, 17:30 UTC
Subject: Re: [Podofo-users] BUG: Erase method from PdfOutlineItem sometimes
segfaults
On Fri, 2016-06-10 at 23:30 +0000, Matthew Brincke wrote:
> I have now implemented my proposed fix, which is
> attached to this e-mail.
Hi,
thanks for the patch. It looks mostly fine and works as you described.
The application doesn't crash, but ends with an exception.
It introduces two compiler warnings though:
src/base/PdfError.cpp: In static member function ‘static const char*
PoDoFo::PdfError::ErrorName(PoDoFo::EPdfError)’:
src/base/PdfError.cpp:193:11: warning: enumeration value
‘ePdfError_OutlineItemAlreadyPresent’ not handled in switch [-Wswitch-enum]
switch( eCode )
^
src/base/PdfError.cpp: In static member function ‘static const char*
PoDoFo::PdfError::ErrorMessage(PoDoFo::EPdfError)’:
src/base/PdfError.cpp:359:11: warning: enumeration value
‘ePdfError_OutlineItemAlreadyPresent’ not handled in switch [-Wswitch-enum]
switch( eCode )
^
which explains why the printed error message is rather uninformative:
PoDoFo encounter an error. Error: 50
Callstack:
#0 Error Source: src/doc/PdfOutlines.cpp:155
I would also change the description of the method, because the current
description (also included in your patch):
+ /** Inserts an existing PdfOutlineItem as a child of this outline item.
invokes to me that "an existing" means something which is already
included in the tree. I know you described it in more detail later in
the paragraph, though it makes things just more confusing. A better
text beginning would be, from my point of view:
+ /** Inserts a new PdfOutlineItem as a child of this outline item.
Then the "detailed" description can be that the new item should not be
part of any tree, and this could be tested for, otherwise a confusion
about ownership of the inserted item is risen. With your proposed
description, consider that I'd take an item from a different tree and
PdfDocument (the description suggests it's possible). Will the
destination tree be corrupted if I delete the source document first? I
guess so, but I do not know for sure.
Bye,
zyxIndex: src/base/PdfError.cpp
===================================================================
--- src/base/PdfError.cpp (revision 1771)
+++ src/base/PdfError.cpp (working copy)
@@ -154,7 +154,7 @@
int i = 0;
- PdfError::LogErrorMessage( eLogSeverity_Error, "\n\nPoDoFo encounter an error. Error: %i %s\n", m_error, pszName ? pszName : "" );
+ PdfError::LogErrorMessage( eLogSeverity_Error, "\n\nPoDoFo encountered an error. Error: %i %s\n", m_error, pszName ? pszName : "" );
if( pszMsg )
PdfError::LogErrorMessage( eLogSeverity_Error, "\tError Description: %s\n", pszMsg );
@@ -342,6 +342,9 @@
case ePdfError_ChangeOnImmutable:
pszMsg = "ePdfError_ChangeOnImmutable";
break;
+ case ePdfError_OutlineItemAlreadyPresent:
+ pszMsg = "ePdfError_OutlineItemAlreadyPresent";
+ break;
case ePdfError_Unknown:
pszMsg = "ePdfError_Unknown";
break;
@@ -478,6 +481,9 @@
case ePdfError_NotCompiled:
pszMsg = "This feature was disabled during compile time.";
break;
+ case ePdfError_OutlineItemAlreadyPresent:
+ pszMsg = "Given OutlineItem already present in destination tree.";
+ break;
case ePdfError_Unknown:
pszMsg = "Error code unknown.";
break;
Index: src/base/PdfError.h
===================================================================
--- src/base/PdfError.h (revision 1771)
+++ src/base/PdfError.h (working copy)
@@ -93,7 +93,7 @@
ePdfError_InvalidStrokeStyle, /**< Invalid stroke style during drawing */
ePdfError_InvalidHexString, /**< Invalid hex string */
ePdfError_InvalidStream, /**< The stream is invalid */
- ePdfError_InvalidStreamLength, /**< The stream length is invlaid */
+ ePdfError_InvalidStreamLength, /**< The stream length is invalid */
ePdfError_InvalidKey, /**< The specified key is invalid */
ePdfError_InvalidName, /**< The specified Name is not valid in this context */
ePdfError_InvalidEncryptionDict, /**< The encryption dictionary is invalid or misses a required key */
@@ -119,11 +119,13 @@
ePdfError_NotImplemented, /**< This feature is currently not implemented. */
- ePdfError_DestinationAlreadyPresent,/**< An destination was already present when trying to add an Action */
+ ePdfError_DestinationAlreadyPresent,/**< A destination was already present when trying to add an Action */
ePdfError_ChangeOnImmutable, /**< Changing values on immutable objects is not allowed. */
ePdfError_NotCompiled, /**< This feature was disabled at compile time. */
+ ePdfError_OutlineItemAlreadyPresent,/**< An outline item to be inserted was already in that outlines tree. */
+
ePdfError_Unknown = 0xffff /**< Unknown error */
};
Index: src/doc/PdfOutlines.cpp
===================================================================
--- src/doc/PdfOutlines.cpp (revision 1771)
+++ src/doc/PdfOutlines.cpp (working copy)
@@ -121,6 +121,39 @@
void PdfOutlineItem::InsertChild( PdfOutlineItem* pItem )
{
+ PdfOutlineItem* pItemToCheckParent = pItem;
+ PdfOutlineItem* pRoot = NULL;
+ PdfOutlineItem* pRootOfThis = NULL;
+
+ if ( !pItemToCheckParent )
+ return;
+
+ while ( pItemToCheckParent )
+ {
+ while ( pItemToCheckParent->GetParentOutline() )
+ pItemToCheckParent = pItemToCheckParent->GetParentOutline();
+
+ if ( pItemToCheckParent == pItem ) // item can't have a parent
+ {
+ pRoot = pItem; // needed later, "root" can mean "standalone" here
+ break; // for performance in standalone or doc-merge case
+ }
+
+ if ( !pRoot )
+ {
+ pRoot = pItemToCheckParent;
+ pItemToCheckParent = this;
+ }
+ else
+ {
+ pRootOfThis = pItemToCheckParent;
+ pItemToCheckParent = NULL;
+ }
+ }
+
+ if ( pRoot == pRootOfThis ) // latter NULL if check skipped for performance
+ PODOFO_RAISE_ERROR( ePdfError_OutlineItemAlreadyPresent );
+
if( m_pLast )
{
m_pLast->SetNext( pItem );
Index: src/doc/PdfOutlines.h
===================================================================
--- src/doc/PdfOutlines.h (revision 1771)
+++ src/doc/PdfOutlines.h (working copy)
@@ -90,8 +90,14 @@
*/
PdfOutlineItem* CreateNext ( const PdfString & sTitle, const PdfAction & rAction );
- /** Inserts an existing PdfOutlineItem as a child
- * of this outline item.
+ /** Inserts a new PdfOutlineItem as a child of this outline item.
+ * The former can't be in the same tree as this one, as the tree property
+ * would be broken. If this prerequisite is violated, a PdfError
+ * exception (code ePdfError_OutlineItemAlreadyPresent) is thrown and
+ * nothing is changed.
+ * The item inserted is not copied, i.e. Erase() calls affect the original!
+ * Therefore also shared ownership is in effect, i.e. deletion by where it
+ * comes from damages the data structure it's inserted into.
*
* \param pItem an existing outline item
*/
------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are
consuming the most bandwidth. Provides multi-vendor support for NetFlow,
J-Flow, sFlow and other flows. Make informed decisions using capacity planning
reports.http://sdm.link/zohodev2dev
_______________________________________________
Podofo-users mailing list
Podofo-users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/podofo-users
