1462855 - Use async IO for add-on install and uninstall operations

Status: NEW

(Toolkit :: Add-ons Manager, enhancement, P3)

Description

[Kris Maglione [:kmag]]

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 1

[Kris Maglione [:kmag]]

Attachment: Bug 1462855: Part 1 - Move add-on install code to async file IO.

This patch moves add-on install and uninstall operations to async file IO,
while removing a lot of cruft and baggage that was included in the old code.

A lot of the existing complexity and special cases were there to support
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
potentially-conflicting asynchronous operations do not overlap with each
other. At the moment, this is only used for the add-on set delayed update
code, which is racy as-is, but becomes racier as we add more asynchrony. In
the future, we'll probably need to do something similar elsewhere, though.

Review commit: https://reviewboard.mozilla.org/r/245278/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/245278/

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1462855: Part 2 - Rename AddonInternal._sourceBundle to .file.

This name is confusing, overly-verbose, and one of the last remaining
important property naming difference between XPIState and AddonInternal
instances.

Review commit: https://reviewboard.mozilla.org/r/245316/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/245316/

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8979004 [details]
Bug 1462855: Part 1 - Move add-on install code to async file IO.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/245278/diff/1-2/

Comment 5

[Andrew Swan [:aswan]]

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?

Comment 6

[Kris Maglione [:kmag]]

(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.

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8979004 [details]
Bug 1462855: Part 1 - Move add-on install code to async file IO.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/245278/diff/2-3/

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8979056 [details]
Bug 1462855: Part 2 - Rename AddonInternal._sourceBundle to .file.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/245316/diff/1-2/

Comment 9

[Andrew Swan [:aswan]]

(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.

Comment 10

[Kris Maglione [:kmag]]

(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.

Comment 11

[Andrew Swan [:aswan]]

(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.

Comment 13

[Kris Maglione [:kmag]]

Attachment: Bug 1462855: Part 1 - Move add-on install code to async file IO. r=aswan

This patch moves add-on install and uninstall operations to async file IO,
while removing a lot of cruft and baggage that was included in the old code.

A lot of the existing complexity and special cases were there to support
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 ensure that certain
potentially-conflicting asynchronous operations do not overlap with each
other. At the moment, this is only used for the add-on set delayed update
code, which is racy as-is, but becomes racier as we add more asynchrony. In
the future, we'll probably need to do something similar elsewhere, though.

MozReview-Commit-ID: AlvZXyP3MVF

Comment 14

[Kris Maglione [:kmag]]

Attachment: Bug 1462855: Part 2 - Rename AddonInternal._sourceBundle to .file. r=aswan

This name is confusing, overly-verbose, and one of the last remaining
important property naming difference between XPIState and AddonInternal
instances.

MozReview-Commit-ID: L19trL8XsyD