[filters] Extend table lifetime
ClosedPublic

Authored by anthonyfieroni on Sep 10 2018, 6:29 PM.

Details

Summary

CCBUG: 379255

Test Plan

Concurrent calls on getPresetTable can lead to race condition but i can't see such a usage, it can declared thread_local in such cases

Diff Detail

Repository
R8 Calligra
Lint
Lint Skipped
Unit
Unit Tests Skipped
anthonyfieroni created this revision.Sep 10 2018, 6:29 PM
Restricted Application added a project: Calligra: 3.0. · View Herald TranscriptSep 10 2018, 6:29 PM
Restricted Application added a subscriber: Calligra-Devel-list. · View Herald Transcript
anthonyfieroni requested review of this revision.Sep 10 2018, 6:29 PM
anthonyfieroni edited the test plan for this revision. (Show Details)

Aggressive refactor, it makes m_table a shared pointer, presetTables is one time initialized.

Ping on this, did you try it?

Sorry, don't have any qualified opinons, I don't quite understand the original code either, so...
Imo if you think it is good, land it.

No idea either - jaroslaw was the original author so maybe he can take a look

staniek requested changes to this revision.Sep 13 2018, 8:52 AM
staniek added a subscriber: staniek.

Thanks, lots of other people also touched this code but I tried this time to review :) I propose to separate the string literal changes.

Also I wonder why there's "Context not available." error in this review, was the patch delivered using arc tools?

filters/libmsooxml/MsooXmlDrawingReaderTableImpl.h
58 ↗(On Diff #41377)

This changes life time

filters/libmsooxml/MsooXmlDrawingReaderTableMethods.h
40

This is declaration-only header, so better no, otherwise allocation of a new KoTable is added to all places we include the header, e.g. also PptxXmlSlideReader and in the future.

filters/libodf2/KoTable.h
112

Good idea, can work but I would propose to more consistent naming, e.g. like from KOSTYLE_DECLARE_SHARED_POINTER:

typedef QSharedPointer<KoTable> Ptr;

(inside of KoTable decl)

filters/words/docx/import/DocxXmlDocumentReader.cpp
5446 ↗(On Diff #41377)

HERE1

6082

Isn't this change in logic?

filters/words/docx/import/DocxXmlDocumentReader.h
258

No code in declaration-only headers please, implementation may be moved to "HERE1" below. This way you won't need the extra #include here too.

This revision now requires changes to proceed.Sep 13 2018, 8:52 AM

It can be allocated only in read_tbl but as you can see in bug report, use-after-free is very common to happen.

filters/libmsooxml/MsooXmlDrawingReaderTableMethods.h
40

That isn't a problem, it's initialized at constructor time.

filters/words/docx/import/DocxXmlDocumentReader.cpp
6082

No

staniek added inline comments.Sep 13 2018, 12:03 PM
filters/libmsooxml/MsooXmlDrawingReaderTableMethods.h
40

I know but original code was not called new KoTable for PptxXmlSlideReader, right?

filters/words/docx/import/DocxXmlDocumentReader.cpp
6082

Did we have new KoTable before?

staniek accepted this revision.Sep 13 2018, 3:31 PM

Good job!

This revision is now accepted and ready to land.Sep 13 2018, 3:31 PM
This revision was automatically updated to reflect the committed changes.