1149732 - GMPInstallManager does a main thread IO in nsZipArchive::ExtractFile

Status: RESOLVED FIXED

(Toolkit :: General, defect, P2)

Description

[Honza Bambas (:mayhemer)]

Current m-c, enabled profiling (if has any effect on this), fresh profile.

0 GMPExtractor.prototype.install/<(entry = "eme-adobe.dll", 2, eme-adobe.voucher,eme-adobe.info,eme-adobe.dll) ["resource://gre/modules/GMPInstallManager.jsm":738]
    this = [object Object]
1 forEach(callbackfn = entry => {
        // We don't need these types of files
        if (entry.contains("__MACOSX")) {
          return;
        }
        let outFile = Cc["@mozilla.org/file/local;1"].
                      createInstance(Ci.nsILocalFile);
        outFile.initWithPath(this.installToDirPath);
        outFile.appendRelativePath(entry);

        // Make sure the directory hierarchy exists
        if(!outFile.parent.exists()) {
          outFile.parent.create(Ci.nsIFile.DIRECTORY_TYPE, parseInt("0755", 8));
        }
        zipReader.extract(entry, outFile);
        extractedPaths.push(outFile.path);
        log.info(entry + " was successfully extracted to: " +
            outFile.path);
      }) ["self-hosted":211]
    this = eme-adobe.voucher,eme-adobe.info,eme-adobe.dll
2 GMPExtractor.prototype.install() ["resource://gre/modules/GMPInstallManager.jsm":724]
    this = [object Object]
3 GMPDownloader.prototype.onStopRequest/<(undefined) ["resource://gre/modules/GMPInstallManager.jsm":871]
    this = [object Object]
4 Handler.prototype.process() ["resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js":867]
    this = [object Object]
5 this.PromiseWalker.walkerLoop() ["resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js":746]
    this = [object Object]
6 this.PromiseWalker.scheduleWalkerLoop/<(undefined) ["resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js":688]
    this = [object Object]

