Avoid using the global workerPort when destruction has started, but not yet finished (issue 16777)
				
					
				
			Given that the `PDFDocumentLoadingTask.destroy()`-method is documented as being asynchronous, you thus need to await its completion before attempting to load a new PDF document when using the global `workerPort`. If you don't await destruction as intended then a new `getDocument`-call can remain pending indefinitely, without any kind of indication of the problem, as shown in the issue. In order to improve the current situation, without unnecessarily complicating the API-implementation, we'll now throw during the `getDocument`-call if the global `workerPort` is in the process of being destroyed. This part of the code-base has apparently never been covered by any tests, hence the patch adds unit-tests for both the *correct* usage (awaiting destruction) as well as the specific case outlined in the issue.
This commit is contained in:
		
							parent
							
								
									690b873897
								
							
						
					
					
						commit
						66437917db
					
				| @ -626,7 +626,17 @@ class PDFDocumentLoadingTask { | |||||||
|    */ |    */ | ||||||
|   async destroy() { |   async destroy() { | ||||||
|     this.destroyed = true; |     this.destroyed = true; | ||||||
|  |     try { | ||||||
|  |       if (this._worker?.port) { | ||||||
|  |         this._worker._pendingDestroy = true; | ||||||
|  |       } | ||||||
|       await this._transport?.destroy(); |       await this._transport?.destroy(); | ||||||
|  |     } catch (ex) { | ||||||
|  |       if (this._worker?.port) { | ||||||
|  |         delete this._worker._pendingDestroy; | ||||||
|  |       } | ||||||
|  |       throw ex; | ||||||
|  |     } | ||||||
| 
 | 
 | ||||||
|     this._transport = null; |     this._transport = null; | ||||||
|     if (this._worker) { |     if (this._worker) { | ||||||
| @ -2058,10 +2068,6 @@ class PDFWorker { | |||||||
|     port = null, |     port = null, | ||||||
|     verbosity = getVerbosityLevel(), |     verbosity = getVerbosityLevel(), | ||||||
|   } = {}) { |   } = {}) { | ||||||
|     if (port && PDFWorker.#workerPorts.has(port)) { |  | ||||||
|       throw new Error("Cannot use more than one PDFWorker per port."); |  | ||||||
|     } |  | ||||||
| 
 |  | ||||||
|     this.name = name; |     this.name = name; | ||||||
|     this.destroyed = false; |     this.destroyed = false; | ||||||
|     this.verbosity = verbosity; |     this.verbosity = verbosity; | ||||||
| @ -2072,6 +2078,9 @@ class PDFWorker { | |||||||
|     this._messageHandler = null; |     this._messageHandler = null; | ||||||
| 
 | 
 | ||||||
|     if (port) { |     if (port) { | ||||||
|  |       if (PDFWorker.#workerPorts.has(port)) { | ||||||
|  |         throw new Error("Cannot use more than one PDFWorker per port."); | ||||||
|  |       } | ||||||
|       PDFWorker.#workerPorts.set(port, this); |       PDFWorker.#workerPorts.set(port, this); | ||||||
|       this._initializeFromPort(port); |       this._initializeFromPort(port); | ||||||
|       return; |       return; | ||||||
| @ -2287,11 +2296,21 @@ class PDFWorker { | |||||||
|    * @param {PDFWorkerParameters} params - The worker initialization parameters. |    * @param {PDFWorkerParameters} params - The worker initialization parameters. | ||||||
|    */ |    */ | ||||||
|   static fromPort(params) { |   static fromPort(params) { | ||||||
|  |     if (typeof PDFJSDev !== "undefined" && PDFJSDev.test("MOZCENTRAL")) { | ||||||
|  |       throw new Error("Not implemented: fromPort"); | ||||||
|  |     } | ||||||
|     if (!params?.port) { |     if (!params?.port) { | ||||||
|       throw new Error("PDFWorker.fromPort - invalid method signature."); |       throw new Error("PDFWorker.fromPort - invalid method signature."); | ||||||
|     } |     } | ||||||
|     if (this.#workerPorts.has(params.port)) { |     const cachedPort = this.#workerPorts.get(params.port); | ||||||
|       return this.#workerPorts.get(params.port); |     if (cachedPort) { | ||||||
|  |       if (cachedPort._pendingDestroy) { | ||||||
|  |         throw new Error( | ||||||
|  |           "PDFWorker.fromPort - the worker is being destroyed.\n" + | ||||||
|  |             "Please remember to await `PDFDocumentLoadingTask.destroy()`-calls." | ||||||
|  |         ); | ||||||
|  |       } | ||||||
|  |       return cachedPort; | ||||||
|     } |     } | ||||||
|     return new PDFWorker(params); |     return new PDFWorker(params); | ||||||
|   } |   } | ||||||
|  | |||||||
| @ -898,6 +898,70 @@ describe("api", function () { | |||||||
|     }); |     }); | ||||||
|   }); |   }); | ||||||
| 
 | 
 | ||||||
|  |   describe("GlobalWorkerOptions", function () { | ||||||
|  |     let savedGlobalWorkerPort; | ||||||
|  | 
 | ||||||
|  |     beforeAll(function () { | ||||||
|  |       savedGlobalWorkerPort = GlobalWorkerOptions.workerPort; | ||||||
|  |     }); | ||||||
|  | 
 | ||||||
|  |     afterAll(function () { | ||||||
|  |       GlobalWorkerOptions.workerPort = savedGlobalWorkerPort; | ||||||
|  |     }); | ||||||
|  | 
 | ||||||
|  |     it("use global `workerPort` with multiple, sequential, documents", async function () { | ||||||
|  |       if (isNodeJS) { | ||||||
|  |         pending("Worker is not supported in Node.js."); | ||||||
|  |       } | ||||||
|  | 
 | ||||||
|  |       GlobalWorkerOptions.workerPort = new Worker( | ||||||
|  |         new URL("../../build/generic/build/pdf.worker.js", window.location) | ||||||
|  |       ); | ||||||
|  | 
 | ||||||
|  |       const loadingTask1 = getDocument(basicApiGetDocumentParams); | ||||||
|  |       const pdfDoc1 = await loadingTask1.promise; | ||||||
|  |       expect(pdfDoc1.numPages).toEqual(3); | ||||||
|  |       await loadingTask1.destroy(); | ||||||
|  | 
 | ||||||
|  |       const loadingTask2 = getDocument( | ||||||
|  |         buildGetDocumentParams("tracemonkey.pdf") | ||||||
|  |       ); | ||||||
|  |       const pdfDoc2 = await loadingTask2.promise; | ||||||
|  |       expect(pdfDoc2.numPages).toEqual(14); | ||||||
|  |       await loadingTask2.destroy(); | ||||||
|  |     }); | ||||||
|  | 
 | ||||||
|  |     it( | ||||||
|  |       "avoid using the global `workerPort` when destruction has started, " + | ||||||
|  |         "but not yet finished (issue 16777)", | ||||||
|  |       async function () { | ||||||
|  |         if (isNodeJS) { | ||||||
|  |           pending("Worker is not supported in Node.js."); | ||||||
|  |         } | ||||||
|  | 
 | ||||||
|  |         GlobalWorkerOptions.workerPort = new Worker( | ||||||
|  |           new URL("../../build/generic/build/pdf.worker.js", window.location) | ||||||
|  |         ); | ||||||
|  | 
 | ||||||
|  |         const loadingTask = getDocument(basicApiGetDocumentParams); | ||||||
|  |         const pdfDoc = await loadingTask.promise; | ||||||
|  |         expect(pdfDoc.numPages).toEqual(3); | ||||||
|  |         const destroyPromise = loadingTask.destroy(); | ||||||
|  | 
 | ||||||
|  |         expect(function () { | ||||||
|  |           getDocument(buildGetDocumentParams("tracemonkey.pdf")); | ||||||
|  |         }).toThrow( | ||||||
|  |           new Error( | ||||||
|  |             "PDFWorker.fromPort - the worker is being destroyed.\n" + | ||||||
|  |               "Please remember to await `PDFDocumentLoadingTask.destroy()`-calls." | ||||||
|  |           ) | ||||||
|  |         ); | ||||||
|  | 
 | ||||||
|  |         await destroyPromise; | ||||||
|  |       } | ||||||
|  |     ); | ||||||
|  |   }); | ||||||
|  | 
 | ||||||
|   describe("PDFDocument", function () { |   describe("PDFDocument", function () { | ||||||
|     let pdfLoadingTask, pdfDocument; |     let pdfLoadingTask, pdfDocument; | ||||||
| 
 | 
 | ||||||
|  | |||||||
		Loading…
	
	
			
			x
			
			
		
	
		Reference in New Issue
	
	Block a user