This has been wanted for a long time, but has been blocked on the synchronous nature of the install/uninstall API operations. It's simple to implement now, and also considerably simplifies the code.

Comment on attachment 8979004 [details] Bug 1462855: Part 1 - Move add-on install code to async file IO. https://reviewboard.mozilla.org/r/245278/#review252374 I'm skeptical about avoiding await in some places. I believe that it works right now but it feels fragile and will be confusing to people reading this code. On the flip side, what do we really save by not just awaiting every I/O operation? ::: commit-message-8bfa6:12 (Diff revision 2) > +unpacked extensions, which tended to introduce a lot of additional corner > +cases with inconsistent file permissions, and partially successful recursive > +copy operations. Since installing unpacked extensions is no longer supported, > +most of this special-casing is no longer necessary. > + > +This patch also introduces a `serialize` primitive to making sure that certain "making sure" -> ensure ::: toolkit/mozapps/extensions/internal/XPIInstall.jsm (Diff revision 2) > - if (stagedAddon.exists()) > - recursiveRemove(stagedAddon); Why is this getting removed? ::: toolkit/mozapps/extensions/internal/XPIInstall.jsm:2775 (Diff revision 2) > - try { > - if (trashDirExists) > + // These don't need to `await`. They'll be queued for the IO thread, and > + // executed in order before any subsequent file operations. This is making an assumption about the implementation of OS.File (ie that these operations will complete immediately when the io thread handles them) that may be true today, but it wouldn't be unreasonable for them to change in the future. What's the harm in having this return a Promise and ensuring that operations that touch the trash directory run after that Promise resolves? ::: toolkit/mozapps/extensions/internal/XPIInstall.jsm:2834 (Diff revision 2) > - } > > if (action == "copy") { > - transaction.copy(source, this.dir); > + await transaction.copyUnder(source.path, this.dir.path); > } else if (action == "move") { > flushJarCache(source); We're already doing this from loadManifest(), do we need it again here?

(In reply to Andrew Swan [:aswan] from comment #5) > On the flip side, what do we really save by not just awaiting every I/O > operation? We save the round trip to the IO thread, which doesn't always matter, but can add up. In this case, it actually makes it *more* likely that we'll run into issues with the directory we created not existing, since it gives other code more of a chance to delete it. > ::: toolkit/mozapps/extensions/internal/XPIInstall.jsm > (Diff revision 2) > > - if (stagedAddon.exists()) > > - recursiveRemove(stagedAddon); > > Why is this getting removed? Because I couldn't think of a particularly compelling reason for it to exist. I suppose it can just be changed to an unchecked async remove call, though. > ::: toolkit/mozapps/extensions/internal/XPIInstall.jsm:2775 > (Diff revision 2) > > - try { > > - if (trashDirExists) > > + // These don't need to `await`. They'll be queued for the IO thread, and > > + // executed in order before any subsequent file operations. > > This is making an assumption about the implementation of OS.File (ie that > these operations will complete immediately when the io thread handles them) > that may be true today, but it wouldn't be unreasonable for them to change > in the future. It's not making assumptions about the implementation. Operations on the IO thread are guaranteed to happen in the order they were dispatched, and all operations that depend on this happen in the IO thread, and will be queued after this. If that ever changes, this (and anything else that schedules OS.File operations without awaiting on the result) can easily be changed. This is all in-tree code, and tested. There isn't any real risk of breaking changes going unnoticed. > ::: toolkit/mozapps/extensions/internal/XPIInstall.jsm:2834 > (Diff revision 2) > > - } > > > > if (action == "copy") { > > - transaction.copy(source, this.dir); > > + await transaction.copyUnder(source.path, this.dir.path); > > } else if (action == "move") { > > flushJarCache(source); > > We're already doing this from loadManifest(), do we need it again here? No idea. I expect Windows will explode if you look at it wrong, though.

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #6) > (In reply to Andrew Swan [:aswan] from comment #5) > > > On the flip side, what do we really save by not just awaiting every I/O > > operation? > > We save the round trip to the IO thread, which doesn't always matter, but > can add up. In this case, it actually makes it *more* likely that we'll run > into issues with the directory we created not existing, since it gives other > code more of a chance to delete it. I don't believe this is worth it. It may be very clear to you but to somebody trying to understand or work with this code who isn't intimately familiar with how OS.File works, this is one more thing to understand and worry about, compared with the familiar pattern of just awaiting an asynchronous operation. Conversely, can you provide some evidence that this makes a meaningful difference? As for races between different tasks, those should be fixed by properly serializing tasks that touch the same directory.

(In reply to Andrew Swan [:aswan] from comment #9) > I don't believe this is worth it. It may be very clear to you but to > somebody trying to understand or work with this code who isn't intimately > familiar with how OS.File works, this is one more thing to understand and > worry about, compared with the familiar pattern of just awaiting an > asynchronous operation. Conversely, can you provide some evidence that this > makes a meaningful difference? I don't understand what's hard to understand. OS.File operations always happen in order. It's simple. If the only thing that depends on an operation is another OS.File operation that happens after it, there's no need to await. > As for races between different tasks, those should be fixed by properly > serializing tasks that touch the same directory. They should. But that's a lot of work, and it's down the road.

(In reply to Kris Maglione [:kmag] from comment #10) > I don't understand what's hard to understand. OS.File operations always > happen in order. It's simple. If the only thing that depends on an operation > is another OS.File operation that happens after it, there's no need to await. The concept isn't hard to understand but it adds some overhead to reading and understanding some code. And it doesn't seem out of the question that somebody might re-order operations or remove the final one in some chain, but not adjust the awaits properly. To be clear, I think this is a relatively small issue, but the benefit we would get (lower latency for installs and uninstalls which aren't something that users do in an interactive way anyway) just does not seem worth it.