[api-minor] Ensure that the getDocument Promise is rejected if the loadingTask is destroyed, or an Error is thrown, inside of the onPassword callback (issue 7806)
#7926
Conversation
| @@ -1435,6 +1436,13 @@ var WorkerTransport = (function WorkerTransportClosure() { | |||
| this.destroyed = true; | |||
| this.destroyCapability = createPromiseCapability(); | |||
|
|
|||
| if (this._passwordCapability) { | |||
| var err = new Error('Worker was destroyed during onPassword callback'); | |||
| this.loadingTask._capability.reject(err); | |||
There was a problem hiding this comment.
I was thinking we can avoid doing that: the loading task canceling shall be coming from worker side.
There was a problem hiding this comment.
This is definitely getting more complicated than I imagined; thanks for your in-depth review comments thus far!
I tried removing that, but it appears to be causing some problems:
When calling loadingTask.destroy(); in the onPassword handler, it seems that the worker is terminated before it has had a chance to send the (new) PasswordException (as outlined in comment https://github.com/mozilla/pdf.js/pull/7926/files#r94335620).
The loadingTask will thus remain in a pending state, without ever getting either resolved or rejected. Note that I'm seeing this both in manual tests in the viewer, as well as in the unit-tests.
EDIT: For now, I removed this and use a WorkerTask to ensure that we wait (in worker.js) for the _passwordCapability from api.js. How would you prefer that we handle this case?
| try { | ||
| loadingTask.onPassword(updatePassword, exception.code); | ||
| return this._passwordCapability.promise; | ||
| } catch (ex) { } |
There was a problem hiding this comment.
We are properly handling exception at sendWithPromise, so we can avoid doing error capture here.
| var error = new PasswordException(exception.message, exception.code); | ||
| loadingTask._capability.reject(error); | ||
|
|
||
| this._passwordCapability.reject(error); |
There was a problem hiding this comment.
can you check for !loadingTask.onPassword and returned rejected promise (with PasswordException).
| }, this); | ||
|
|
||
|
|
There was a problem hiding this comment.
nit: remove unneeded line
| } | ||
| handler.sendWithPromise('Password', e).then(function (data) { | ||
| pdfManager.updatePassword(data.password); | ||
| }); |
There was a problem hiding this comment.
add .catch(function (e) { handler.send('PasswordException', e); });. (I'm not sure if we need to re-wrap e, but I think this will work out) We need to add 'PasswordException' handler, to abort loading task. Password can be PasswordRequest to avoid confusion.
There was a problem hiding this comment.
The code at pdfManagerReady below looks non-symmetrical to recovery password handling. I think we shall remove pdfManager.passwordChanged/._passwordChangedCapability? The pdfManagerReady will be called after password update (we need to move pdfManagerReady from being anonymous function to closure before getPdfManager() call).
There was a problem hiding this comment.
add .catch(function (e) { handler.send('PasswordException', e); });. (I'm not sure if we need to re-wrap e, but I think this will work out)
A complication with this is that the exception we catch here is actually a string (it's converted by the MessageManager), rather than the expected Error here since postMessage has issues with serializing those.
EDIT: For now, the patch is still sending the original PasswordException in this case, since I'm having trouble getting the rejection of the loadingTask to work properly otherwise.
|
@yurydelendik Thanks for the in-depth review comments! I've tried to address them, but there's a couple of things I'm not entirely sure how to handle, please see #7926 (comment) and #7926 (comment). |
…`loadingTask` is destroyed, or an `Error` is thrown, inside of the `onPassword` callback (issue 7806) This patch also removes the `UpdatePassword` message, in favour of using the `sendWithPromise` method of `MessageHandler`. Furthermore, the patch also refactors the `BasePdfManager_updatePassword`/`BasePdfManager_passwordChanged` methods (in pdf_manager.js), and the `pdfManagerReady` function (in worker.js).
…er`s, instead of in the `XRef` We're already passing in a, currently unused, `PdfManager` instance when initializing the `XRef`. To avoid having to pass a single `password` parameter around, we could thus simply get the `password` through the `PdfManager` instance instead.
|
/botio test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.22.172.223:8877/66a013cc55d0e54/output.txt |
From: Bot.io (Linux)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.21.233.14:8877/70ebc3f297bfa0d/output.txt |
From: Bot.io (Windows)FailedFull output at http://107.22.172.223:8877/66a013cc55d0e54/output.txt Total script time: 25.99 mins
Image differences available at: http://107.22.172.223:8877/66a013cc55d0e54/reftest-analyzer.html#web=eq.log |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/70ebc3f297bfa0d/output.txt Total script time: 26.82 mins
|
|
/botio-windows test |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.22.172.223:8877/20d27329fbde995/output.txt |
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/20d27329fbde995/output.txt Total script time: 25.71 mins
|
…rt/throw-Promise [api-minor] Ensure that the `getDocument` Promise is rejected if the `loadingTask` is destroyed, or an `Error` is thrown, inside of the `onPassword` callback (issue 7806)
Tentatively fixes #7806 (and replaces PR #7924).
This change is