SkPDF: wait for jobs to complete on abort()

Before, we could only wait for all reserved objects to serialize.

The new unit test `SkPDF_abort_jobs` crashed before this change.

Change-Id: Ia2fcb33251c6c32c125f631ed3eeb619f616bef3
Reviewed-on: https://skia-review.googlesource.com/c/179856
Auto-Submit: Hal Canary <halcanary@google.com>
Commit-Queue: Herb Derby <herb@google.com>
Reviewed-by: Herb Derby <herb@google.com>

src/pdf/SkPDFBitmap.cpp

@@ -310,9 +310,11 @@ SkPDFIndirectReference SkPDFSerializeImage(const SkImage* img,
     SkPDFIndirectReference ref = doc->reserveRef();
     if (SkExecutor* executor = doc->executor()) {
         SkRef(img);
+        doc->incrementJobCount();
         executor->add([img, encodingQuality, doc, ref]() {
             serialize_image(img, encodingQuality, doc, ref);
             SkSafeUnref(img);
+            doc->signalJobComplete();
         });
         return ref;
     }

src/pdf/SkPDFDocument.cpp

@@ -218,7 +218,6 @@ SkWStream* SkPDFDocument::beginObject(SkPDFIndirectReference ref) {
 void SkPDFDocument::endObject() {
     end_indirect_object(this->getStream());
     fMutex.release();
-    fSemaphore.signal();
 };
 
 static SkSize operator*(SkISize u, SkScalar s) { return SkSize{u.width() * s, u.height() * s}; }
@@ -292,6 +291,7 @@ void SkPDFDocument::onEndPage() {
 }
 
 void SkPDFDocument::onAbort() {
+    this->waitForJobs();
     this->reset();
 }
 
@@ -468,6 +468,7 @@ static std::vector<const SkPDFFont*> get_fonts(const SkPDFCanon& canon) {
 void SkPDFDocument::onClose(SkWStream* stream) {
     SkASSERT(fCanvas.imageInfo().dimensions().isZero());
     if (fPages.empty()) {
+        this->waitForJobs();
         this->reset();
         return;
     }
@@ -502,13 +503,7 @@ void SkPDFDocument::onClose(SkWStream* stream) {
         f->emitSubset(this);
     }
 
-    int waits = 0;
-    // fNextObjectNumber can increase while we wait.
-    while (waits + 1 < fNextObjectNumber.load()) {
-        fSemaphore.wait();
-        ++waits;
-    }
-    SkASSERT(fNextObjectNumber.load() == fOffsetMap.objectCount());
+    this->waitForJobs();
     {
         SkAutoMutexAcquire autoMutexAcquire(fMutex);
         serialize_footer(fOffsetMap, this->getStream(), fInfoDict, docCatalogRef, fUUID);
@@ -516,6 +511,17 @@ void SkPDFDocument::onClose(SkWStream* stream) {
     this->reset();
 }
 
+void SkPDFDocument::incrementJobCount() { fJobCount++; }
+
+void SkPDFDocument::signalJobComplete() { fSemaphore.signal(); }
+
+void SkPDFDocument::waitForJobs() {
+     // fJobCount can increase while we wait.
+     while (fJobCount > 0) {
+         fSemaphore.wait();
+         --fJobCount;
+     }
+}
 
 ///////////////////////////////////////////////////////////////////////////////
 

src/pdf/SkPDFDocumentPriv.h

@@ -71,6 +71,8 @@ public:
     void endObject();
 
     SkExecutor* executor() const { return fExecutor; }
+    void incrementJobCount();
+    void signalJobComplete();
     size_t currentPageIndex() { return fPages.size(); }
     size_t pageCount() { return fPageRefs.size(); }
 
@@ -83,6 +85,7 @@ private:
     SkPDFDict fDests;
     sk_sp<SkPDFDevice> fPageDevice;
     std::atomic<int> fNextObjectNumber = {1};
+    std::atomic<int> fJobCount = {0};
     SkUUID fUUID;
     SkPDFIndirectReference fInfoDict;
     SkPDFIndirectReference fXMP;
@@ -98,6 +101,7 @@ private:
     SkSemaphore fSemaphore;
 
     void reset();
+    void waitForJobs();
 };
 
 #endif  // SkPDFDocumentPriv_DEFINED

src/pdf/SkPDFTypes.cpp

@@ -484,10 +484,12 @@ SkPDFIndirectReference SkPDFStreamOut(std::unique_ptr<SkPDFDict> dict,
         SkStreamAsset* contentPtr = content.release();
         // Pass ownership of both pointers into a std::function, which should
         // only be executed once.
+        doc->incrementJobCount();
         executor->add([dictPtr, contentPtr, deflate, doc, ref]() {
             serialize_stream(dictPtr, contentPtr, deflate, doc, ref);
             delete dictPtr;
             delete contentPtr;
+            doc->signalJobComplete();
         });
         return ref;
     }

tests/PDFDocumentTest.cpp

@@ -8,6 +8,7 @@
 
 #include "Resources.h"
 #include "SkCanvas.h"
+#include "SkExecutor.h"
 #include "SkOSFile.h"
 #include "SkOSPath.h"
 #include "SkPDFDocument.h"
@@ -239,3 +240,19 @@ DEF_TEST(SkPDF_multiple_pages, r) {
                 SkColorSetARGB(0xFF, 0x00, (uint8_t)(255.0f * i / (n - 1)), 0x00));
     }
 }
+
+// Test to make sure that jobs launched by PDF backend don't cause a segfault
+// after calling abort().
+DEF_TEST(SkPDF_abort_jobs, rep) {
+    SkBitmap b;
+    b.allocN32Pixels(612, 792);
+    b.eraseColor(0x4F9643A0);
+    SkPDF::Metadata metadata;
+    std::unique_ptr<SkExecutor> executor = SkExecutor::MakeFIFOThreadPool();
+    metadata.fExecutor = executor.get();
+    SkNullWStream dst;
+    sk_sp<SkDocument> doc = SkPDF::MakeDocument(&dst, metadata);
+    doc->beginPage(612, 792)->drawBitmap(b, 0, 0);
+    doc->abort();
+}
+