>	nss3.dll!_PR_MD_WRITE(0x00000000, 0x00429844, 4096) Line 286	C
 	nss3.dll!FileWrite(0x0a2bb220, 0x00429844, 4096) Line 77	C
 	xul.dll!nsZipArchive::ExtractFile(0x1cbf2010, 0x0042a890, 0x0a2bb220) Line 450	C++
 	xul.dll!nsJAR::Extract({...}, 0x0a4f9400) Line 262	C++
 	xul.dll!NS_InvokeByIndex(0x1e4ccc40, 9, 2, 0x0042a940) Line 71	C++
 	xul.dll!XPCWrappedNative::CallMethod({...}, CALL_METHOD) Line 1385	C++
 	xul.dll!XPC_WN_CallMethod(0x1978e180, 2, 0x1978e1bc) Line 1140	C++
 	xul.dll!js::Invoke(0x00d51420, {...}, NO_CONSTRUCT) Line 463	C++
 	xul.dll!Interpret(0x00d51420, {...}) Line 2592	C++
 	xul.dll!js::RunScript(0x00d51420, {...}) Line 420	C++
 	xul.dll!js::Invoke(0x00d51420, {...}, NO_CONSTRUCT) Line 492	C++
 	xul.dll!Interpret(0x00d51420, {...}) Line 2592	C++
 	xul.dll!js::RunScript(0x00d51420, {...}) Line 420	C++
 	xul.dll!js::Invoke(0x00d51420, {...}, NO_CONSTRUCT) Line 492	C++
 	xul.dll!Interpret(0x00d51420, {...}) Line 2592	C++
 	xul.dll!js::RunScript(0x00d51420, {...}) Line 420	C++
 	xul.dll!js::Invoke(0x00d51420, {...}, NO_CONSTRUCT) Line 492	C++
 	xul.dll!js::fun_call(0x00d51420, 1, 0x0042d26c) Line 1240	C++
 	xul.dll!js::Invoke(0x00d51420, {...}, NO_CONSTRUCT) Line 463	C++
 	xul.dll!js::Invoke(0x00d51420, {...}, {...}, 2, 0x07bce118, {...}) Line 526	C++
 	xul.dll!js::CrossCompartmentWrapper::call(0x07b16920, {...}, {...}) Line 289	C++
 	xul.dll!js::Proxy::call(0x00d51420, {...}, {...}) Line 391	C++
 	xul.dll!js::proxy_Call(0x00d51420, 2, 0x07bce108) Line 697	C++
 	xul.dll!js::Invoke(0x00d51420, {...}, NO_CONSTRUCT) Line 463	C++
 	xul.dll!Interpret(0x00d51420, {...}) Line 2592	C++
 	xul.dll!js::RunScript(0x00d51420, {...}) Line 420	C++
 	xul.dll!js::Invoke(0x00d51420, {...}, NO_CONSTRUCT) Line 492	C++
 	xul.dll!js::CallOrConstructBoundFunction(0x00d51420, 0, 0x07bce050) Line 1589	C++
 	xul.dll!js::Invoke(0x00d51420, {...}, NO_CONSTRUCT) Line 463	C++
 	xul.dll!Interpret(0x00d51420, {...}) Line 2592	C++
 	xul.dll!js::RunScript(0x00d51420, {...}) Line 420	C++
 	xul.dll!js::Invoke(0x00d51420, {...}, NO_CONSTRUCT) Line 492	C++
 	xul.dll!js::Invoke(0x00d51420, {...}, {...}, 1, 0x0042f13c, {...}) Line 526	C++
 	xul.dll!JS::Call(0x00d51420, {...}, {...}, {...}, {...}) Line 4331	C++
 	xul.dll!mozilla::dom::AnyCallback::Call(0x00d51420, {...}, {...}, {...}, {...}) Line 79	C++
 	xul.dll!mozilla::dom::AnyCallback::Call({...}, {...}, {...}, eRethrowExceptions, 0x07b16920) Line 122	C++
 	xul.dll!mozilla::dom::WrapperPromiseCallback::Call(0x00d51420, {...}) Line 214	C++
 	xul.dll!mozilla::dom::PromiseCallbackTask::Run() Line 93	C++
 	xul.dll!mozilla::dom::Promise::PerformMicroTaskCheckpoint() Line 436	C++
 	xul.dll!nsXPConnect::AfterProcessNextEvent(0x00d44160, 0, true) Line 989	C++
 	xul.dll!nsThread::ProcessNextEvent(false, 0x0042f55f) Line 890	C++
 	xul.dll!NS_ProcessNextEvent(0x01d44160, false) Line 265	C++
 	xul.dll!mozilla::ipc::MessagePump::Run(0x00d750c0) Line 99	C++
 	xul.dll!MessageLoop::RunHandler() Line 227	C++
 	xul.dll!MessageLoop::Run() Line 201	C++
 	xul.dll!nsBaseAppShell::Run() Line 166	C++
 	xul.dll!nsAppShell::Run() Line 180	C++
 	xul.dll!nsAppStartup::Run() Line 282	C++
 	xul.dll!XREMain::XRE_mainRun() Line 4172	C++

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

This is really obvious if I run my Firefox profile off an old/slow SD card.

Comment 2

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

David - do you know who owns this stuff?

Comment 3

[Stephen A Pohl [:spohl]]

I've got a couple of photon and quantum flow bugs on my plate, but unless David has someone else in mind, I'd be happy to take a stab at this as soon as possible.

Since this is a qf:p1 bug, should the general priority be a P1 too or are these independent?

Comment 4

[David Durst [:ddurst]]

I don't know who owns it, but I'm fine if spohl takes it.

Comment 5

[Florian Quèze [:florian]]

Here is a profile where it causes 27ms of jank on a the quantum reference device (low end laptop without ssd) https://perfht.ml/2qjeLzs

Comment 6

[Marco Castelluccio [:marco]]

The ZIP extraction could be implemented with https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/ZipUtils.jsm.

Comment 7

[:Gijs (he/him)]

(In reply to Marco Castelluccio [:marco] from comment #6)
> The ZIP extraction could be implemented with
> https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/ZipUtils.jsm.

That would be better than the status quo, but AFAICT it still passes all the data through the main thread with runnable dispatching, and then off to the OS File worker thread... It would be swell if we could add "proper" async extraction & entry iteration APIs to nsIZipReader.

Comment 8

[Stephen A Pohl [:spohl]]

(In reply to :Gijs from comment #7)
> (In reply to Marco Castelluccio [:marco] from comment #6)
> > The ZIP extraction could be implemented with
> > https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/ZipUtils.jsm.
> 
> That would be better than the status quo, but AFAICT it still passes all the
> data through the main thread with runnable dispatching, and then off to the
> OS File worker thread... It would be swell if we could add "proper" async
> extraction & entry iteration APIs to nsIZipReader.

I've explored this idea, but I'm concerned about the fact that we're flattening the folder structure in this loop[1] and we would still need this loop, even with async extraction. We also want to ignore some files in the archive[2]. Adding an async function to nsIZipReader that would flatten the structure doesn't seem to make sense, since this might be the only place where we need to do this.

I wanted to explore if we could use a worker thread in the GMPInstallManager.jsm, but I'm not sure if we're going to run into thread-safety issues by calling into nsIZipReader from a worker thread. This solution, if possible, would be nice because the specialized logic of ignoring files and flattening the structure could be kept in GMPInstallManager. Gijs, thoughts?


[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/GMPInstallManager.jsm#435
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/GMPInstallManager.jsm#428

Comment 9

[:Gijs (he/him)]

(In reply to Stephen A Pohl [:spohl] from comment #8)
> (In reply to :Gijs from comment #7)
> > (In reply to Marco Castelluccio [:marco] from comment #6)
> > > The ZIP extraction could be implemented with
> > > https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/ZipUtils.jsm.
> > 
> > That would be better than the status quo, but AFAICT it still passes all the
> > data through the main thread with runnable dispatching, and then off to the
> > OS File worker thread... It would be swell if we could add "proper" async
> > extraction & entry iteration APIs to nsIZipReader.
> 
> I've explored this idea, but I'm concerned about the fact that we're
> flattening the folder structure in this loop[1] and we would still need this
> loop, even with async extraction. We also want to ignore some files in the
> archive[2]. Adding an async function to nsIZipReader that would flatten the
> structure doesn't seem to make sense, since this might be the only place
> where we need to do this.

Right. If we have an async API like OS.File.DirectoryIterator ( https://developer.mozilla.org/docs/Mozilla/JavaScript_code_modules/OSFile.jsm/OS.File.DirectoryIterator_for_the_main_thread ) that returns entry strings, and another API that looks something like this:

Promise extractAsync(entryPath, absoluteFilePath);

wouldn't that work? Then the consumer (GMPInstallManager in this case) can still do the directory flattening by modifying the entry's path before passing it to extractAsync, right?

> I wanted to explore if we could use a worker thread in the
> GMPInstallManager.jsm, but I'm not sure if we're going to run into
> thread-safety issues by calling into nsIZipReader from a worker thread. This
> solution, if possible, would be nice because the specialized logic of
> ignoring files and flattening the structure could be kept in
> GMPInstallManager. Gijs, thoughts?

This would also work, but I don't have enough know-how of gecko/XPCOM/nsIZipReader to comment on whether using nsIZipReader from a worker thread is safe. In principle, if the ref to the zipreader never leaves that worker thread, I would assume so, but then I assume lots of things, and if the zipreader somehow has a pool or static/global cache, it might not. Though, AFAICT, nsZipArchive just says "don't pass references to me across thread boundaries", which isn't the same as "don't use me off-main-thread". If there are existing off-mainthread consumers that'd help give some confidence...

Comment 10

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfc1cc3a4ab1da9cccd078fef7af8515f94e2950

Comment 11

[Stephen A Pohl [:spohl]]

Attachment: wip

This patch uses a ChromeWorker to extract files from the GMP zip file using jar: file URLs. This works for me locally on macOS 10.12.5 with local builds. I've kicked off a try build to see how we do in our tests across other platforms (see comment 10). This patch cannot yet handle files in subdirectories in a zip file. The GMP modules for OpenH264 and Widevine that get automatically downloaded by nightly don't seem to have subdirectories and I'm not sure how these would appear in our jar: directory listing. I figured I'd get some initial feedback before adding support for subdirectories.

Gijs, how does this approach look to you?

Comment 12

[:Gijs (he/him)]

Comment on attachment 8873593 [details] [diff] [review]
wip

Review of attachment 8873593 [details] [diff] [review]:
-----------------------------------------------------------------

There's a few issues still, but on the whole the approach looks sane to me, on the assumption that you've verified this takes the IO off the main thread. Thanks for working on this!

::: toolkit/modules/GMPExtractorWorker.js
@@ +5,5 @@
> +"use strict";
> +
> +importScripts("resource://gre/modules/osfile.jsm");
> +
> +onmessage = function(msg) {

I wonder, can you declare this to be an async function? Then you could use something like:

let response = await fetch(jarPath);
let dirListing = await response.text();

etc.

which would make this slightly simpler to read.

@@ +25,5 @@
> +        // We don't need these types of files.
> +        if (fileName == "verified_contents.json" ||
> +            fileName == "icon-128x128.png") {
> +          continue;
> +        }

Should we just skip everything with particular extensions? `fileName.endsWith(".png") etc.

Wonder if we could/should just whitelist only the files we need.

@@ +28,5 @@
> +          continue;
> +        }
> +        let filePath = jarPath + fileName;
> +        fetch(filePath).then(function(filePathResponse) {
> +          return filePathResponse.text();

Shouldn't this use .blob() ?

Also, I'd use an arrow function here for brevity:

fetch(filePath).then(resp => resp.text()).then(...

though async/await will make this even more readable...

@@ +41,5 @@
> +            let curPath = "";
> +            for (let j = 0; j < destPathDirs.length; j++) {
> +              curPath += destPathDirs[j] + "/";
> +              try {
> +                OS.File.makeDir(curPath, {unixMode: 0o755});

I think this should pass whatever the switch is for allowing dirs to exist without throwing an exception.

@@ +42,5 @@
> +            for (let j = 0; j < destPathDirs.length; j++) {
> +              curPath += destPathDirs[j] + "/";
> +              try {
> +                OS.File.makeDir(curPath, {unixMode: 0o755});
> +              } catch(e) { }

This should probably actually handle errors. :-)

@@ +47,5 @@
> +            }
> +            try {
> +              let curFile = OS.File.open(destPath, {create: true});
> +              curFile.close()
> +            } catch(e) { }

Why do we need to do this? This doesn't write anything, but the next line (writeAtomic) does. Why do we open and then close the file?

@@ +58,5 @@
> +        });
> +      }
> +      postMessage({
> +        "result": "success",
> +        "paths": extractedPaths

I'm sure I'm being dumb, but it doesn't look like the fetch()s of the files and their processing blocks the loop, and therefore I expect this postMessage to happen irrespective of whether the files have been extracted or not.

::: toolkit/modules/GMPInstallManager.jsm
@@ +382,5 @@
>     *         See GMPInstallManager.installAddon for resolve/rejected info
>     */
>    install() {
> +    let log = getScopedLogger("GMPExtractor");
> +    this._deferred = Promise.defer();

Do we need to use Promise.defer()? I'm not super familiar with this code, but if we can just make install() an async function (which will make it return a promise) then that seems simpler.

Comment 13

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d71cc79141f280c97dac4dc5f8ce84b4ca4c2f0

Comment 14

[Stephen A Pohl [:spohl]]

Attachment: Patch

Updated per feedback. Green try run in comment 13.

(In reply to :Gijs from comment #12)
> Comment on attachment 8873593 [details] [diff] [review]
> wip
> 
> Review of attachment 8873593 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There's a few issues still, but on the whole the approach looks sane to me,
> on the assumption that you've verified this takes the IO off the main
> thread. Thanks for working on this!

As discussed via IRC, I couldn't find a good way to verify this. If anyone knows of a way to verify, I'm happy to do so. I can confirm that about:debugging#workers shows that one worker is instantiated per GMP that is being installed.

> ::: toolkit/modules/GMPExtractorWorker.js
> @@ +5,5 @@
> > +"use strict";
> > +
> > +importScripts("resource://gre/modules/osfile.jsm");
> > +
> > +onmessage = function(msg) {
> 
> I wonder, can you declare this to be an async function? Then you could use
> something like:
> 
> let response = await fetch(jarPath);
> let dirListing = await response.text();
> 
> etc.
> 
> which would make this slightly simpler to read.

Done.

> @@ +25,5 @@
> > +        // We don't need these types of files.
> > +        if (fileName == "verified_contents.json" ||
> > +            fileName == "icon-128x128.png") {
> > +          continue;
> > +        }
> 
> Should we just skip everything with particular extensions?
> `fileName.endsWith(".png") etc.
> 
> Wonder if we could/should just whitelist only the files we need.

Whitelisting isn't possible because GMPs may be released for Firefox versions after they ship. The filenames to skip are taken from existing code, minus the __MACOSX folder (since this patch doesn't currently support GMPs with subdirectories). I will need to check with someone if we ever expect to have GMPs with subdirectories at this time.

> @@ +28,5 @@
> > +          continue;
> > +        }
> > +        let filePath = jarPath + fileName;
> > +        fetch(filePath).then(function(filePathResponse) {
> > +          return filePathResponse.text();
> 
> Shouldn't this use .blob() ?

.blob() was my preferred option, but I couldn't get it to work. No matter what I tried, I couldn't seem to convert this correctly for the second argument to OS.File.writeAtomic(). Currently, there doesn't seem to be a single instance where we write a blob to a file via OS.File.writeAtomic() in our codebase.

> Also, I'd use an arrow function here for brevity:
> 
> fetch(filePath).then(resp => resp.text()).then(...
> 
> though async/await will make this even more readable...

Switched to async/await.

> @@ +41,5 @@
> > +            let curPath = "";
> > +            for (let j = 0; j < destPathDirs.length; j++) {
> > +              curPath += destPathDirs[j] + "/";
> > +              try {
> > +                OS.File.makeDir(curPath, {unixMode: 0o755});
> 
> I think this should pass whatever the switch is for allowing dirs to exist
> without throwing an exception.

It turns out that this doesn't seem to throw an exception when the directory exists, so I dropped the try/catch.

> @@ +42,5 @@
> > +            for (let j = 0; j < destPathDirs.length; j++) {
> > +              curPath += destPathDirs[j] + "/";
> > +              try {
> > +                OS.File.makeDir(curPath, {unixMode: 0o755});
> > +              } catch(e) { }
> 
> This should probably actually handle errors. :-)

This is now handled by the main try/catch block in this function.

> @@ +47,5 @@
> > +            }
> > +            try {
> > +              let curFile = OS.File.open(destPath, {create: true});
> > +              curFile.close()
> > +            } catch(e) { }
> 
> Why do we need to do this? This doesn't write anything, but the next line
> (writeAtomic) does. Why do we open and then close the file?

You're correct, this isn't needed. An earlier patch of mine failed to create these files, but this must have been due to an error in directory creation. I've removed this.

> @@ +58,5 @@
> > +        });
> > +      }
> > +      postMessage({
> > +        "result": "success",
> > +        "paths": extractedPaths
> 
> I'm sure I'm being dumb, but it doesn't look like the fetch()s of the files
> and their processing blocks the loop, and therefore I expect this
> postMessage to happen irrespective of whether the files have been extracted
> or not.

No, you're correct. This is fixed now by using await.

> ::: toolkit/modules/GMPInstallManager.jsm
> @@ +382,5 @@
> >     *         See GMPInstallManager.installAddon for resolve/rejected info
> >     */
> >    install() {
> > +    let log = getScopedLogger("GMPExtractor");
> > +    this._deferred = Promise.defer();
> 
> Do we need to use Promise.defer()? I'm not super familiar with this code,
> but if we can just make install() an async function (which will make it
> return a promise) then that seems simpler.

I took a stab at this, but I couldn't get it to work. this._deferred is used all over this file and I would prefer if we could handle this in a separate bug.

Comment 15

[Stephen A Pohl [:spohl]]

(In reply to Stephen A Pohl [:spohl] from comment #14)
> Created attachment 8874660 [details] [diff] [review]
> Patch
> 
> (In reply to :Gijs from comment #12)
> > @@ +25,5 @@
> > > +        // We don't need these types of files.
> > > +        if (fileName == "verified_contents.json" ||
> > > +            fileName == "icon-128x128.png") {
> > > +          continue;
> > > +        }
> > 
> > Should we just skip everything with particular extensions?
> > `fileName.endsWith(".png") etc.
> > 
> > Wonder if we could/should just whitelist only the files we need.
> 
> Whitelisting isn't possible because GMPs may be released for Firefox
> versions after they ship. The filenames to skip are taken from existing
> code, minus the __MACOSX folder (since this patch doesn't currently support
> GMPs with subdirectories). I will need to check with someone if we ever
> expect to have GMPs with subdirectories at this time.

Chris, do you know if we ever expect to have GMPs with subdirectories? Is this something that we need to continue to support? None of our tests seem to exercise this. Thanks!

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Stephen A Pohl [:spohl] from comment #15)
> (In reply to Stephen A Pohl [:spohl] from comment #14)
> > Created attachment 8874660 [details] [diff] [review]
> > Patch
> > 
> > (In reply to :Gijs from comment #12)
> > > @@ +25,5 @@
> > > > +        // We don't need these types of files.
> > > > +        if (fileName == "verified_contents.json" ||
> > > > +            fileName == "icon-128x128.png") {
> > > > +          continue;
> > > > +        }
> > > 
> > > Should we just skip everything with particular extensions?
> > > `fileName.endsWith(".png") etc.
> > > 
> > > Wonder if we could/should just whitelist only the files we need.
> > 
> > Whitelisting isn't possible because GMPs may be released for Firefox
> > versions after they ship. The filenames to skip are taken from existing
> > code, minus the __MACOSX folder (since this patch doesn't currently support
> > GMPs with subdirectories). I will need to check with someone if we ever
> > expect to have GMPs with subdirectories at this time.
> 
> Chris, do you know if we ever expect to have GMPs with subdirectories? Is
> this something that we need to continue to support? None of our tests seem
> to exercise this. Thanks!

I do not expect we'll need GMPs to have subdirs. GMPs shouldn't be able to access the file system anyway.

Comment 17

[Stephen A Pohl [:spohl]]

(In reply to Chris Pearce (:cpearce) from comment #16)
> I do not expect we'll need GMPs to have subdirs. GMPs shouldn't be able to
> access the file system anyway.

To be clear, I'm referring to the GMP zip file that is being downloaded. Currently, if the zip file has subdirs, we extract the individual files into the same directory without subdirectories, i.e. we flatten it. Can you confirm that GMP zip files don't have subdirs, now or in the future? Thanks!

Comment 18

[Florian Quèze [:florian]]

Comment on attachment 8874660 [details] [diff] [review]
Patch

Review of attachment 8874660 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for working on this! I like the async/await version of the patch much better than the previous one :-). Here are a few trivial drive by comments:

::: toolkit/modules/GMPExtractorWorker.js
@@ +13,5 @@
> +    let jarResponse = await fetch(jarPath);
> +    let dirListing = await jarResponse.text();
> +    let lines = dirListing.split("\n");
> +    let encoder = new TextEncoder();
> +    for (let i = 0; i < lines.length; i++) {

nit: can be simplified to "for (let line of lines) {"

@@ +14,5 @@
> +    let dirListing = await jarResponse.text();
> +    let lines = dirListing.split("\n");
> +    let encoder = new TextEncoder();
> +    for (let i = 0; i < lines.length; i++) {
> +      if (lines[i].substr(0, 5) != "201: ") {

nit: if (!line.startsWith("201: ")) {

@@ +18,5 @@
> +      if (lines[i].substr(0, 5) != "201: ") {
> +        // Not a file entry, skip.
> +        continue;
> +      }
> +      let lineSplits = lines[i].substr(5).split(" ");

nit: I'm not a fan of the magic 5 constant, I would prefer "201: ".length

@@ +36,5 @@
> +      let destTempPath = destPath + ".tmp";
> +
> +      let destPathDirs = msg.data.installToDirPath.split("/");
> +      let curPath = "";
> +      for (let j = 0; j < destPathDirs.length; j++) {

for of?

Comment 19

[:Gijs (he/him)]

Comment on attachment 8874660 [details] [diff] [review]
Patch

Review of attachment 8874660 [details] [diff] [review]:
-----------------------------------------------------------------

This generally looks OK though I'd like to see this again using blobs. I also have one more general comment:

this serializes all IO - that is, we iterate and read, then write, each file in turn. I wonder if it'd be more efficient to either serialize all of the reads, followed by all of the writes, or even to do some of this concurrently (you could collect all the write promises and then await Promise.all(writePromiseArray), for instance). I don't feel strongly either way. It's effectively an IO scheduling problem and I don't have a lot of expertise to offer. Also not sure if the general number of files (which I think is low?) justifies over-thinking this particular aspect of this implementation.

::: toolkit/modules/GMPExtractorWorker.js
@@ +27,5 @@
> +        continue;
> +      }
> +      let filePath = jarPath + fileName;
> +      let filePathResponse = await fetch(filePath);
> +      let fileContents = await filePathResponse.text();

Re: blob and .text() - I think you can use a FileReader. See the example at https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/OSFile.jsm/OS.File_for_the_main_thread#Example_Save_Canvas_to_Disk - though I think given that this is a chrome worker, which is just an overprivileged web worker, I think you construct the file reader with "new FileReader()", rather than XPCOM. The FileReader should give you the fileData without needing an encoder. Just like the encoder, I expect you can reuse one FileReader object and just call readAsArrayBuffer and pass it each blob in turn. You'll need to wait for the operation to complete, probably something like:

let fileData = await new Promise(resolve => {
  reader.onloadend = resolve;
  reader.readAsArrayBuffer(blob);
});

given that FileReader doesn't seem to have a Promise-y API. :-\

::: toolkit/modules/GMPInstallManager.jsm
@@ +8,5 @@
>  
>  const {classes: Cc, interfaces: Ci, results: Cr, utils: Cu, manager: Cm} =
>    Components;
>  // 1 day default
> +const DEFAULT_SECONDS_BETWEEN_CHECKS = 1;//60 * 60 * 24;

Err, probably don't want to ship this. :-)

@@ +381,5 @@
>     * @return a promise which will be resolved or rejected
>     *         See GMPInstallManager.installAddon for resolve/rejected info
>     */
>    install() {
> +    let log = getScopedLogger("GMPExtractor");

Nit: stick this in the onmessage? :-)

@@ +404,3 @@
>      }
> +    worker.postMessage({
> +      "zipPath": this.zipPath,

In JS, dictionary keys don't need quotes if they're valid identifiers. Also, you already stuck `zipPath` into a local, and in an object literal of the form: {foo: foo}, you can omit the value (so shorten to {foo}), so you can just do:

worker.postMessage({zipPath, ... });

In fact, it might make sense to also put `installToDirPath` into a local variable too, and then this line can just be:

worker.postMessage({zipPath, installToDirPath});

Comment 20

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Stephen A Pohl [:spohl] from comment #17)
> (In reply to Chris Pearce (:cpearce) from comment #16)
> > I do not expect we'll need GMPs to have subdirs. GMPs shouldn't be able to
> > access the file system anyway.
> 
> To be clear, I'm referring to the GMP zip file that is being downloaded.
> Currently, if the zip file has subdirs, we extract the individual files into
> the same directory without subdirectories, i.e. we flatten it. Can you
> confirm that GMP zip files don't have subdirs, now or in the future? Thanks!

Currently GMP zip files don't have subdirs, and I don't expect they ever will.

Comment 21

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=485a3e88cee79b74a9e81df3584c97ec3e5a552d

Comment 22

[Stephen A Pohl [:spohl]]

Attachment: Patch

Try run in comment 21. Addressed all feedback from comment 18 and comment 19, with the exception of:

(In reply to :Gijs from comment #19)
> Comment on attachment 8874660 [details] [diff] [review]
> Patch
> 
> Review of attachment 8874660 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This generally looks OK though I'd like to see this again using blobs. I
> also have one more general comment:
> 
> this serializes all IO - that is, we iterate and read, then write, each file
> in turn. I wonder if it'd be more efficient to either serialize all of the
> reads, followed by all of the writes, or even to do some of this
> concurrently (you could collect all the write promises and then await
> Promise.all(writePromiseArray), for instance). I don't feel strongly either
> way. It's effectively an IO scheduling problem and I don't have a lot of
> expertise to offer. Also not sure if the general number of files (which I
> think is low?) justifies over-thinking this particular aspect of this
> implementation.

We currently download/install two GMPs, OpenH264 and Widevine, with 2 and 3 files, respectively. I don't think overthinking this is justified at this time.

Comment 23

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=62bfb661332599d87fb7d1174b106358ddd5341f

Comment 24

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee438b7dd9cce25d393f837dabcb24e8e4f441f5

Comment 25

[Stephen A Pohl [:spohl]]

Attachment: Patch

This fixes lint failures in the previous push. Confirmed green in the try push in comment 24.

Comment 26

[Stephen A Pohl [:spohl]]

Comment on attachment 8875907 [details] [diff] [review]
Patch

Looks like I missed the fact that xpcshell was failing on Windows all along. Will need to look into this on a Windows system.

Comment 27

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c36ae6caaefc4d8cef73c0ae697384d119ecc68f

Comment 28

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a8fb0ed3ea0b23cdb65008f7698384bcd15f5dc

Comment 29

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0da3603bf9f7ade72febbfecf7e1fb356813fa6a

Comment 30

[Stephen A Pohl [:spohl]]

Attachment: Patch

Comment 31

[Stephen A Pohl [:spohl]]

Comment on attachment 8876978 [details] [diff] [review]
Patch

Try run in comment 29 is green.

Comment 32

[:Gijs (he/him)]

Comment on attachment 8876978 [details] [diff] [review]
Patch

Review of attachment 8876978 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine, though I would prefer not to have to loop the dirs and create them one by one. Is there some reason my suggestion there doesn't work? Looking at it some more, it looks like the options listed for makeDir() on the OS.File documentation for the worker APIs and the main-thread APIs is different. I don't know if that's a documentation error or if they actually behave differently (which would be very confusing!).

::: toolkit/modules/GMPExtractorWorker.js
@@ +45,5 @@
> +      }
> +      for (let dir of destPathComponents.components) {
> +        curPath = OS.Path.join(curPath, dir);
> +        await OS.File.makeDir(curPath, {unixMode: 0o755});
> +      }

So, I continue to be confused by this. My understanding from:

https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/OSFile.jsm/OS.File_for_the_main_thread#OS.File.makeDir()

is that this will fail if the directory exists, unless you pass ignoreExisting: true.

My other suggestion would be to use the from: option to avoid the loop here. I think you can also avoid the OS.Path.split in that case?

::: toolkit/modules/GMPInstallManager.jsm
@@ +384,5 @@
>    install() {
> +    this._deferred = Promise.defer();
> +    let deferredPromise = this._deferred;
> +    let zipPath = this.zipPath;
> +    let installToDirPath = this.installToDirPath;

Nit: you can collapse these lines as:

let {zipPath, installToDirPath} = this;

Comment 33

[Stephen A Pohl [:spohl]]

Attachment: Patch

(In reply to :Gijs from comment #32)
> This looks fine, though I would prefer not to have to loop the dirs and
> create them one by one. Is there some reason my suggestion there doesn't
> work? Looking at it some more, it looks like the options listed for
> makeDir() on the OS.File documentation for the worker APIs and the
> main-thread APIs is different. I don't know if that's a documentation error
> or if they actually behave differently (which would be very confusing!).

You're correct that I was looking at the worker API documentation, which neither mentioned the |ignoreExisting| nor the |from| flag. It was hard for me to believe that we wouldn't have flags for this, but I couldn't find out what the right flags were.

I like this solution a lot more. I'm sending this back for a final review because I changed the GMPExtractorWorker to take a relative path instead of an absolute install path. This way, we can keep the logic about being relative to the user's profileDir in one place (since we need to specify profileDir for the |from| flag in our |OS.File.makeDir| call).

> ::: toolkit/modules/GMPExtractorWorker.js
> @@ +45,5 @@
> > +      }
> > +      for (let dir of destPathComponents.components) {
> > +        curPath = OS.Path.join(curPath, dir);
> > +        await OS.File.makeDir(curPath, {unixMode: 0o755});
> > +      }
> 
> So, I continue to be confused by this. My understanding from:
> 
> https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/
> OSFile.jsm/OS.File_for_the_main_thread#OS.File.makeDir()
> 
> is that this will fail if the directory exists, unless you pass
> ignoreExisting: true.

Confusingly, this neither failed nor threw an exception, despite what the documentation says.

> My other suggestion would be to use the from: option to avoid the loop here.
> I think you can also avoid the OS.Path.split in that case?

Done!

> ::: toolkit/modules/GMPInstallManager.jsm
> @@ +384,5 @@
> >    install() {
> > +    this._deferred = Promise.defer();
> > +    let deferredPromise = this._deferred;
> > +    let zipPath = this.zipPath;
> > +    let installToDirPath = this.installToDirPath;
> 
> Nit: you can collapse these lines as:
> 
> let {zipPath, installToDirPath} = this;

Done!

Comment 34

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f992d8104e200acfa7ebd7a56149119bc17d012

Comment 35

[:Gijs (he/him)]

Comment on attachment 8877200 [details] [diff] [review]
Patch

Review of attachment 8877200 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome! Thanks!

Comment 36

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a49846fa096f9296f5028e1901322639fafecb8
Bug 1149732: Avoid main-thread IO when installing GMP modules. r=Gijs

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8a49846fa096