1334550 - Proxy moz-extension protocol requests to the parent process

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, defect, P1)

Description

[Haik Aftandilian [:haik]]

When we start to apply process sandbox restrictions to the extension process, if we load moz-extension resources in the extension process, it will require the process to have filesystem read permissions. It would be better if we proxy those requests to the parent, or even better, if we proxy those requests to a file content process which (at this time) is a web content process with file read permissions.

Comment 1

[Kris Maglione [:kmag]]

I think we want to proxy the requests to the parent. I don't think we'd gain much by bringing a separate content process into the mix, and I don't think the extra overhead of keeping that process running, and the extra layer of proxying, would be acceptable.

Comment 2

[:shell escalante]

did you plan to work on this one?  or have someone else in mind

Comment 3

[Haik Aftandilian [:haik]]

(In reply to :shell escalante from comment #2)
> did you plan to work on this one?  or have someone else in mind

My expectation was that someone working on webextensions would pick it up. If nobody is available, let me know.

Comment 4

[Haik Aftandilian [:haik]]

(In reply to Haik Aftandilian [:haik] from comment #0)
> When we start to apply process sandbox restrictions to the extension
> process, if we load moz-extension resources in the extension process, it
> will require the process to have filesystem read permissions.

This also applies to the content process and could be a blocker for removing read access from content processes. Extensions can use moz-extension URI's to load resources from the content process. We can white list directories that could contain extensions at content process start time, but developers need to be able to load extensions from arbitrary locations.

Comment 5

[Haik Aftandilian [:haik]]

To address developers needing to install webextensions from arbitrary locations on the filesystem, rather than remoting moz-extension, could we instead modify the web-ext tool to copy/sync files to a white-listed directory?

Comment 6

[Kris Maglione [:kmag]]

(In reply to Haik Aftandilian [:haik] from comment #5)
> To address developers needing to install webextensions from arbitrary
> locations on the filesystem, rather than remoting moz-extension, could we
> instead modify the web-ext tool to copy/sync files to a white-listed
> directory?

No, this also applies to extensions installed via about:debugging, and the unpacked extensions installed that way are meant to be modified in-place. The amount of effort we'd have to put into scanning, copying, and cleaning up files to make that even mostly work as expected would probably be more work than just implementing the proxying to begin with.

Comment 7

[Andy McKay]

Currently unassigned, we need to find someone to take this.

Comment 8

[Haik Aftandilian [:haik]]

Attachment: Incomplete non-functional patch, posting for advice

Posting this not-working patch to get some feedback on the general approach. The patch introduces ExtensionChannelParent/Child to forward moz-extension protocol reads to the parent. I've been trying to hack this into a working state and it's not ready for review or code cleanliness feedback.

Prior to the patch, the ExtensionProtocolHandler is a SubstitutingProtocolHandler and is used in both parent and child. With the patch, that is still true, but the child instance of ExtensionProtocolHandler creates channels itself instead of using the substituting protocol logic, but is still a SubstitutingProtocolHandler. I think it would make sense to have a new _Child_ExtensionProtocolHandler that is not a SubstitutingProtocolHandler and handles moz-extension in the child only.

The patch doesn't work yet. With the extension I'm testing (https://github.com/mdn/webextensions-examples/tree/master/beastify), with the patch, a content-script JS file is read through the parent and ExtensionChannelChild is receiving the data and forwarding it on, but the content-script doesn't seem to be ever executed. I suspect I've done something wrong with the listeners/observers. Still debugging.

Comment 9

[Haik Aftandilian [:haik]]

Attachment: Incomplete patch, posting for advice

Updated the patch to fix the problem that was preventing the beastify extension from working. The OnStopRequest event wasn't being forwarded by ExtensionChannelChild. Now, the web accessible resources (all images in this case) are opened by the parent and not the content process. Among other things still needing to be addressed, the patch remotes all opens as asynchronous opens and that will have to be fixed to do synchronous opens for some .css files like ExtensionProtocolHandler::SubstituteChannel.

Comment 10

[Haik Aftandilian [:haik]]

After getting some feedback via email regarding the synchronous open requirement, the approach I'm working on now is as follows.

Treat packed and unpacked extensions differently.

For a packed extension (.xpi file), the parent will send a FileDescriptor to the child for the .xpi file when the extension is loaded. The child will load moz-extension: resources from the .xpi using sync or async methods as needed. (Jason Duell pointed out that we used to have support for doing this for B2G Apps via a PRemoteOpenFile protocol which was removed in the fix for 1307467 and I'm look at that as a reference.)

For an unpacked extension, when a moz-extension: resource is loaded, the child will send a message to the parent requesting a FileDescriptor for the resource. The parent will send back the FileDescriptor. In some cases, this will be required to be a sync message. So both a sync and async version of the message will be needed.

Comment 11

[Haik Aftandilian [:haik]]

Attachment: In progress patch for remoting moz-extension protocol requests, not quite review-ready

Posting my work-in-progress patch that needs a bit more work before review.

With the fix for bug 1348442, we don't need to load moz-extension file resources synchronously which means we don't need to request an FD from the parent with a sync message. The two types of remote moz-extension loads we need to handle are 1) when the extension is in a JAR or XPI file (aka packed extension) and 2) when the extension is unpacked.

For packed extensions, when the extension is loaded, the JAR file is opened and its file descriptor is sent to the child. ExtensionProtocolHandler has been changed to override SubstitutingProtocolHandler::SetSubstitution and when a when a host->JAR mapping is being set and sent to the child, it opens the JAR and sends the file descriptor. (For now, the file URL is still sent, but isn't needed by the child.) The child ExtensionProtocolHandler caches host->(file descriptor) mappings. In the child, ExtensionProtocolHandler::NewChannel2 has been changed to fix up JarChannels by using a new nsIFile implementation FileDescriptorFile. FileDescriptorFile and this approach is mostly copied from the RemoteOpenFile class that used to be in the tree for B2G App JAR handling.

For unpacked loads, when ExtensionProtocolHandler resolves a moz-extension URI to a file:// URI, an asynchronous request is sent to the parent for an nsIInputStream for the file. ExtensionProtocolHandler::NewChannel2 creates an nsIInputStreamChannel which wraps a new class nsDeferredInputStream which implements nsAsyncInputStream. nsDeferredInputStream waits for the parent to send back an input stream and returns NS_BASE_STREAM_WOULD_BLOCK in the meantime. Once the stream is received from the parent, nsDeferredInputStream wraps it in a nsIBufferedInputStream and redirects read calls to it.

For unpacked loads, the input stream is not cached so each moz-extension load generates a request to the parent for a stream. This should be straightforward to fix in a followup fix.

Besides more testing, I'm working on a pref to disable the remoting in case of problems on Nightly, improving code comments, and cleaning up an FD leak. And I have some questions on the threading model, specifically what is guaranteed to be executed on the main thread and what isn't.

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

Comment 12

[Kris Maglione [:kmag]]

(In reply to Haik Aftandilian [:haik] from comment #11)
> For unpacked loads, when ExtensionProtocolHandler resolves a moz-extension
> URI to a file:// URI, an asynchronous request is sent to the parent for an
> nsIInputStream for the file. ExtensionProtocolHandler::NewChannel2 creates
> an nsIInputStreamChannel which wraps a new class nsDeferredInputStream which
> implements nsAsyncInputStream. nsDeferredInputStream waits for the parent to
> send back an input stream and returns NS_BASE_STREAM_WOULD_BLOCK in the
> meantime. Once the stream is received from the parent, nsDeferredInputStream
> wraps it in a nsIBufferedInputStream and redirects read calls to it.

Once bug 1351163 is fixed, this shouldn't be necessary. At that point, we'll
be given a stream listener when AsyncOpen2 is called, and we can just wait for
the stream to become available before we begin pumping data to it.

Comment 13

[Haik Aftandilian [:haik]]

Attachment: Patch for remoting moz-extension protocol requests, need some feedback

Posting my updated patch for remoting moz-extension URI loads.

With this version, moz-extension loads are handled in a similar way for both packed and unpacked WebExtensions. In both cases, we asynchronously query the parent for when the resource is loaded. For a packed extension, we request an FD for the underlying JAR file which is then used to create a FileDescriptorFile in the child which is "sideloaded" into the JAR channel. For unpacked extensions, we request an AutoIPCStream from the parent which is used as the input stream in the child.

Instead of the DeferredInputStream class added in the previous patch, I'm using the new SimpleChannel added in bug 1351163 to trigger the remote request when channel is opened.

A couple of questions on how I'm using SimpleChannel for Kris:

1) With my async callback, when the parent fails to send back a valid stream or descriptor, I'm calling SimpleChannel->Cancel(), but this doesn't cleanly cancel the request. Still debugging this, but it may be because the channel's mRequest is NULL when Cancel() is called. Any recommendation on how the cancellation should be handled?

2) I have an ExtensionStreamGetter class used as the context SimpleChannel callback argument that ends up leaking. How should the lifetime of the context object be handled?

Comment 14

[Haik Aftandilian [:haik]]

Comment on attachment 8858952 [details] [diff] [review]
Patch for remoting moz-extension protocol requests, need some feedback

Resolved the issues I was having. Posting patch for review shortly.

Comment 15

[Haik Aftandilian [:haik]]

Attachment: Bug 1334550 - Part 3 - Open extension JAR files off the main thread;

Changes ExtensionProtocolHandler to use remote streams for
moz-extension loads of file and JAR URI's to allow for
filesystem read sandboxing.

Adds messaging to PNecko to allow child processes to
request an input stream or file descriptor for moz-extension
URI's.

Adds FileDescriptorFile, a limited implementation of nsIFile
that wraps a file descriptor, to be sideloaded into
nsJARChannels so that extension JAR files can be read
using a file descriptor without accessing the filesystem
directly.

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

Comment 16

[Haik Aftandilian [:haik]]

Attachment: Bug 1334550 - Part 2 - Add nsISubstitutionObserver and update test_extensionURL.html;

Adds nsISubstitutionObserver so that substitutions set on a
parent nsISubstitutingProtocolHandler which are then propagated
to child processes can be observed in the child.

Updates test_extensionURL.html to set substitutions on the parent
ExtensionProtocolHandler before trying to load moz-extension URI's
using those substitutions.

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

Comment 17

[Haik Aftandilian [:haik]]

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

Comment 18

[Haik Aftandilian [:haik]]

Security implications of the patch:

With the fix, the parent exposes two new IPDL interfaces to the child process:
  NeckoParent::RecvAsyncGetExtensionResourceStream,
  NeckoParent::RecvAsyncGetExtensionResourceFD
  
Both endpoints accept a moz-extension URI for what is expected to be a web-accessible resource from the extension's directory or JAR file. The intent is to only allow the content process to load a moz-extension URI for a resource that is within the directory or JAR file of an extension enabled at the time of the request.

RecvAsyncGetExtensionResourceStream is used for moz-extension loads of unpacked moz-extension resources. It sends back an input stream to the child if the following are all true.

  1) the scheme of the requested URI is moz-extension
  2) the parent ExtensionProtocolHandler has a substition
     mapping the host of the requested URI to a file URL
  3) the mapped file URL is a directory
  4) GetProtocolHandler("moz-extension").NewChannel2(requested URI)
     returns a fileChannel
  5) the fileChannel's file is located within the directory
     from 3--this is tested using nsIFile::Normalize and
     nsIFile::Contains()
     
RecvAsyncGetExtensionResourceFD is used for moz-extension loads of packed (JAR file) moz-extension resources. It sends back a FileDescriptor that refers to the extension JAR file to the child if the following are all true.

  1) the scheme of the requested URI is moz-extension
  2) the parent ExtensionProtocolHandler has a substition
     mapping the host of the requested URI to a JAR URI

Comment 19

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8862184 [details]
Bug 1334550 - Part 3 - Open extension JAR files off the main thread;

https://reviewboard.mozilla.org/r/115848/#review138064

::: ipc/glue/FileDescriptorUtils.cpp:138
(Diff revision 1)
> +  FileDescriptor::PlatformHandleType newPlatformHandle;
> +#ifdef XP_WIN
> +  if (!::DuplicateHandle(GetCurrentProcess(), platformHandle,
> +                         GetCurrentProcess(), &newPlatformHandle,
> +                         0, FALSE, DUPLICATE_SAME_ACCESS)) {
> +    return nullptr;
> +  }
> +#else // XP_WIN
> +  newPlatformHandle = dup(platformHandle);
> +  if (newPlatformHandle == -1) {
> +    return nullptr;
> +  }
> +#endif

This code is mostly the same as `FileDescriptor::Clone`, can they be de-duplicated somehow?

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:149
(Diff revision 1)
> +  listener.swap(mListener);
> +
> +  MOZ_ASSERT(mChannel);
> +  nsresult rv;
> +
> +  nsCOMPtr<nsIInputStream> stream = aStream;

Beginer question, why not just make `aStream` be an `nsCOMPtr`?

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:651
(Diff revision 1)
> +  nsCOMPtr<nsIChannel> channel = NS_NewSimpleChannel(
> +    aURI, aLoadinfo, streamGetter.get(),
> +    [] (nsIStreamListener* listener, nsIChannel* channel,
> +        ExtensionStreamGetter* getter) -> RequestOrReason {
> +      if (getter->GetAsync(listener, channel)) {
> +        return RequestOrReason(NS_OK);
> +      } else {
> +        return RequestOrReason(NS_ERROR_UNEXPECTED);
> +      }
> +    });

This is exactly the same as in the method above (there might be bother bits from this method as well that are duplicated). Can it be factored out?

Comment 20

[Haik Aftandilian [:haik]]

Comment on attachment 8862184 [details]
Bug 1334550 - Part 3 - Open extension JAR files off the main thread;

https://reviewboard.mozilla.org/r/115848/#review138064

> This code is mostly the same as `FileDescriptor::Clone`, can they be de-duplicated somehow?

Looks like it could. I'll try refactoring so the code uses FileDescriptor::Clone.

> Beginer question, why not just make `aStream` be an `nsCOMPtr`?

Good question. I thought using nsCOMPtr as an argument was not recommended. I don't see anything in the docs about it. Will look into it some more.

> This is exactly the same as in the method above (there might be bother bits from this method as well that are duplicated). Can it be factored out?

Yes, thanks. I'll apply some de-duping here. That function doesn't need to be a lambda.

Comment 21

[Haik Aftandilian [:haik]]

Comment on attachment 8862184 [details]
Bug 1334550 - Part 3 - Open extension JAR files off the main thread;

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

Comment 22

[Haik Aftandilian [:haik]]

Comment on attachment 8862185 [details]
Bug 1334550 - Part 2 - Add nsISubstitutionObserver and update test_extensionURL.html;

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

Comment 23

[Haik Aftandilian [:haik]]

Comment on attachment 8862184 [details]
Bug 1334550 - Part 3 - Open extension JAR files off the main thread;

https://reviewboard.mozilla.org/r/115848/#review138064

> Looks like it could. I'll try refactoring so the code uses FileDescriptor::Clone.

Added a new static method to FileDescriptor to allow for cloning of the platform handle without instantiating a FileDescriptor object.

> Good question. I thought using nsCOMPtr as an argument was not recommended. I don't see anything in the docs about it. Will look into it some more.

From what I can tell, there's no reason not to use a reference to an nsCOMPtr as an argument. Changed OnStream to take a reference to an nsCOMPtr<nsIInputStream>.

Comment 24

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8862184 [details]
Bug 1334550 - Part 3 - Open extension JAR files off the main thread;

https://reviewboard.mozilla.org/r/115848/#review138514

Just a few more small comments, I'm still learning my way around this code.

For things like this, do we generally add tests, or just rely on the existing tests for extensions?

::: modules/libjar/nsJARChannel.cpp:900
(Diff revision 2)
> +    if (mOpened) {
> +        return NS_ERROR_IN_PROGRESS;
> +    } else if (!aFile) {
> +        return NS_ERROR_INVALID_ARG;
> +    } else {
> +        mJarFileOverride = aFile;

Why does this need to assign to `mJarFileOverride` instead of just assigning directly to `mJarFile`?

::: modules/libpref/init/all.js:4797
(Diff revision 2)
>  // Whether or not webextension themes are supported.
>  pref("extensions.webextensions.themes.enabled", false);
>  pref("extensions.webextensions.themes.icons.enabled", false);
>  pref("extensions.webextensions.remote", false);
> +// Whether or not the moz-extension resource loads are remoted
> +pref("extensions.webextensions.protocol.remote", true);

Is this temporary, or do we plan to support this indefinitely?

Comment 25

[Haik Aftandilian [:haik]]

Comment on attachment 8862184 [details]
Bug 1334550 - Part 3 - Open extension JAR files off the main thread;

https://reviewboard.mozilla.org/r/115848/#review138514

We just have to make sure the important code paths get executed in tests one way or another. So if the existing tests aren't adequate we add new tests to either sandboxing or extension tests. If the test requires lots of supporting code from the extensions tests, I would try to include it there. If it's more standalone or low level, I think it could go in sandboxing specific tests. Others might have stronger opinions.

In this case, I've found the WebExtensions tests we have do exercise this code pretty well, including the failure path when the parent does not return an FD or stream for the URI. So I think the tests we have are acceptable for the remoting.

However, I'd like to add some more tests that are targeted at how the parent handles URI's that try to break out of the extension dir or JAR. I'll file another bug for that. Of course, any feedback on that is welcome.

> Why does this need to assign to `mJarFileOverride` instead of just assigning directly to `mJarFile`?

That's so that we can check whether or not we're using the "side-loaded" file in nsJARChannel::LookupFile(). I took this approach in an attempt to minimize the changes to nsJARChannel because I considered the JAR loading pretty complex and therefore high risk to change. But that reminds me, I plan to file a new bug on nsJARChannel requesting it support being created from an input stream or file descriptor, making FileDescriptorFile unnecessary. Will do that and link to the bug.

> Is this temporary, or do we plan to support this indefinitely?

It's intended to be used for debugging on Nightly after this lands, in case there is unexpected breakage. If the remoting somehow blows up the world, it can be quickly disabled. Once we land our read-access restrictions, setting this to false will not work unless the sandbox level is also lowered.

As long as we allow setting security.sandbox.content.level on Nightly to disable sandbox settings, this pref could be useful for debugging. So its expected lifetime is approximately the same as security.sandbox.content.level and I'd hope this can be removed once our sandbox is stabilized.

Comment 26

[Haik Aftandilian [:haik]]

Comment on attachment 8862184 [details]
Bug 1334550 - Part 3 - Open extension JAR files off the main thread;

https://reviewboard.mozilla.org/r/115848/#review138514

Comment 27

[Haik Aftandilian [:haik]]

Jason, could you review this from the Necko perspective, or recommend a reviewer?

Comment 28

[Haik Aftandilian [:haik]]

Comment on attachment 8862184 [details]
Bug 1334550 - Part 3 - Open extension JAR files off the main thread;

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

Comment 29

[Haik Aftandilian [:haik]]

Comment on attachment 8862185 [details]
Bug 1334550 - Part 2 - Add nsISubstitutionObserver and update test_extensionURL.html;

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

Comment 30

[Jason Duell]

Comment on attachment 8862184 [details]
Bug 1334550 - Part 3 - Open extension JAR files off the main thread;

I'd like Honza to look over this and make sure the necko bits at least make sense.  Honza, if you manage to review it all, you can remove r? for kmag.  Otherwise hand this off to him after you've looked at the necko bits.

Comment 31

[Honza Bambas (:mayhemer)]

Comment on attachment 8862184 [details]
Bug 1334550 - Part 3 - Open extension JAR files off the main thread;

https://reviewboard.mozilla.org/r/115848/#review141566

I am not sure why I should review this exactly, but I suspect a lot of code is duplicated here.  We already have a toolkit for sending streams and fd's open on parent to child processes.  ask :baku and check on bug 1353629 for details.  blob and remote stream/file opening is being updated/refactored as part of that bug.

I can't review the mozprotocol handler part since I don't know that at all.

::: ipc/glue/FileDescriptorUtils.h:64
(Diff revision 3)
> +PRFileDesc* ClonePRFileDesc(PRFileDesc *aFileDesc);
> +
> +// Converts a FileDescriptor to a PRFileDesc. When called more than
> +// once for the same FileDescriptor object, returns a new PRFileDesc
> +// each time.
> +PRFileDesc* FileDescriptorToPRFileDesc(const FileDescriptor& aDesc);

we already have a code for cloning file handles.

::: modules/libjar/nsJARChannel.cpp:351
(Diff revision 3)
>      // have e.g. spaces in their filenames.
>      NS_UnescapeURL(mJarEntry);
>  
> +    if (mJarFileOverride) {
> +      mJarFile = mJarFileOverride;
> +      return NS_OK;

log this

::: modules/libjar/nsJARChannel.cpp:897
(Diff revision 3)
>  NS_IMETHODIMP
> +nsJARChannel::SetJarFile(nsIFile *aFile)
> +{
> +    if (mOpened) {
> +        return NS_ERROR_IN_PROGRESS;
> +    } else if (!aFile) {

no else after return

::: modules/libjar/nsJARChannel.cpp:901
(Diff revision 3)
> +        return NS_ERROR_IN_PROGRESS;
> +    } else if (!aFile) {
> +        return NS_ERROR_INVALID_ARG;
> +    } else {
> +        mJarFileOverride = aFile;
> +        return NS_OK;

warning: not all control paths return a result

::: netwerk/ipc/NeckoChild.h:138
(Diff revision 3)
> +                                 const FileDescriptor& fd) override;
> +
> +private:
> +  nsClassHashtable<nsUint64HashKey, NeckoChildExtensionCallbacks>
> +    mExtensionPendingCallbacks;
> +  uint64_t mExtensionMsgID;

all new members must have good comments what they are!

::: netwerk/ipc/NeckoChild.cpp:478
(Diff revision 3)
> +{
> +  uint64_t msgID = mExtensionMsgID++;
> +  if (SendAsyncGetExtensionResourceStream(msgID, uri, loadInfo)) {
> +    mExtensionPendingCallbacks.Put(msgID, aCallbacks);
> +    return true;
> +  } else {

again, else after return, seeing this quite often in the new code

::: netwerk/ipc/NeckoParent.cpp:1049
(Diff revision 3)
> +  rv = LoadInfoArgsToLoadInfo(aLoadInfo, getter_AddRefs(deserializedLoadInfo));
> +  if (NS_FAILED(rv)) {
> +    return IPC_FAIL_NO_REASON(this);
> +  }
> +
> +  // Ask the ExtensionProtocolHandler to give us a new input stream for

is it really the correct comment?

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:416
(Diff revision 3)
> +   * Make sure there is a substitution installed for the host found
> +   * in the child's request URI and make sure the host resolves to
> +   * a directory.
> +   */
> +
> +  // Get the host string from the child's request URI

please don't add comments like these, say why you do stuff, and not what you obviously do

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:469
(Diff revision 3)
> +
> +  /*
> +   * Make sure the file we resolved to is within the extension directory.
> +   */
> +
> +  // Normalize paths for sane comparisons

yes, everyone can see you are doing just that clearly from the code.  but important is to say why you are doing it?

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:486
(Diff revision 3)
> +    return NS_ERROR_FILE_ACCESS_DENIED;
> +  }
> +
> +  // Get and return input stream
> +  nsCOMPtr<nsIInputStream> inputStream;
> +  rv = channel->Open2(getter_AddRefs(inputStream));

never use this API!!

if you want a stream, then you can open a file input stream directly using the file with the lazy-open flag.  I don't see a reason to use a channel to obtain it, since it will likely cause main thread jank (IO).

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:545
(Diff revision 3)
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // Ensure this is a JAR URI
> +  // Example: jar:file:///extension.xpi!/
> +  nsCOMPtr<nsIJARURI> jarURI = do_QueryInterface(subURI, &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);

so, if the mapping is to something else than jar: this will throw NS_ERROR_NO_INTERFACE.  that is not much saying.  but I'm not sure what better error to throw here...

Comment 32

[Honza Bambas (:mayhemer)]

Comment on attachment 8862184 [details]
Bug 1334550 - Part 3 - Open extension JAR files off the main thread;

:baku, can you please take a look at this patch, mainly ipc and xpcom changes, and potentially suggest what of the existing blob/remote stream code should be used instead?  I have a strong feeling a wheel is being reinvented here.

thanks.

Comment 33

[Honza Bambas (:mayhemer)]

Comment on attachment 8862185 [details]
Bug 1334550 - Part 2 - Add nsISubstitutionObserver and update test_extensionURL.html;

https://reviewboard.mozilla.org/r/115850/#review141628

can't review the test changes.

the existing SubstitutingProtocolHandler::SendSubstitution method already sends the changes to the child.

only use of the new interfaces by the test, is it intended to be used for something else as well?

::: netwerk/protocol/res/SubstitutingProtocolHandler.h:12
(Diff revision 3)
>  #ifndef SubstitutingProtocolHandler_h___
>  #define SubstitutingProtocolHandler_h___
>  
>  #include "nsISubstitutingProtocolHandler.h"
>  
> +#include "nsCOMArray.h"

don't use this class for any new code

::: netwerk/protocol/res/SubstitutingProtocolHandler.h:17
(Diff revision 3)
> +#include "nsCOMArray.h"
>  #include "nsInterfaceHashtable.h"
>  #include "nsIOService.h"
> +#include "nsISubstitutionObserver.h"
>  #include "nsStandardURL.h"
> +#include "nsTHashtable.h"

why?

::: netwerk/protocol/res/SubstitutingProtocolHandler.h:79
(Diff revision 3)
>    }
>  
>    nsIIOService* IOService() { return mIOService; }
>  
>  private:
> +  void NotifyObservers(const nsACString& aRoot, nsIURI* aBaseURI);

comment what this is doing

::: netwerk/protocol/res/SubstitutingProtocolHandler.h:84
(Diff revision 3)
> +  void NotifyObservers(const nsACString& aRoot, nsIURI* aBaseURI);
> +
>    nsCString mScheme;
>    Maybe<uint32_t> mFlags;
>    nsInterfaceHashtable<nsCStringHashKey,nsIURI> mSubstitutions;
> +  nsInterfaceHashtable<nsISupportsHashKey,nsISubstitutionObserver> mObservers;

space after ,

comment what his is, who is the key, who is the observer, lifetime of both, how are used etc...

::: netwerk/protocol/res/SubstitutingProtocolHandler.cpp:416
(Diff revision 3)
>    }
>    return rv;
>  }
>  
> +nsresult
> +SubstitutingProtocolHandler::AddObserver(nsISubstitutionObserver* aObserver)

where are these declared?

::: netwerk/protocol/res/SubstitutingProtocolHandler.cpp:419
(Diff revision 3)
>  
> +nsresult
> +SubstitutingProtocolHandler::AddObserver(nsISubstitutionObserver* aObserver)
> +{
> +  NS_ENSURE_ARG(aObserver);
> +  mObservers.Put(aObserver, aObserver);

ok, use nsTArray<nsCOMPtr<nsISubstitutionObserver>> for mObservers

::: netwerk/protocol/res/nsISubstitutingProtocolHandler.idl:51
(Diff revision 3)
>     */
>    [must_use] AUTF8String resolveURI(in nsIURI resURI);
> +
> +  [must_use] void addObserver(in nsISubstitutionObserver observer);
> +
> +  [must_use] void removeObserver(in nsISubstitutionObserver observer);

comments...

::: netwerk/protocol/res/nsISubstitutionObserver.idl:11
(Diff revision 3)
> +#include "nsISupports.idl"
> +
> +interface nsIURI;
> +
> +/**
> + * nsISubstitutionObserver

some richer comment please - what is this used for, what class consumes that, what is the lifetime

::: netwerk/protocol/res/nsISubstitutionObserver.idl:22
(Diff revision 3)
> +     * Called when a new substition has been set.
> +     *
> +     * @param aRoot the root key of the mapping
> +     * @param aBaseURI the base URI which aRoot maps to
> +     */
> +    void onSet(in ACString aRoot, in nsIURI aBaseURI);

a more descriptive name of the method please.  "onProtocolHandlerSubstitutionSet" or something?

on what thread is this called?

::: netwerk/protocol/res/nsResProtocolHandler.h:55
(Diff revision 3)
>          return mozilla::SubstitutingProtocolHandler::ResolveURI(aResURI, aResult);
>      }
>  
> +    NS_IMETHOD AddObserver(nsISubstitutionObserver *aObserver) override
> +    {
> +        return mozilla::SubstitutingProtocolHandler::AddObserver(aObserver);

weird.  those are not static.

Comment 34

[Haik Aftandilian [:haik]]

Comment on attachment 8862184 [details]
Bug 1334550 - Part 3 - Open extension JAR files off the main thread;

https://reviewboard.mozilla.org/r/115848/#review141566

Thanks. The idea was that we need someone with Necko background as a reviewer in addition to WebExtensions review coverage.

We're using IPCStream for the unpacked extension case, but not the FD JAR file case. I'll look into changing that, but we'll just be pulling out the FD from the IPCStream in the child so it probably doesn't make sense.

> we already have a code for cloning file handles.

I wanted to clone the PRFileDesc without needlessly instantiating a FileDescriptor object and performing extra descriptor dup calls.

> no else after return

Sorry, I did not know that no-else-after-return was a Mozilla coding standard. I'll change these to be standards compliant.

> warning: not all control paths return a result

I think you misread the code.

> is it really the correct comment?

I'll clarify that the _parent_ ExtensionProtocolHandler is reponsbile for validating the request.

Is there something specific you thought was wrong with the comment?

> please don't add comments like these, say why you do stuff, and not what you obviously do

Sure. I admit I was overly verbose here.

> yes, everyone can see you are doing just that clearly from the code.  but important is to say why you are doing it?

I'll clarify the comment. The "for sane comparisons" was meant to explain how nsIFile::Contains() requires Normalize to be called to do reliable path checks.

> never use this API!!
> 
> if you want a stream, then you can open a file input stream directly using the file with the lazy-open flag.  I don't see a reason to use a channel to obtain it, since it will likely cause main thread jank (IO).

I'll give that a try.

Comment 35

[Andrea Marchesini [:baku]]

Comment on attachment 8862184 [details]
Bug 1334550 - Part 3 - Open extension JAR files off the main thread;

https://reviewboard.mozilla.org/r/115848/#review142026

Instead doing this what about if you use the existing Blob code.

1. We already have a File::CreateFromNsFile(nsIFile* aFile) and File::CreateFromFileName(const nsAString&); We can extend them and add File::CreateFromJARFile(...)
2. when the promise is resolved you have a DOM File object. Just do: GetInternalStream() and: NS_NewInputStreamChannelInternal().
3. just because you need to call  NS_NewInputStreamChannelInternal() immediately, create a new nsIAsyncInputStream, and when the promise is resolved, call the AsyncWait() callback.
4. On the parent side, you need to obtain a nsJARInputStream, create a StreamBlobImpl with that stream and resolve the CreateFromJARFile() promise.

No FileDescriptor involved.

Comment 36

[Honza Bambas (:mayhemer)]

(In reply to Andrea Marchesini [:baku] from comment #35)

Thanks baku for these details!

Comment 37

[Haik Aftandilian [:haik]]

Comment on attachment 8862184 [details]
Bug 1334550 - Part 3 - Open extension JAR files off the main thread;

https://reviewboard.mozilla.org/r/115848/#review141566

> I'll give that a try.

But could you explain why that API should never be used? I understand the point about doing a synchronous open on the main thread.

Comment 38

[Haik Aftandilian [:haik]]

Comment on attachment 8862185 [details]
Bug 1334550 - Part 2 - Add nsISubstitutionObserver and update test_extensionURL.html;

https://reviewboard.mozilla.org/r/115850/#review141628

> only use of the new interfaces by the test, is it intended to be used for something else as well?

No, it's only use is in testing. I'm definitely open to a better approach.

Yes, SubstitutingProtocolHandler::SendSubstitutions forwards substitutions from parent to child, but I needed the test code running in the child to know when a substitution has been installed in the parent.

The test installs a substitution in the parent and needs to wait until the substitution is propagated to the child before it can attempt to load the substitution. With these changes, setting a moz-extension substitution in the child will not be sufficient to successfully load the substituted URI because a child process is not given access unless the substitution is installed in the parent.

> don't use this class for any new code

Leftover cruft. Will remove. Sorry.

> why?

Leftover cruft. Will remove.

> comment what this is doing

Added comment.

> space after ,
> 
> comment what his is, who is the key, who is the observer, lifetime of both, how are used etc...

Changed to use nsTArray.

> where are these declared?

Via NS_DECL_NON_VIRTUAL_NSISUBSTITUTINGPROTOCOLHANDLER in the class definition.

> ok, use nsTArray<nsCOMPtr<nsISubstitutionObserver>> for mObservers

Done.

> comments...

Added

> some richer comment please - what is this used for, what class consumes that, what is the lifetime

Added comments here and in nsISubstitutingProtocolHandler.idl.

> a more descriptive name of the method please.  "onProtocolHandlerSubstitutionSet" or something?
> 
> on what thread is this called?

Renamed method to onSetSubstitution and added more comments.

> on what thread is this called?

Only on the main thread because substitutions are only added/removed from the main thread.

> weird.  those are not static.

Not sure what you mean here.

Comment 39

[Haik Aftandilian [:haik]]

Comment on attachment 8862184 [details]
Bug 1334550 - Part 3 - Open extension JAR files off the main thread;

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

Comment 40

[Haik Aftandilian [:haik]]

Comment on attachment 8862185 [details]
Bug 1334550 - Part 2 - Add nsISubstitutionObserver and update test_extensionURL.html;

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

Comment 41

[Haik Aftandilian [:haik]]

Comment on attachment 8862184 [details]
Bug 1334550 - Part 3 - Open extension JAR files off the main thread;

https://reviewboard.mozilla.org/r/115848/#review142026

I'll work on this and then reflag for review.

Comment 42

[Haik Aftandilian [:haik]]

(In reply to Andrea Marchesini [:baku] from comment #35)
> Comment on attachment 8862184 [details]
> Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent
> process;
> 
> https://reviewboard.mozilla.org/r/115848/#review142026
> 
> Instead doing this what about if you use the existing Blob code.
> 
> 1. We already have a File::CreateFromNsFile(nsIFile* aFile) and
> File::CreateFromFileName(const nsAString&); We can extend them and add
> File::CreateFromJARFile(...)
> 2. when the promise is resolved you have a DOM File object. Just do:
> GetInternalStream() and: NS_NewInputStreamChannelInternal().
> 3. just because you need to call  NS_NewInputStreamChannelInternal()
> immediately, create a new nsIAsyncInputStream, and when the promise is
> resolved, call the AsyncWait() callback.
> 4. On the parent side, you need to obtain a nsJARInputStream, create a
> StreamBlobImpl with that stream and resolve the CreateFromJARFile() promise.
> 
> No FileDescriptor involved.

The advantage of sending an FD for the JAR file is that it means the child process definitely can't use the FD to read anything outside of the JAR file.

If the parent sends back an nsJARInputStream, there's a risk that the parent could parse the URI received from the child and inadvertently (due to a bug) return a stream for a file outside of the JAR which would be a read-access sandbox escape. Also, wouldn't this require sending the JAR's content over IPC? And I think this would prevent caching in the child. With the current approach, we just extract the host section from the URI and don't depend on parsing the full URI. I'm concerned it might be possible to construct a URI that gets resolved in the parent to something outside of the JAR.

Comment 43

[Andrea Marchesini [:baku]]

> The advantage of sending an FD for the JAR file is that it means the child
> process definitely can't use the FD to read anything outside of the JAR file.

Oh yes. We do send the FD with IPCBlobInputStream but only when necko (or any other gecko component) needs it for real.
Basically we send the FD only when AsyncWait() is called. At that point, we serialize the original inputStream and we send it back to the child process. In this case, it will be a nsFileInputStream. A better description is here: https://dxr.mozilla.org/mozilla-central/source/dom/file/ipc/IPCBlobUtils.h#12

What I'm suggesting is to use existing code to retrieve an inputStream with a FD in it. You can also use Blob::CreateFromNsFile() to retrieve a Blob for the full JAR file. The data is not sent, and you have a IPCBlobInputStream, that, eventually (after a AsyncWait()) becomes a simple wrapper of a nsFileInputStream. Data is not send across processes.

I don't want to push too much, we should investigate if this approach is doable and the pros/cons compared with your solution.

Comment 44

[Haik Aftandilian [:haik]]

(In reply to Andrea Marchesini [:baku] from comment #43)
> What I'm suggesting is to use existing code to retrieve an inputStream with
> a FD in it. You can also use Blob::CreateFromNsFile() to retrieve a Blob for
> the full JAR file. The data is not sent, and you have a IPCBlobInputStream,
> that, eventually (after a AsyncWait()) becomes a simple wrapper of a
> nsFileInputStream. Data is not send across processes.
> 
> I don't want to push too much, we should investigate if this approach is
> doable and the pros/cons compared with your solution.

I discussed this with baku on IRC.

Using IPCBlobInputStream would be nice because we could use a similar technique to File::CreateFromNsIFile() and do the file open off the main thread, but the child still needs an FD and that isn't exposed by IPCBlobInputStream in the child. However, fixing bug 1364151 would allow us to use IPCBlobInputStream in the child without needing access to the FD. I don't want to try and tackle that with this fix.

I'd like to continue to work on getting the current approach landed.

In a follow up I can land a patch that does the file open in the parent off the main thread. Either by fixing 1364151 and using IPCBlobInputStream or simply using a short-lived thread in the parent to do the file open off the main thread.

Comment 45

[Haik Aftandilian [:haik]]

Honza, I tried reflagging you for review, but reviewboard isn't letting me do that. Could you take a look at my updates/questions?

Comment 46

[Andy McKay]

Just marking this down to P2, since we don't consider this a blocker for WebExtensions for turning this on. It is a blocker for content sandboxing, but not for out of process.

Comment 47

[Haik Aftandilian [:haik]]

Comment on attachment 8862184 [details]
Bug 1334550 - Part 3 - Open extension JAR files off the main thread;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/115848/diff/4-5/

Comment 48

[Haik Aftandilian [:haik]]

Attachment: Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent process;

Code review feedback, round 1

Changes ExtensionProtocolHandler to use remote streams for
moz-extension loads of file and JAR URI's to allow for
filesystem read sandboxing.

Adds messaging to PNecko to allow child processes to
request an input stream or file descriptor for moz-extension
URI's.

Adds FileDescriptorFile, a limited implementation of nsIFile
that wraps a file descriptor, to be sideloaded into
nsJARChannels so that extension JAR files can be read
using a file descriptor without accessing the filesystem
directly.

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

Comment 49

[Haik Aftandilian [:haik]]

Attachment: Bug 1334550 - Part 2 - Add nsISubstitutionObserver and update test_extensionURL.html;

Adds nsISubstitutionObserver so that substitutions set on a
parent nsISubstitutingProtocolHandler which are then propagated
to child processes can be observed in the child.

Updates test_extensionURL.html to set substitutions on the parent
ExtensionProtocolHandler before trying to load moz-extension URI's
using those substitutions.

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

Comment 50

[Haik Aftandilian [:haik]]

I've added a "part 3" patch to reviewboard that, for packed moz-extension requests, opens the JAR file off the main thread. That required me to make the NewFD method non-static. So I've added an ExtensionProtocolHandler singleton so that NeckoParent can access the protocol handler to call ExtensionProtocolHandler::{NewFD,NewStream} methods.

Comment 51

[Haik Aftandilian [:haik]]

Latest test results with security.sandbox.content.level=3 (for Mac only). The bc14 Linux debug failure shows there's a crash in shutdown. Working on that now.

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

Comment 52

[Kris Maglione [:kmag]]

Comment on attachment 8862184 [details]
Bug 1334550 - Part 3 - Open extension JAR files off the main thread;

https://reviewboard.mozilla.org/r/115848/#review144152

::: ipc/glue/FileDescriptor.cpp:146
(Diff revision 4)
> +// static
> +FileDescriptor::UniquePlatformHandle
> +FileDescriptor::ClonePlatformHandle(PlatformHandleType aHandle)
> +{
> +  return UniquePlatformHandle(Clone(aHandle));
> +}

This also doesn't really belong here.

::: ipc/glue/FileDescriptorUtils.cpp:125
(Diff revision 4)
> +PRFileDesc*
> +ClonePRFileDesc(PRFileDesc *aFileDesc)
> +{
> +  if (!aFileDesc) {
> +    return nullptr;
> +  }
> +
> +  FileDescriptor::PlatformHandleType platformHandle =
> +    FileDescriptor::PlatformHandleType(PR_FileDesc2NativeHandle(aFileDesc));
> +  if (!platformHandle) {
> +    return nullptr;
> +  }
> +
> +  FileDescriptor::PlatformHandleType newPlatformHandle =
> +    FileDescriptor::ClonePlatformHandle(platformHandle).release();
> +  if (newPlatformHandle != INVALID_HANDLE) {
> +    return PR_ImportFile(PROsfd(newPlatformHandle));
> +  } else {
> +    return nullptr;
> +  }
> +}

This doesn't really belong here, and shouldn't be necessary, in any case.

::: ipc/glue/FileDescriptorUtils.cpp:147
(Diff revision 4)
> +PRFileDesc *
> +FileDescriptorToPRFileDesc(const FileDescriptor& aDesc)
> +{
> +  auto platformFD = aDesc.ClonePlatformHandle();

This should probably be a FileDescriptor method, `CloneNSPRFileDescriptor`, or something along those lines.

::: netwerk/ipc/NeckoChild.h:136
(Diff revision 4)
> +  /*
> +   * Used to associate WebExtension request responses from the parent with
> +   * callbacks. Each WebExtension resource request sent to the parent includes
> +   * an ID that is sent back in its reply. This hash table maps ID's to
> +   * callbacks. |mExtensionMsgID| holds the ID of the last sent WebExtension
> +   * request and is incremented for each new outoing request.
> +   */
> +  nsClassHashtable<nsUint64HashKey, NeckoChildExtensionCallbacks>
> +    mExtensionPendingCallbacks;
> +  uint64_t mExtensionMsgID;

We should able to use async messages that return MozPromises for this now (bug 1313200). If we can't do that, for some reason, I'd prefer that we create a single-use protocol to return the response rather than using an ID map like this, since it has some builtin consistency guarantees that this doesn't give us.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:49
(Diff revision 4)
> +NS_IMPL_ISUPPORTS(ExtensionStreamGetter, nsISupports)
> +
> +class ErrorInputStream final : public nsIInputStream
> +{
> +public:
> +  explicit ErrorInputStream(nsresult error) : mError(error) {

Nit: Opening brace for function bodies on its own line. Same for the ones below.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:56
(Diff revision 4)
> +  }
> +  NS_DECL_THREADSAFE_ISUPPORTS
> +  NS_IMETHOD Close(void) override {
> +    return NS_OK;
> +  }
> +  NS_IMETHOD Available(uint64_t *_retval) override {

Nit: `*` attaches to the type, and all arguments should begin with `a`. Same below.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:122
(Diff revision 4)
> +  OptionalLoadInfoArgs loadInfo;
> +  nsresult rv;
> +  rv = mozilla::ipc::LoadInfoToLoadInfoArgs(mLoadInfo, &loadInfo);
> +  NS_ENSURE_SUCCESS(rv, false);
> +
> +  NeckoChild *gc = static_cast<mozilla::net::NeckoChild*>(gNeckoChild);

No need for the cast if you use an async return value in the protocol.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:126
(Diff revision 4)
> +
> +  NeckoChild *gc = static_cast<mozilla::net::NeckoChild*>(gNeckoChild);
> +
> +  if (mIsJarChannel) {
> +    // Request an FD for this moz-extension URI
> +    if (!gc->DoSendAsyncGetExtensionResourceFD(uri, loadInfo, this)) {

We shouldn't need to do this for every request, just once, the first time it's needed. And then make sure it's flushed whenever we get a matching "flush-cache-entry" observer notification.

Ideally, we should probably just make sure that the zip reader for the underlying jar: URI is stored in the zip reader cache, and then just use the underlying jar: URI directly after that, which would solve both problems.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:183
(Diff revision 4)
> +    FileDescriptorFile* fdFile = new FileDescriptorFile();
> +    fdFile->SetNSPRFileDesc(aPRFileDesc);
> +    fdFile->Init(mJarFileURI);

Please make this something like:

    RefPtr<FileDescriptorFile> fdFile = new FileDescriptorFile(aFD);

and remove the Init and SetNSPRFileDesc methods from the FileDescriptorFile class.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:188
(Diff revision 4)
> +    FileDescriptorFile* fdFile = new FileDescriptorFile();
> +    fdFile->SetNSPRFileDesc(aPRFileDesc);
> +    fdFile->Init(mJarFileURI);
> +    mJarChannel->SetJarFile(fdFile);
> +
> +    nsresult rv = mJarChannel->Open2(getter_AddRefs(inputStream));

`return mJarChannel->AsyncOpen2(mListener)`

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:370
(Diff revision 4)
>    return NS_OK;
>  }
>  
> -#undef NS_TRY
> +/* static */
> +nsresult
> +ExtensionProtocolHandler::NewStream(nsIURI* aChildURI,

It would be much more efficient to send back a file descriptor for the unpacked file here.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:376
(Diff revision 4)
> +                                    nsILoadInfo* aChildLoadInfo,
> +                                    nsIInputStream** aInputStream,
> +                                    bool* aTerminateSender)
> +{
> +  MOZ_ASSERT(!IsNeckoChild());
> +  *aTerminateSender = true;

Please add a comment about the purpose of this parameter.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:384
(Diff revision 4)
> +  // these requests ordinarily come from the child's ExtensionProtocolHandler.
> +  // Ensure this request is for a moz-extension URI. A rogue child process
> +  // could send us any URI.
> +  nsAutoCString scheme;
> +  nsresult rv = aChildURI->GetScheme(scheme);
> +  NS_ENSURE_SUCCESS(rv, rv);

Nit: I'm trying to use Result and MOZ_TRY consistently in extension code. I'm working on getting some shared helpers landed for this, but in the mean time, please copy this boilerplate:

http://searchfox.org/mozilla-central/rev/f55349994fdac101d121b11dac769f3f17fbec4b/toolkit/mozapps/extensions/AddonManagerStartup.cpp#30-61

and change any non-XPCOM functions to return an appropriate Result type (like `Result<Ok, nsresult>`), and use NS_TRY rather than NS_ENSURE_SUCCESS when calling XPCOM functions.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:385
(Diff revision 4)
> +  // Ensure this request is for a moz-extension URI. A rogue child process
> +  // could send us any URI.
> +  nsAutoCString scheme;
> +  nsresult rv = aChildURI->GetScheme(scheme);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  if (!scheme.EqualsLiteral(EXTENSION_PROTOCOL_HANDLER_SCHEME_LITERAL)) {

Nit: I'd prefer something like:

    if (NS_FAILED(uri->SchemeIs(EXTENSION_PROTOCOL_SCHEME, &matches) || !matches)

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:389
(Diff revision 4)
> +  nsCOMPtr<nsIIOService> ioService;
> +  ioService = do_GetIOService(&rv);

Nit: `nsCOMPtr<nsIIOService> ioService = do_GetIOService(&rv);`

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:396
(Diff revision 4)
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsCOMPtr<nsIProtocolHandler> protocolHandler;
> +  rv = ioService->GetProtocolHandler(scheme.get(),
> +                                     getter_AddRefs(protocolHandler));
> +  NS_ENSURE_SUCCESS(rv, rv);

There's no need to explicitly fetch any protocol handlers here, or to duplicate any of the substitution or security check logic here. Please just confirm that the scheme is correct and then use `NS_NewChannel` to open the channel. If we need additional security checks, please add them to the protocol handler.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:403
(Diff revision 4)
> +  // Ensure it's a substituting protocol handler. We expect the protocol
> +  // handler to be an ExtensionProtocolHandler because we've already
> +  // checked the URI scheme.
> +  nsCOMPtr<nsISubstitutingProtocolHandler> subProtocolHandler =
> +    do_QueryInterface(protocolHandler, &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);

No need to check the `rv` here. You can `MOZ_ASSERT(subProtocolHandler)`, though.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:433
(Diff revision 4)
> +
> +  nsCOMPtr<nsIFile> extensionDir;
> +  rv = fileURL->GetFile(getter_AddRefs(extensionDir));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // Ensure it's a directory

This comment isn't necesary.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:486
(Diff revision 4)
> +  rv = NS_NewLocalFileInputStream(getter_AddRefs(inputStream), requestedFile,
> +                                  PR_RDONLY, -1,
> +                                  nsIFileInputStream::DEFER_OPEN);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  NS_ADDREF(*aInputStream = inputStream);

inputStream.forget(aInputStream);

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:500
(Diff revision 4)
> +  // Ensure this is a moz-extension URI
> +  nsAutoCString scheme;
> +  nsresult rv = aChildURI->GetScheme(scheme);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  if (!scheme.EqualsLiteral(EXTENSION_PROTOCOL_HANDLER_SCHEME_LITERAL)) {
> +    return NS_ERROR_UNKNOWN_PROTOCOL;
> +  }
> +
> +  nsCOMPtr<nsIIOService> ioService;
> +  ioService = do_GetIOService(&rv);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // Get the protocol handler
> +  nsCOMPtr<nsIProtocolHandler> protocolHandler;
> +  rv = ioService->GetProtocolHandler(scheme.get(),
> +                                     getter_AddRefs(protocolHandler));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsCOMPtr<nsISubstitutingProtocolHandler> subProtocolHandler =
> +    do_QueryInterface(protocolHandler, &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);

Same comments as above.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:526
(Diff revision 4)
> +  // Get the host string from the child's request URI
> +  nsAutoCString host;
> +  rv = aChildURI->GetAsciiHost(host);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // We expect the host string to map to a JAR file because the URI
> +  // should refer to a web accessible resource for an enabled extension.
> +  // We bypass calling ResolveURI to avoid any risk of the path and file
> +  // portion of the URI leading outside of the JAR.
> +  nsCOMPtr<nsIURI> subURI;
> +  rv = subProtocolHandler->GetSubstitution(host, getter_AddRefs(subURI));
> +  NS_ENSURE_SUCCESS(rv, rv);

Please just use ResolveURI here.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:539
(Diff revision 4)
> +  // Ensure this is a JAR URI
> +  // Example: jar:file:///extension.xpi!/
> +  nsCOMPtr<nsIJARURI> jarURI = do_QueryInterface(subURI, &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // Get the inner file URI
> +  // Example: file:///extension.xpi
> +  nsCOMPtr<nsIURI> innerFileURI;
> +  rv = jarURI->GetJARFile(getter_AddRefs(innerFileURI));
> +  NS_ENSURE_SUCCESS(rv, rv);

These comments aren't very helpful.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:550
(Diff revision 4)
> +  // Get the inner file spec
> +  nsAutoCString fileSpec;
> +  rv = innerFileURI->GetSpec(fileSpec);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // Get an nsIFile for the actual file
> +  nsCOMPtr<nsIFile> jarFile;
> +  rv = net_GetFileFromURLSpec(fileSpec, getter_AddRefs(jarFile));
> +  NS_ENSURE_SUCCESS(rv, rv);

Just QueryInterface to nsIFileURI and call GetFile to get the underlying nsIFile.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:561
(Diff revision 4)
> +  nsCOMPtr<nsIFile> jarFile;
> +  rv = net_GetFileFromURLSpec(fileSpec, getter_AddRefs(jarFile));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // Get our local FD
> +  PRFileDesc* prFileDesc;

Please use `AutoFDClose` for this: http://searchfox.org/mozilla-central/rev/24c443a440104dabd9447608bd41b8766e8fc2f5/xpcom/glue/FileUtils.h#71

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:566
(Diff revision 4)
> +  FileDescriptor ipcFileDesc;
> +  ipcFileDesc = FileDescriptor(FileDescriptor::PlatformHandleType(
> +                               PR_FileDesc2NativeHandle(prFileDesc)));

FileDescriptor ipcFileDesc(...);

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:570
(Diff revision 4)
> +  // Get IPC FD
> +  FileDescriptor ipcFileDesc;
> +  ipcFileDesc = FileDescriptor(FileDescriptor::PlatformHandleType(
> +                               PR_FileDesc2NativeHandle(prFileDesc)));
> +  PR_Close(prFileDesc);
> +  if (!ipcFileDesc.IsValid()) {

No need to check this.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:574
(Diff revision 4)
> +  PR_Close(prFileDesc);
> +  if (!ipcFileDesc.IsValid()) {
> +    return NS_ERROR_UNEXPECTED;
> +  }
> +
> +  aFD = ipcFileDesc;

`= Move(ipcFileDesc)`

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:589
(Diff revision 4)
> +  nsCOMPtr<nsIChannel> channel = NS_NewSimpleChannel(
> +    aURI, aLoadinfo, aStreamGetter,
> +    [] (nsIStreamListener* listener, nsIChannel* channel,
> +        ExtensionStreamGetter* getter) -> RequestOrReason {
> +      if (getter->GetAsync(listener, channel)) {
> +        return RequestOrReason(NS_OK);

return nullptr;

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:590
(Diff revision 4)
> +    aURI, aLoadinfo, aStreamGetter,
> +    [] (nsIStreamListener* listener, nsIChannel* channel,
> +        ExtensionStreamGetter* getter) -> RequestOrReason {
> +      if (getter->GetAsync(listener, channel)) {
> +        return RequestOrReason(NS_OK);
> +      } else {

No need for the else.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:591
(Diff revision 4)
> +    [] (nsIStreamListener* listener, nsIChannel* channel,
> +        ExtensionStreamGetter* getter) -> RequestOrReason {
> +      if (getter->GetAsync(listener, channel)) {
> +        return RequestOrReason(NS_OK);
> +      } else {
> +        return RequestOrReason(NS_ERROR_UNEXPECTED);

return Err(NS_ERROR_UNEXPECTED);

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:595
(Diff revision 4)
> +      } else {
> +        return RequestOrReason(NS_ERROR_UNEXPECTED);
> +      }
> +    });
> +
> +  NS_ADDREF(*aRetVal = channel);

return channel.forget();

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:621
(Diff revision 4)
> +  nsCOMPtr<nsIIOService> ioService;
> +  ioService = do_GetIOService(&rv);

`nsCOMPtr<nsIIOServices> ioService = do_GetIOService();`

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:625
(Diff revision 4)
> +  // Build a JAR URI for this jar:file:// URI and use it to extract the
> +  // inner file URI.
> +  nsCOMPtr<nsIURI> uri;
> +  rv = ioService->NewURI(aResolvedSpec, nullptr, nullptr, getter_AddRefs(uri));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  nsCOMPtr<nsIJARURI> jarURI = do_QueryInterface(uri, &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);

`NS_NewURI(getter_AddRefs(uri), aResolvedSpec);`

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:638
(Diff revision 4)
> +  // Get the JAR URI scheme string, this should be "jar:"
> +  nsAutoCString jarScheme;
> +  rv = jarURI->GetScheme(jarScheme);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // Get a JAR protocol handler
> +  nsCOMPtr<nsIProtocolHandler> ph;
> +  rv = ioService->GetProtocolHandler(jarScheme.get(), getter_AddRefs(ph));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // Get a JAR channel
> +  nsCOMPtr<nsIChannel> baseChannel;
> +  rv = ph->NewChannel2(jarURI, aLoadinfo, getter_AddRefs(baseChannel));
> +  NS_ENSURE_SUCCESS(rv, rv);

`NS_NewChannel(getter_AddRefs(channel), uri);`

::: xpcom/io/FileDescriptorFile.h:36
(Diff revision 4)
> +  // regular nsIFile object, that we forward most calls to.
> +  nsCOMPtr<nsIFile> mFile;
> +  nsCOMPtr<nsIURI> mURI;
> +  PRFileDesc* mNSPRFileDesc;

Please just store the FileDescriptor object here, and extract a native file descriptor from that where necessary.

::: xpcom/io/FileDescriptorFile.cpp:37
(Diff revision 4)
> +  if (mNSPRFileDesc) {
> +    PR_Close(mNSPRFileDesc);
> +  }

Not necessary if you use a FileDescriptor object.

::: xpcom/io/FileDescriptorFile.cpp:43
(Diff revision 4)
> +    PR_Close(mNSPRFileDesc);
> +  }
> +}
> +
> +nsresult
> +FileDescriptorFile::Init(nsIURI* aFileURI)

Please make this take a nsIFileURL directly, rather than checking and querying a nsIURI argument.

::: xpcom/io/FileDescriptorFile.cpp:108
(Diff revision 4)
> +  // Ignore optional/deprecated DELETE_ON_CLOSE flag
> +  aFlags &= ~nsIFile::DELETE_ON_CLOSE;

We should probably throw if this is set.

Comment 53

[Kris Maglione [:kmag]]

Comment on attachment 8862184 [details]
Bug 1334550 - Part 3 - Open extension JAR files off the main thread;

https://reviewboard.mozilla.org/r/115848/#review144774

Sorry, lots of comments so far. I may have some more once you address these.

Over all it looks pretty good, I just have issues with some of the particulars.

Comment 54

[Honza Bambas (:mayhemer)]

(In reply to Haik Aftandilian [:haik] from comment #45)
> Honza, I tried reflagging you for review, but reviewboard isn't letting me
> do that. Could you take a look at my updates/questions?

Which one?  You can also use the good old bugzilla attachment edit (the details link) to do that :)  Please do so otherwise I loose it.

Comment 55

[Honza Bambas (:mayhemer)]

Bad NI... sorry.

Comment 56

[Honza Bambas (:mayhemer)]

Comment on attachment 8869275 [details]
Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent process;

https://reviewboard.mozilla.org/r/140834/#review145980

You haven't addressed comment 35 _AT ALL_ and I don't see (or is well hidden) any explanation _why_.  Please provide that explanation or update the patch.

Comment 57

[Haik Aftandilian [:haik]]

(In reply to Honza Bambas (:mayhemer) from comment #56)
> Comment on attachment 8869275 [details]
> Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent
> process;
> 
> https://reviewboard.mozilla.org/r/140834/#review145980
> 
> You haven't addressed comment 35 _AT ALL_ and I don't see (or is well
> hidden) any explanation _why_.  Please provide that explanation or update
> the patch.

I responded to that in comment 44. I've also discussed this with baku. That recommendation isn't going to work because we don't have support for creating a JARChannel from an input stream (Bug 1364151). Without 1364151, we need an FD in the child for the JARChannel. IPCBlobInputStream doesn't work here because it doesn't expose the FD to the sender when the receiver is sending a FileInputStream. I don't think this work should block on bug 1364151 because read-access sandboxing is a high priority. And my latest changes (will post shortly) open the file off the main thread.

I'll post new patches today that address Kris' feedback and reduce the amount of Necko changes. Using MozPromise simplified the code significantly. Then I'll reflag you for review. After Kris provided comments, I was waiting to finish those before asking you to look again. Sorry for the confusion.

Comment 58

[Haik Aftandilian [:haik]]

Clearing the NI for Kris. Will post updated code shortly.

Comment 59

[Honza Bambas (:mayhemer)]

(In reply to Haik Aftandilian [:haik] from comment #57)
> I responded to that in comment 44. I've also discussed this with baku. That

Thanks for the detailed explanation, it was not in my power to see comment 44.

Comment 60

[Haik Aftandilian [:haik]]

Comment on attachment 8862184 [details]
Bug 1334550 - Part 3 - Open extension JAR files off the main thread;

https://reviewboard.mozilla.org/r/115848/#review144152

New version of the patches on the way.

I'm condensed the patches down to 2 since it was difficult to keep patch 3 separate.

The only changes to patch 2 are for eslint correctness of the test file.

Some notes:

By changing FileDescriptorFile to use FileDescriptor instead of PR_FileDesc, all the FileDescriptor.{h,cpp} and FileDescriptorUtils.{h,cpp} changes fall away.

Changing the code to use MozPromises eliminated all the NeckoChild changes.

I'd like to address the following of Kris' comments in a follow up patch:
1) Changing all code to use Result and MOZ_TRY
2) Move the unpacked stream directory check to common ExtensionProtocolHandler code so it's used for all NewChannel calls.
3) Throwing when nsIFile::DELETE_ON_CLOSE is passed to FileDescriptorFile::OpenNSPRFileDesc. I think there's a test that passes this that will need to be fixed first.
4) Investigate JARChannel caching. I know some level of caching is occurring because I've observed that ExtensionProtocolHandler::NewChannel2 is not called for every request.

I think I've addressed all other comments. See other responses.

> This should probably be a FileDescriptor method, `CloneNSPRFileDescriptor`, or something along those lines.

Not needed in the end.

> We shouldn't need to do this for every request, just once, the first time it's needed. And then make sure it's flushed whenever we get a matching "flush-cache-entry" observer notification.
> 
> Ideally, we should probably just make sure that the zip reader for the underlying jar: URI is stored in the zip reader cache, and then just use the underlying jar: URI directly after that, which would solve both problems.

I'd like to investigate the caching in a follow-up patch. I know some type of caching is happening because a new ExtensionProtocolHandler::NewChannel2 call isn't happening for every request.

> Please make this something like:
> 
>     RefPtr<FileDescriptorFile> fdFile = new FileDescriptorFile(aFD);
> 
> and remove the Init and SetNSPRFileDesc methods from the FileDescriptorFile class.

I kept the Init method so that I could do error handling when the nsIFileURL->GetFile() fails.

> It would be much more efficient to send back a file descriptor for the unpacked file here.

In NeckoParent we use IPCBlobInputStream which opens the file off the main thread and sends the FD to the child, doing the I/O in the child. So we are doing FD-based I/O in the child and not streaming data over IPC if that was your concern.

> Please add a comment about the purpose of this parameter.

There's an existing JavaDoc-style comment for this parameter in ExtensionProtocolHandler.h.

> Nit: I'm trying to use Result and MOZ_TRY consistently in extension code. I'm working on getting some shared helpers landed for this, but in the mean time, please copy this boilerplate:
> 
> http://searchfox.org/mozilla-central/rev/f55349994fdac101d121b11dac769f3f17fbec4b/toolkit/mozapps/extensions/AddonManagerStartup.cpp#30-61
> 
> and change any non-XPCOM functions to return an appropriate Result type (like `Result<Ok, nsresult>`), and use NS_TRY rather than NS_ENSURE_SUCCESS when calling XPCOM functions.

I'ved marked this as dropped and plan to address it in a follow-up.

> There's no need to explicitly fetch any protocol handlers here, or to duplicate any of the substitution or security check logic here. Please just confirm that the scheme is correct and then use `NS_NewChannel` to open the channel. If we need additional security checks, please add them to the protocol handler.

Updated the code to use NS_NewChannel. Some of the old code fetching the protocol handler was stale, from when NewStream was a static method.

I'd like to move the directory check to common protocol handler code paths in a follow up since it affects all unpacked extension loading and I'm weary of adding this change right now.

> Please just use ResolveURI here.

I don't think it makes sense to call ResolveURI. We only want to get the URI that GetSubstitution returns and don't need the ResolveURI result. The ResolveURI result would be the URI for the resource within the JAR file and then we'd extract the base file out of it. Also, ResolveURI calls ResolveSpecialCases which doesn't apply because the special cases are handled in the child. If you feel strongly that that's the right way to go, I can spend more time on it in a follow-up.

> return nullptr;

Changed it to RequestOrReason(nullptr).

> We should probably throw if this is set.

I think we have some tests that pass nsIFile::DELETE_ON_CLOSE. I'll file a bug on that.

Comment 61

[Haik Aftandilian [:haik]]

Comment on attachment 8869275 [details]
Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent process;

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

Comment 62

[Haik Aftandilian [:haik]]

Comment on attachment 8869276 [details]
Bug 1334550 - Part 2 - Add nsISubstitutionObserver and update test_extensionURL.html;

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

Comment 63

[Honza Bambas (:mayhemer)]

Comment on attachment 8869275 [details]
Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent process;

https://reviewboard.mozilla.org/r/140834/#review146404

::: modules/libjar/nsIJARChannel.idl:26
(Diff revision 2)
>  
>      /**
>       * Returns the JAR file.  May be null if the jar is remote.
> +     * Setting the JAR file is optional and overrides the JAR
> +     * file used for local file JARs. Setting the JAR file after
> +     * the channel has been opened is an error.

is an error and doesn't have any effect on the channel.

?

::: modules/libjar/nsJARChannel.cpp:353
(Diff revision 2)
>  
> +    if (mJarFileOverride) {
> +      mJarFile = mJarFileOverride;
> +      LOG(("nsJARChannel::LookupFile [this=%p] Overriding mJarFile\n", this));
> +      return NS_OK;
> +    }

any reason to do this elsewhere than on line 333? (right after we check that we already have mJarFile)

::: modules/libjar/nsJARChannel.cpp:900
(Diff revision 2)
> +{
> +    if (mOpened) {
> +        return NS_ERROR_IN_PROGRESS;
> +    }
> +    if (!aFile) {
> +        return NS_ERROR_INVALID_ARG;

just an idea: what if someone wants to remove the override?

::: netwerk/protocol/res/ExtensionProtocolHandler.h:26
(Diff revision 2)
> +
>  namespace mozilla {
> +
>  namespace net {
>  
> +class ExtensionStreamGetter : public nsISupports

add a comment what this is used for

::: netwerk/protocol/res/ExtensionProtocolHandler.h:58
(Diff revision 2)
> +    nsCOMPtr<nsILoadInfo> mLoadInfo;
> +    nsCOMPtr<nsIJARChannel> mJarChannel;
> +    nsCOMPtr<nsIFileURL> mJarFileURL;
> +    nsCOMPtr<nsIStreamListener> mListener;
> +    nsCOMPtr<nsIInputStreamPump> mPump;
> +    nsIChannel* mChannel;

explain very carefully why this is held only weakly

::: netwerk/protocol/res/ExtensionProtocolHandler.h:104
(Diff revision 2)
> +  /*
> +   * Helper runnable argument for the NewFD method. Meant to be subclassed.
> +   * After the JAR file is opened and its file descriptor is obtained on
> +   * another thread, an instance of this runnable is dispatched to the main
> +   * thread so that the caller can consume the FD.
> +   */

I don't see this used, is there some other patch that uses this?

::: netwerk/protocol/res/ExtensionProtocolHandler.h:165
(Diff revision 2)
>                                                    nsILoadInfo* aLoadInfo,
>                                                    nsIChannel** result) override;
> +
> +  nsresult SubstituteRemoteChannel(nsIURI* aURI,
> +                                   nsILoadInfo* aLoadInfo,
> +                                   nsIChannel** result);

comments please

::: netwerk/protocol/res/ExtensionProtocolHandler.h:178
(Diff revision 2)
> +                        nsIChannel** aRetVal);
> +
> +private:
> +  nsCOMPtr<nsIThread> mFileOpenerThread;
> +  static ExtensionProtocolHandler *sSingleton;
> +  bool mUseRemoteFileChannels;

comment on _all_ members what they are

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:67
(Diff revision 2)
> +    return mError;
> +  }
> +  NS_IMETHOD ReadSegments(nsWriteSegmentFun aWriter, void* aClosure,
> +                         uint32_t aCount, uint32_t* aRetVal) override
> +  {
> +    return NS_ERROR_NOT_IMPLEMENTED;

you may return mError too?  if not, comment why NOT_IMPL is what you return.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:75
(Diff revision 2)
> +  {
> +    *aRetVal = false;
> +    return NS_OK;
> +  }
> +protected:
> +  ~ErrorInputStream()

probably needs to be virtual

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:588
(Diff revision 2)
> +  nsCOMPtr<nsIFile> jarFile;
> +  rv = innerFileURL->GetFile(getter_AddRefs(jarFile));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  if (!mFileOpenerThread) {
> +    rv = NS_NewThread(getter_AddRefs(mFileOpenerThread));

we have LazyIdleThread for this, please use it.  it closes itself when not used for some time.

https://dxr.mozilla.org/mozilla-central/source/xpcom/threads/LazyIdleThread.h

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:662
(Diff revision 2)
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsCOMPtr<nsIChannel> channel;
> +  rv = NS_NewChannel(getter_AddRefs(channel),
> +                     uri,
> +                     nsContentUtils::GetSystemPrincipal(),

are we sure it's the right principal to use here?  if so, can you comment why?

::: xpcom/io/FileDescriptorFile.h:20
(Diff revision 2)
> +#include "private/pprio.h"
> +
> +namespace mozilla {
> +namespace net {
> +
> +class FileDescriptorFile final : public nsIFile

please write what's this used for, how..

::: xpcom/io/FileDescriptorFile.cpp:40
(Diff revision 2)
> +
> +FileDescriptorFile::FileDescriptorFile(const FileDescriptorFile& other)
> +{
> +  mFD = FileDescriptor(other.mFD.ClonePlatformHandle().get());
> +
> +  nsCOMPtr<nsIURI> uri = do_QueryInterface(other.mFileURL);

Clone is on nsIFileURL (inherited), no need for an expensive QI

::: xpcom/io/FileDescriptorFile.cpp:54
(Diff revision 2)
> +  if (!mFileURL) {
> +    DBG(("::Init error, invalid file URL\n"));
> +    return NS_ERROR_INVALID_ARG;
> +  }
> +
> +  nsresult rv = mFileURL->GetFile(getter_AddRefs(mFile));

nit: please declare nsresult rv as the first var in the function

::: xpcom/io/FileDescriptorFile.cpp:81
(Diff revision 2)
> + */
> +NS_IMETHODIMP
> +FileDescriptorFile::OpenNSPRFileDesc(int32_t aFlags, int32_t aMode,
> +                                     PRFileDesc **aRetval)
> +{
> +  // Ignore optional OS_READAHEAD flag

if this is not just copied from somewhere else (I suspect it is) please say "why"

Comment 64

[Haik Aftandilian [:haik]]

Comment on attachment 8869275 [details]
Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent process;

https://reviewboard.mozilla.org/r/140834/#review146404

Thanks for the comments. Responded to most feedback now. Will answer your other questions later today after my meetings.

> is an error and doesn't have any effect on the channel.
> 
> ?

I'll update the comment. It doesn't have any effect on the channel because the SetJarFile() call just returns an error.

> any reason to do this elsewhere than on line 333? (right after we check that we already have mJarFile)

Because we still want to call GetJARFile() and GetJAREntry() before we return. With the JARChannel changes, I tried to be as minimal as possible.

> just an idea: what if someone wants to remove the override?

Good point. I'll add code to clear it as long as the channel has not been opened.

> I don't see this used, is there some other patch that uses this?

I need to delete it, sorry. It's not used. It factored out when I switched to using MozPromises.

> are we sure it's the right principal to use here?  if so, can you comment why?

OK. I need some help with that. Any page could load a moz-extension URI so I think SystemPrincipal is wrong. I'll ask around.

Comment 65

[Haik Aftandilian [:haik]]

Comment on attachment 8869275 [details]
Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent process;

https://reviewboard.mozilla.org/r/140834/#review146404

> add a comment what this is used for

Done and moved the definition of ExtensionStreamGetter to the .cpp because it is now completely private to the implementation.

> explain very carefully why this is held only weakly

Will make sure I'm doing this correctly and update comment if necessary.

> comments please

Added

> comment on _all_ members what they are

Done

> you may return mError too?  if not, comment why NOT_IMPL is what you return.

Changed to return mError.

> probably needs to be virtual

Yes, done.

> we have LazyIdleThread for this, please use it.  it closes itself when not used for some time.
> 
> https://dxr.mozilla.org/mozilla-central/source/xpcom/threads/LazyIdleThread.h

Done

> please write what's this used for, how..

Done.

> Clone is on nsIFileURL (inherited), no need for an expensive QI

I asked about this on #developers and it was recommended to keep the QI because nsIFileURL is scriptable and hence a cast isn't safe. I did fix a bug here. Previously the code was not setting mFileURL, just cloning other.mFileURL to itself.

> nit: please declare nsresult rv as the first var in the function

Done

> if this is not just copied from somewhere else (I suspect it is) please say "why"

This code was mostly copied from the old RemoteOpenFile class that was removed from Gecko.

We remove OS_READAHEAD just to simply the aFlags check below where the code checks "if (aFlags != PR_RDONLY)".

Alternatively, the code could be "((aFlags & ~nsIFile::OS_READAHEAD) != PR_RDONLY)".

Comment 66

[Haik Aftandilian [:haik]]

Comment on attachment 8869275 [details]
Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent process;

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

Comment 67

[Haik Aftandilian [:haik]]

Comment on attachment 8869276 [details]
Bug 1334550 - Part 2 - Add nsISubstitutionObserver and update test_extensionURL.html;

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

Comment 68

[Haik Aftandilian [:haik]]

Comment on attachment 8869275 [details]
Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent process;

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

Comment 69

[Haik Aftandilian [:haik]]

Comment on attachment 8869276 [details]
Bug 1334550 - Part 2 - Add nsISubstitutionObserver and update test_extensionURL.html;

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

Comment 70

[Honza Bambas (:mayhemer)]

(In reply to Haik Aftandilian [:haik] from comment #65)
> > Clone is on nsIFileURL (inherited), no need for an expensive QI
> 
> I asked about this on #developers and it was recommended to keep the QI
> because nsIFileURL is scriptable and hence a cast isn't safe. I did fix a
> bug here. Previously the code was not setting mFileURL, just cloning
> other.mFileURL to itself.

Thanks, if you got a chance, maybe add this comment to the code?

Comment 71

[Honza Bambas (:mayhemer)]

Please be aware of: https://bugzilla.mozilla.org/show_bug.cgi?id=1256122#c88

Comment 72

[Kris Maglione [:kmag]]

Comment on attachment 8869275 [details]
Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent process;

https://reviewboard.mozilla.org/r/140834/#review146404

> I asked about this on #developers and it was recommended to keep the QI because nsIFileURL is scriptable and hence a cast isn't safe. I did fix a bug here. Previously the code was not setting mFileURL, just cloning other.mFileURL to itself.

Why do you need the do_QueryInterface at all? The Clone() call will just overwrite whatever value the query stores there unless it fails. If it fails, the value of the out param is undefined. If it succeeds, chances are it will overwrite the value in the nsCOMPtr rather than swapping it with the new value, which means this will leak, since the nsIURI value you got from mFileURL will never be decrefed.

Comment 73

[Haik Aftandilian [:haik]]

Comment on attachment 8869275 [details]
Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent process;

https://reviewboard.mozilla.org/r/140834/#review146404

> Why do you need the do_QueryInterface at all? The Clone() call will just overwrite whatever value the query stores there unless it fails. If it fails, the value of the out param is undefined. If it succeeds, chances are it will overwrite the value in the nsCOMPtr rather than swapping it with the new value, which means this will leak, since the nsIURI value you got from mFileURL will never be decrefed.

This code was broken in that wasn't cloning other.mFileURL at all. I've since updated the patch. But, now that you point it out, I don't check the return value of Clone() and need to do that. In the latest version, do you still think the QueryInterface is not needed?

Comment 74

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Haik Aftandilian [:haik] from comment #64)
> OK. I need some help with that. Any page could load a moz-extension URI so I
> think SystemPrincipal is wrong. I'll ask around.

Using SystemPrincipal in that case is most likely wrong. As a rule of thumb, if the URI to be loaded can be provided by anything other than Firefox itself (e.g. a webpage can provide the URI) than using systemPrincipal is not desired. Anyway, looking at the code where you create the new channel you are receiving a loadinfo as an argument. It's best if you just use NS_NewChannelInternal(..., aLoadInfo, ...).

Comment 75

[Haik Aftandilian [:haik]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #74)
> (In reply to Haik Aftandilian [:haik] from comment #64)
> > OK. I need some help with that. Any page could load a moz-extension URI so I
> > think SystemPrincipal is wrong. I'll ask around.
> 
> Using SystemPrincipal in that case is most likely wrong. As a rule of thumb,
> if the URI to be loaded can be provided by anything other than Firefox
> itself (e.g. a webpage can provide the URI) than using systemPrincipal is
> not desired. Anyway, looking at the code where you create the new channel
> you are receiving a loadinfo as an argument. It's best if you just use
> NS_NewChannelInternal(..., aLoadInfo, ...).

Thanks Christoph. I'll update the patch to use NS_NewChannelInternal.

Comment 76

[Haik Aftandilian [:haik]]

I'm hitting a failure on Windows in devtools/server/tests/mochitest/test_webextension-addon-debugging-reload.html during the addon.uninstall() call for OOP extensions. There appears to be a race condition caused or exposed by the fix here. It only occurs with the OOP test, not the in-process extension test in the same file. When I slowed down addon.uninstall() using breakpoints, the test passed.

As a workaround, I added and will post this fix as patch 3. The same workaround is used in Experiments.jsm:uninstallAddons().

   let waitShutdown = promiseWebExtensionShutdown();
+  // Disabling the add-on before uninstalling is necessary to cause tests to
+  // pass. This might be indicative of a bug in XPIProvider. Bug 992396.
+  addon.userDisabled = true;
   addon.uninstall();
   await waitShutdown;

Here's the full error.

GECKO(9344) | 1496082194920	addons.xpi ERROR
Failed to move file c:\users\haikaf~1\appdata\local\temp\tmpabbsqp.mozrunner\extensions\[e-mail]
to c:\users\haikaf~1\appdata\local\temp\tmpabbsqp.mozrunner\extensions\trash:
[Exception... "Component returned failure code: 0x8052000e (NS_ERROR_FILE_IS_LOCKED)
  [nsIFile.moveTo]"  nsresult: "0x8052000e (NS_ERROR_FILE_IS_LOCKED)" 
                     location: "JS frame :: resource://gre/modules/addons/XPIProvider.jsm :: _installFile :: line 607"  data: no]
  Stack trace: _installFile()@resource://gre/modules/addons/XPIProvider.jsm:607
             < _installDirEntry()@resource://gre/modules/addons/XPIProvider.jsm:688
             < moveUnder()@resource://gre/modules/addons/XPIProvider.jsm:708
             < uninstallAddon()@resource://gre/modules/addons/XPIProvider.jsm:6297
             < uninstallAddon()@resource://gre/modules/addons/XPIProvider.jsm:4754
             < uninstall()@resource://gre/modules/addons/XPIProvider.jsm:5563
             < test_reload_addon()@test_webextension-addon-debugging-reload.html:109

GECKO(9344) | 1496082194927	addons.xpi ERROR
Failure moving c:\users\haikaf~1\appdata\local\temp\tmpabbsqp.mozrunner\extensions\[e-mail]
to c:\users\haikaf~1\appdata\local\temp\tmpabbsqp.mozrunner\extensions\trash

26 INFO TEST-UNEXPECTED-FAIL | devtools/server/tests/mochitest/test_webextension-addon-debugging-reload.html | 
[Exception... "Component returned failure code: 0x8052000e (NS_ERROR_FILE_IS_LOCKED)
[nsIFile.moveTo]"  nsresult: "0x8052000e (NS_ERROR_FILE_IS_LOCKED)"
                   location: "JS frame :: resource://gre/modules/addons/XPIProvider.jsm :: _installFile :: line 607"  data: no]

Comment 77

[Haik Aftandilian [:haik]]

Attachment: Bug 1334550 - Part 3 - Workaround for addon.uninstall() file locking issue;

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

Comment 78

[Haik Aftandilian [:haik]]

Comment on attachment 8869275 [details]
Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent process;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140834/diff/4-5/

Comment 79

[Haik Aftandilian [:haik]]

Comment on attachment 8869276 [details]
Bug 1334550 - Part 2 - Add nsISubstitutionObserver and update test_extensionURL.html;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140836/diff/4-5/

Comment 80

[Jim Mathies [:jimm]]

Kris, any chance we can get these finished up this week? Alternatively is there someone else who can finish these reviews up?

Comment 81

[Haik Aftandilian [:haik]]

Comment on attachment 8869275 [details]
Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent process;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140834/diff/5-6/

Comment 82

[Haik Aftandilian [:haik]]

Comment on attachment 8869276 [details]
Bug 1334550 - Part 2 - Add nsISubstitutionObserver and update test_extensionURL.html;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140836/diff/5-6/

Comment 83

[Haik Aftandilian [:haik]]

Comment on attachment 8872427 [details]
Bug 1334550 - Part 3 - Workaround for addon.uninstall() file locking issue;

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

Comment 84

[Haik Aftandilian [:haik]]

Comment on attachment 8869275 [details]
Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent process;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140834/diff/6-7/

Comment 85

[Haik Aftandilian [:haik]]

Comment on attachment 8869276 [details]
Bug 1334550 - Part 2 - Add nsISubstitutionObserver and update test_extensionURL.html;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140836/diff/6-7/

Comment 86

[Haik Aftandilian [:haik]]

Comment on attachment 8872427 [details]
Bug 1334550 - Part 3 - Workaround for addon.uninstall() file locking issue;

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

Comment 87

[Haik Aftandilian [:haik]]

Comment on attachment 8869275 [details]
Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent process;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140834/diff/7-8/

Comment 88

[Haik Aftandilian [:haik]]

Comment on attachment 8869276 [details]
Bug 1334550 - Part 2 - Add nsISubstitutionObserver and update test_extensionURL.html;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140836/diff/7-8/

Comment 89

[Haik Aftandilian [:haik]]

Comment on attachment 8872427 [details]
Bug 1334550 - Part 3 - Workaround for addon.uninstall() file locking issue;

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

Comment 90

[Haik Aftandilian [:haik]]

Posted an update to use NS_TRY rather than NS_ENSURE_SUCCESS where possible. This was a pretty superficial change.

Remaining work to land in a follow-up patch is 1) move directory security checks to common code paths (not just remoting code) and 2) utilize the JAR cache to avoid requesting an FD when the JAR is cached.

Comment 91

[Haik Aftandilian [:haik]]

Comment on attachment 8869275 [details]
Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent process;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140834/diff/8-9/

Comment 92

[Haik Aftandilian [:haik]]

Comment on attachment 8869276 [details]
Bug 1334550 - Part 2 - Add nsISubstitutionObserver and update test_extensionURL.html;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140836/diff/8-9/

Comment 93

[Haik Aftandilian [:haik]]

Comment on attachment 8872427 [details]
Bug 1334550 - Part 3 - Workaround for addon.uninstall() file locking issue;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143928/diff/4-5/

Comment 94

[Kris Maglione [:kmag]]

Comment on attachment 8869275 [details]
Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent process;

https://reviewboard.mozilla.org/r/140834/#review147178

::: xpcom/io/FileDescriptorFile.cpp:26
(Diff revision 4)
> +
> +NS_IMPL_ISUPPORTS(FileDescriptorFile, nsIFile)
> +
> +LazyLogModule gFDFileLog("FDFile");
> +#undef DBG
> +#define DBG(args) MOZ_LOG(gFDFileLog, mozilla::LogLevel::Debug, args)

Nit: No need for `mozilla::`

Also, please use __VA_ARGS__ for this.

::: xpcom/io/FileDescriptorFile.cpp:32
(Diff revision 4)
> +
> +FileDescriptorFile::FileDescriptorFile(const FileDescriptor& aFD,
> +                                       nsIFileURL* aFileURL)
> +{
> +  MOZ_ASSERT(aFD.IsValid());
> +  mFD = FileDescriptor(aFD.ClonePlatformHandle().get());

This isn't safe. You need to store the UniquePtr that ClonePlatformHandle returns and make sure it isn't released until after the FileDescriptor constructor completes. Same goes for below.

::: xpcom/io/FileDescriptorFile.cpp:40
(Diff revision 4)
> +  nsCOMPtr<nsIURI> uri;
> +  other.mFileURL->Clone(getter_AddRefs(uri));
> +  mFileURL = do_QueryInterface(uri);
> +  MOZ_ASSERT(mFileURL);

We shouldn't actually need to store a file URL at all. Please either accept a nsIFile directly or resolve the URL to a nsIFile in the constructor.

::: xpcom/io/FileDescriptorFile.cpp:53
(Diff revision 4)
> +nsresult
> +FileDescriptorFile::Init()
> +{
> +  nsresult rv;
> +
> +  if (!mFileURL) {

Please just assert that this is non-null in the constructor.

::: xpcom/io/FileDescriptorFile.cpp:72
(Diff revision 4)
> +//-----------------------------------------------------------------------------
> +// FileDescriptorFile::nsIFile functions that we override logic for
> +//-----------------------------------------------------------------------------
> +
> +NS_IMETHODIMP
> +FileDescriptorFile::Clone(nsIFile **file)

Please name this param something like `aFileOut`

::: xpcom/io/FileDescriptorFile.cpp:74
(Diff revision 4)
> +  *file = new FileDescriptorFile(*this);
> +  NS_ADDREF(*file);

This should be something like:

    RefPtr<FileDescriptorFile> fd = new FileDesctiptorFile(*this);
    fd.forget(aFileOut);

::: netwerk/protocol/res/ExtensionProtocolHandler.h:9
(Diff revision 8)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #ifndef ExtensionProtocolHandler_h___
>  #define ExtensionProtocolHandler_h___
>  
> +#include "mozilla/ipc/IPCStreamUtils.h"

This isn't used.

::: netwerk/protocol/res/ExtensionProtocolHandler.h:10
(Diff revision 8)
> +#include "mozilla/net/NeckoChild.h"
> +#include "mozilla/net/NeckoParent.h"

Please forward declare these rather than including.

::: netwerk/protocol/res/ExtensionProtocolHandler.h:13
(Diff revision 8)
>  
> +#include "mozilla/ipc/IPCStreamUtils.h"
> +#include "mozilla/net/NeckoChild.h"
> +#include "mozilla/net/NeckoParent.h"
> +#include "mozilla/LazyIdleThread.h"
> +#include "mozilla/UniquePtr.h"

This isn't used.

::: netwerk/protocol/res/ExtensionProtocolHandler.h:14
(Diff revision 8)
> +#include "nsIFileURL.h"
> +#include "nsIInputStreamPump.h"
> +#include "nsIJARChannel.h"
> +#include "nsISupports.h"

These aren't used.

::: netwerk/protocol/res/ExtensionProtocolHandler.h:20
(Diff revision 8)
> +#include "nsIJARChannel.h"
> +#include "nsISupports.h"
>  #include "SubstitutingProtocolHandler.h"
> -#include "nsWeakReference.h"
>  
> +#define EXTENSION_SCHEME_LITERAL "moz-extension"

`s/_LITERAL//`

::: netwerk/protocol/res/ExtensionProtocolHandler.h:22
(Diff revision 8)
>  #include "SubstitutingProtocolHandler.h"
> -#include "nsWeakReference.h"
>  
> +#define EXTENSION_SCHEME_LITERAL "moz-extension"
> +
> +using mozilla::ipc::FileDescriptor;

This isn't used, and in any case should not be in a public header.

::: netwerk/protocol/res/ExtensionProtocolHandler.h:39
(Diff revision 8)
>    NS_DECL_NSIPROTOCOLHANDLERWITHDYNAMICFLAGS
> +
>    NS_FORWARD_NSIPROTOCOLHANDLER(SubstitutingProtocolHandler::)
>    NS_FORWARD_NSISUBSTITUTINGPROTOCOLHANDLER(SubstitutingProtocolHandler::)
>  
> -  ExtensionProtocolHandler() : SubstitutingProtocolHandler("moz-extension") {}
> +  ExtensionProtocolHandler();

Please make this private.

::: netwerk/protocol/res/ExtensionProtocolHandler.h:48
(Diff revision 8)
> +   * a web accessible resource from an unpacked WebExtension dir.
> +   *
> +   * @param aChildURI a moz-extension URI sent from the child that refers
> +   *        to a web accessible resource file in an enabled unpacked extension
> +   * @param aChildLoadInfo the loadinfo for the request sent from the child
> +   * @param aInputStream the input stream out param for the requested URI

This should be `Result<RefPtr<nsIInputStream>, nsresult>` rather than having an inputStream out param.

::: netwerk/protocol/res/ExtensionProtocolHandler.h:49
(Diff revision 8)
> +   *
> +   * @param aChildURI a moz-extension URI sent from the child that refers
> +   *        to a web accessible resource file in an enabled unpacked extension
> +   * @param aChildLoadInfo the loadinfo for the request sent from the child
> +   * @param aInputStream the input stream out param for the requested URI
> +   * @param aTerminateSender out param set to true when the params are invalid

Please make this `NotNull<bool*>` or `bool&`.

Same for below.

::: netwerk/protocol/res/ExtensionProtocolHandler.h:92
(Diff revision 8)
> +  static ExtensionProtocolHandler* GetSingleton()
> +  {
> +    return sSingleton;
> +  }

Please have this return either a reference or an `already_AddRefed`.

::: netwerk/protocol/res/ExtensionProtocolHandler.h:109
(Diff revision 8)
> +  /**
> +   * For moz-extension URI's that resolve to file or JAR URI's, replaces
> +   * the provided channel with a channel that will proxy the load to the
> +   * parent process. For moz-extension URI's that resolve to other types
> +   * of URI's (not file or JAR), the provide channel is not replaced and
> +   * NS_OK is returned.
> +   *
> +   * @param aURI the moz-extension URI
> +   * @param aLoadInfo the loadinfo for the request
> +   * @param aResult in/out channel param referring to the channel that
> +   *        might need to be substituted with a remote channel.
> +   * @return NS_OK if the channel does not need to be substituted or
> +   *         or the replacement channel was created successfully.
> +   *         Otherwise returns an error.
> +   */
> +  Result<Ok, nsresult> SubstituteRemoteChannel(nsIURI* aURI,
> +                                               nsILoadInfo* aLoadInfo,
> +                                               nsIChannel** aResult);
> +
> +  void NewFileChannel(nsIURI* aURI,
> +                      nsILoadInfo* aLoadinfo,
> +                      nsACString& aResolvedFileSpec,
> +                      nsIChannel** aRetVal);
> +
> +  Result<Ok, nsresult> NewJarChannel(nsIURI* aURI,
> +                                     nsILoadInfo* aLoadinfo,
> +                                     nsACString& aResolvedSpec,
> +                                     nsIChannel** aRetVal);

Why are these protected rather than private?

::: netwerk/protocol/res/ExtensionProtocolHandler.h:133
(Diff revision 8)
> +  void NewFileChannel(nsIURI* aURI,
> +                      nsILoadInfo* aLoadinfo,
> +                      nsACString& aResolvedFileSpec,
> +                      nsIChannel** aRetVal);
> +
> +  Result<Ok, nsresult> NewJarChannel(nsIURI* aURI,

Please add comments describing these methods, and in particular explaining the `aRetVal` in/out param. And ideally rename them to make it clear that we're not just creating a new channel.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:82
(Diff revision 8)
> +/**
> + * Helper class used with SimpleChannel to asynchronously obtain an input
> + * stream or file descriptor from the parent for a remote moz-extension load
> + * from the child.
> + */
> +class ExtensionStreamGetter : public nsISupports

This doesn't need to implement `nsISupports`. Just inherit from `public RefCounted<ExtensionStreamGetter>` and add `MOZ_DECLARE_REFCOUNTED_TYPENAME(ExtensionStreamGetter)` to the class body.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:84
(Diff revision 8)
> + * stream or file descriptor from the parent for a remote moz-extension load
> + * from the child.
> + */
> +class ExtensionStreamGetter : public nsISupports
> +{
> +  typedef mozilla::ipc::OptionalIPCStream OptionalIPCStream;

Nit: `using OptionalIPCStream = mozilla::ipc::OptionalIPCStream;`

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:87
(Diff revision 8)
> +    // To use when getting a remote input stream for a resource
> +    // in an unpacked extension.
> +    ExtensionStreamGetter(nsIURI* aURI, nsILoadInfo* aLoadInfo);
> +
> +    // To use when getting an FD for a packed extension JAR file
> +    // in order to load a resource.
> +    ExtensionStreamGetter(nsIURI* aURI, nsILoadInfo* aLoadInfo,
> +                          already_AddRefed<nsIJARChannel>&& aJarChannel,
> +                          nsIFileURL* aJarFileURL);

Please declare these constructors inline. Also, please implement methods for each class before the next class declaration.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:102
(Diff revision 8)
> +    // Get an input stream or file descriptor from the parent asynchronously.
> +    Result<Ok, nsresult> GetAsync(nsIStreamListener* aListener,
> +                                  nsIChannel* aChannel);
> +
> +    // Handle an input stream being returned from the parent
> +    void OnStream(nsCOMPtr<nsIInputStream>& aStream);

`OnStream(nsIInputStream* aStream)`

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:117
(Diff revision 8)
> +    nsCOMPtr<nsIURI> mURI;
> +    nsCOMPtr<nsILoadInfo> mLoadInfo;
> +    nsCOMPtr<nsIJARChannel> mJarChannel;
> +    nsCOMPtr<nsIFileURL> mJarFileURL;
> +    nsCOMPtr<nsIStreamListener> mListener;
> +    nsCOMPtr<nsIInputStreamPump> mPump;

This doesn't need to be a member variable.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:175
(Diff revision 8)
> +  NS_IMETHOD Run() override
> +  {
> +    // First we are run off the main thread to open the file.
> +    if (!NS_IsMainThread()) {
> +      AutoFDClose prFileDesc;
> +      nsresult rv = mFile->OpenNSPRFileDesc(PR_RDONLY, 0, &prFileDesc.rwget());
> +      if (NS_SUCCEEDED(rv)) {
> +        mFD = FileDescriptor(FileDescriptor::PlatformHandleType(
> +                             PR_FileDesc2NativeHandle(prFileDesc)));
> +      }
> +
> +      rv = NS_DispatchToMainThread(this, nsIEventTarget::DISPATCH_NORMAL);
> +      NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "NS_DispatchToMainThread");
> +      return NS_OK;
> +    }
> +
> +    mResolve(mFD);
> +    return NS_OK;
> +  }

Please don't reuse the same runnable for two separate purposes. You can use a single class, and call different methods with `NewRunnableMethod`

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:261
(Diff revision 8)
> +        FileDescriptor nullFD;
> +        self->OnFD(nullFD);

`self->OnFD(FileDescriptor())`

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:280
(Diff revision 8)
> +      nsCOMPtr<nsIInputStream> nullInputStream;
> +      self->OnStream(nullInputStream);

`self->OnStream(nullptr)`

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:296
(Diff revision 8)
> +  nsCOMPtr<nsIStreamListener> listener;
> +  listener.swap(mListener);

`nsCOMPtr<nsIStreamListener> listener = mListener.forget()`

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:302
(Diff revision 8)
> +    // The parent didn't send us back a stream.
> +    aStream = new ErrorInputStream(NS_ERROR_FILE_ACCESS_DENIED);

This input stream isn't necessary. Just call the listener's `OnStartRequest` and `OnStopRequest` listeners directly, and pass the appropriate error to `OnStopRequest`.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:456
(Diff revision 8)
> +    auto subResult = SubstituteRemoteChannel(aURI, aLoadInfo, result);
> +    if (subResult.isErr()) {
> +      return subResult.unwrapErr();
> +    }

`MOZ_TRY(SubstituteRemoteChannel(aURI, aLoadInfo, result))`

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:575
(Diff revision 8)
> +                       nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL,
> +                       nsIContentPolicy::TYPE_OTHER));

These need to come from the original loadInfo.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:679
(Diff revision 8)
> +      if (getter->GetAsync(listener, channel).isOk()) {
> +        return RequestOrReason(nullptr);
> +      }
> +      return Err(NS_ERROR_UNEXPECTED);

This should be something like:

    MOZ_TRY(getter->GetAsync(listener, channel);
    return nullptr;

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:710
(Diff revision 8)
> +  nsCOMPtr<nsIIOService> ioService = do_GetIOService(&rv);
> +  NS_TRY(rv);

This isn't used.

Comment 95

[Jim Mathies [:jimm]]

Comment on attachment 8872427 [details]
Bug 1334550 - Part 3 - Workaround for addon.uninstall() file locking issue;

https://reviewboard.mozilla.org/r/143928/#review155704

Comment 96

[Haik Aftandilian [:haik]]

Comment on attachment 8869275 [details]
Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent process;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140834/diff/9-10/

Comment 97

[Haik Aftandilian [:haik]]

Comment on attachment 8869276 [details]
Bug 1334550 - Part 2 - Add nsISubstitutionObserver and update test_extensionURL.html;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140836/diff/9-10/

Comment 98

[Haik Aftandilian [:haik]]

Comment on attachment 8872427 [details]
Bug 1334550 - Part 3 - Workaround for addon.uninstall() file locking issue;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143928/diff/5-6/

Comment 99

[Haik Aftandilian [:haik]]

Comment on attachment 8869275 [details]
Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent process;

https://reviewboard.mozilla.org/r/140834/#review147178

> We shouldn't actually need to store a file URL at all. Please either accept a nsIFile directly or resolve the URL to a nsIFile in the constructor.

I made the constructor accept an nsIFile instead of a file URL and, as a result, mFileURL is no longer needed.

> Please just assert that this is non-null in the constructor.

Init method no longer needed.

> Please forward declare these rather than including.

I didn't need NeckoChild.h any more, but NeckoParent is needed for the IPDL promise resolver NeckoParent::GetExtensionFDResolver.

> Please make this private.

I may have misunderstood what you meant here. The ExtensionProtocolHandler still needs to be public.

> This should be `Result<RefPtr<nsIInputStream>, nsresult>` rather than having an inputStream out param.

Used Result<nsCOMPtr<nsIInputStream, nsresult>

> These need to come from the original loadInfo.

I'll ask Christoph (:ckerschb) about this (and the principal). The loadinfo is sent from the child process and therefore untrusted. The directory check should ensure the file is within the extension directory, but I don't know if a malicious loadinfo or principal could cause problems.

Comment 100

[Haik Aftandilian [:haik]]

>> These need to come from the original loadInfo.
> I'll ask Christoph (:ckerschb) about this (and the principal).
> The loadinfo is sent from the child process and therefore untrusted.
> The directory check should ensure the file is within the extension
> directory, but I don't know if a malicious loadinfo or principal
> could cause problems.

Christoph, do you have a recommendation on the parameters to NS_NewChannel here? (This is similar to my earlier question you answered in comment 23, but the code has changed so this case is slightly different.) The context is that we're executing in the parent process using a loadinfo provided by the child process. The URI provided by the child is a moz-extension URI. We call NS_NewChannel which will get a channel that we will later confirm is a file channel for a file in the appropriate extension directory. Is it safe to use principal and loadinfo flags provided by the child? It seems as though, regardless of the loadinfo, as long as we can't end up using a file outside of the extension directory, we should be OK.

  ExtensionProtocolHandler.cpp:
   ...
   529   nsCOMPtr<nsIChannel> channel;
   530   NS_TRY(NS_NewChannel(getter_AddRefs(channel),
   531                        aChildURI,
   532                        childPrincipal,
   533                        nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL,
   534                        nsIContentPolicy::TYPE_OTHER));

Comment 101

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Haik Aftandilian [:haik] from comment #100)
> Christoph, do you have a recommendation on the parameters to NS_NewChannel
> here?

In general we should not trust the child to provide any of those arguments, because the child might be compromised. I see that you are already performing a check that aChildURI has an EXTENSION_SCHEME, which is good. I am still a little worried that the principal might be a systemPrincipal which has more privileges than other principals. Can you also return an error it the principal is a systemPrincipal? Something like:

if (nsContentUtils::IsSystemPrincipal(childPrincipal) {
  return some error;
}

Further, I think you could use NS_NewChannelInternal(..., loadInfo) to simplify things, but you should still perform that check on the principal in the loadInfo.

Comment 102

[Haik Aftandilian [:haik]]

Comment on attachment 8869275 [details]
Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent process;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140834/diff/10-11/

Comment 103

[Haik Aftandilian [:haik]]

Comment on attachment 8869276 [details]
Bug 1334550 - Part 2 - Add nsISubstitutionObserver and update test_extensionURL.html;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140836/diff/10-11/

Comment 104

[Haik Aftandilian [:haik]]

Comment on attachment 8872427 [details]
Bug 1334550 - Part 3 - Workaround for addon.uninstall() file locking issue;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143928/diff/6-7/

Comment 105

[Haik Aftandilian [:haik]]

Comment on attachment 8869275 [details]
Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent process;

https://reviewboard.mozilla.org/r/140834/#review147178

> I'll ask Christoph (:ckerschb) about this (and the principal). The loadinfo is sent from the child process and therefore untrusted. The directory check should ensure the file is within the extension directory, but I don't know if a malicious loadinfo or principal could cause problems.

Updated the code so that 1) it checks that the principal from the child is not the system principal and 2) now uses NS_NewChannelInternal(). Since child requests that use the system principal are dropped, the question is should the child ever need to use the system principal to load a moz-extension URI. We don't think that should ever be needed.

Comment 106

[Haik Aftandilian [:haik]]

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

Comment 107

[Kris Maglione [:kmag]]

Comment on attachment 8869275 [details]
Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent process;

https://reviewboard.mozilla.org/r/140834/#review147178

> I made the constructor accept an nsIFile instead of a file URL and, as a result, mFileURL is no longer needed.

Yeah, sorry, these comments were for a previous version of the patch, but I guess mozreview submitted them along with the latest version.

> I didn't need NeckoChild.h any more, but NeckoParent is needed for the IPDL promise resolver NeckoParent::GetExtensionFDResolver.

Ah, I didn't realize that was a child class. Hm. OK.

> I may have misunderstood what you meant here. The ExtensionProtocolHandler still needs to be public.

The constructor can be private. The `GetSingleton` method should construct the singleton instance the first time it's called.

Comment 108

[Jim Mathies [:jimm]]

(In reply to Haik Aftandilian [:haik] from comment #106)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=492829c7a5455564ea3e52ace8ba7b8d092e887c

Those osx build errors are infra I believe. We had similar problems with nightlies this morning.

Comment 109

[Haik Aftandilian [:haik]]

(In reply to Jim Mathies [:jimm] from comment #108)
> (In reply to Haik Aftandilian [:haik] from comment #106)
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=492829c7a5455564ea3e52ace8ba7b8d092e887c
> 
> Those osx build errors are infra I believe. We had similar problems with
> nightlies this morning.

Yeah, the logs show it failed to sign the dmg. A later push had a clean OS X opt build:

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

Comment 110

[Jim Mathies [:jimm]]

Comment on attachment 8869275 [details]
Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent process;

https://reviewboard.mozilla.org/r/140834/#review156378

::: modules/libjar/nsJARChannel.cpp:350
(Diff revision 11)
>      // does basic escaping by default, which breaks reading zipped files which
>      // have e.g. spaces in their filenames.
>      NS_UnescapeURL(mJarEntry);
>  
> +    if (mJarFileOverride) {
> +      mJarFile = mJarFileOverride;

nit - indentation in this block is off

::: netwerk/ipc/NeckoParent.cpp:986
(Diff revision 11)
> +  MOZ_ASSERT(ph);
> +
> +  // Ask the ExtensionProtocolHandler to give us a new input stream for
> +  // this URI. The request comes from an ExtensionProtocolHandler in the
> +  // child process, but is not guaranteed to be a valid moz-extension URI,
> +  // and not guaranteed to represent a resources that the child should be

nit - s/resources/resource

::: netwerk/protocol/res/ExtensionProtocolHandler.h:60
(Diff revision 11)
> +                                                       nsILoadInfo* aChildLoadInfo,
> +                                                       bool& aTerminateSender);
> +
> +  /**
> +   * To be called in the parent process to obtain a file descriptor for an
> +   * enabled WebExtension JAR file.

Curious, why do you call out enabled extensions here? This seems odd, but maybe there's a reason you explicitly call this out?

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:90
(Diff revision 11)
> +/**
> + * Helper class used with SimpleChannel to asynchronously obtain an input
> + * stream or file descriptor from the parent for a remote moz-extension load
> + * from the child.
> + */
> +class ExtensionStreamGetter : public RefCounted<ExtensionStreamGetter>

Lets add MOZ_ASSERT on the incoming pointers for constructors here. Same with ExtensionJARFileOpener.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:139
(Diff revision 11)
> +    nsCOMPtr<nsIStreamListener> mListener;
> +    nsCOMPtr<nsIChannel> mChannel;
> +    bool mIsJarChannel;
> +};
> +
> +//NS_IMPL_ISUPPORTS(ExtensionStreamGetter, nsISupports)

nit - remove me

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:377
(Diff revision 11)
> -WrapNSResult(nsresult aRv)
> +Result<Ok, nsresult>
> +ExtensionProtocolHandler::SubstituteRemoteChannel(nsIURI* aURI,
> +                                                  nsILoadInfo* aLoadInfo,
> +                                                  nsIChannel** aRetVal)
>  {
> -  if (NS_FAILED(aRv)) {
> +  MOZ_ASSERT(IsNeckoChild());

nit - MOZ_ASSERTs here on incoming pointers

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:475
(Diff revision 11)
> +Result<nsCOMPtr<nsIInputStream>, nsresult>
> +ExtensionProtocolHandler::NewStream(nsIURI* aChildURI,
> +                                    nsILoadInfo* aChildLoadInfo,
> +                                    bool& aTerminateSender)
> +{
> +  MOZ_ASSERT(!IsNeckoChild());

nit - assert incoming pointers

::: xpcom/io/FileDescriptorFile.cpp:37
(Diff revision 11)
> +  auto platformHandle = aFD.ClonePlatformHandle();
> +  mFD = FileDescriptor(platformHandle.get());
> +  mFile = aFile;
> +}
> +
> +FileDescriptorFile::FileDescriptorFile(const FileDescriptorFile& other)

nit - aOther

Comment 111

[Haik Aftandilian [:haik]]

Comment on attachment 8869275 [details]
Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent process;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140834/diff/11-12/

Comment 112

[Haik Aftandilian [:haik]]

Comment on attachment 8869276 [details]
Bug 1334550 - Part 2 - Add nsISubstitutionObserver and update test_extensionURL.html;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140836/diff/11-12/

Comment 113

[Haik Aftandilian [:haik]]

Comment on attachment 8872427 [details]
Bug 1334550 - Part 3 - Workaround for addon.uninstall() file locking issue;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143928/diff/7-8/

Comment 114

[Haik Aftandilian [:haik]]

Comment on attachment 8869275 [details]
Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent process;

https://reviewboard.mozilla.org/r/140834/#review156378

Thanks. I've updated patch 1 to address your comments. I've also changed the ExtensionProtocolHandler constructor to be private which required a small change to netwerk/build/nsNetModule.cpp.

> Curious, why do you call out enabled extensions here? This seems odd, but maybe there's a reason you explicitly call this out?

I wanted to make it clear that if the extension isn't enabled, these methods can't be used to load a resource from the JAR file (or directory for the unpacked case). That's because when the extension isn't enabled, its UUID mapping is not present in the substitution table in the protocol handler and that will result in NewFD or NewStream returning an error when the GetSubstitution() call fails.

Comment 115

[Jim Mathies [:jimm]]

Comment on attachment 8869276 [details]
Bug 1334550 - Part 2 - Add nsISubstitutionObserver and update test_extensionURL.html;

https://reviewboard.mozilla.org/r/140836/#review156462

Kinda sucks we have to add all this just for tests, but I guess making sure this code work properly is worth it. 

lgtm

::: caps/tests/mochitest/test_extensionURL.html:29
(Diff revision 11)
>    var extensionHandler = SpecialPowers.Services.io.getProtocolHandler("moz-extension")
>                                       .QueryInterface(SpecialPowers.Ci.nsISubstitutingProtocolHandler);
> +  var fileHandler = SpecialPowers.Cc["@mozilla.org/network/protocol;1?name=file"]
> +                                  .getService(SpecialPowers.Ci.nsIFileProtocolHandler);
> +
> +  var script = SpecialPowers.loadChromeScript(() => {

Might be good to add a comment header here calling this out as a chrome side swcript. Just feels like it should stand out for that.

::: caps/tests/mochitest/test_extensionURL.html:80
(Diff revision 11)
> +        pendingSubstitutions.splice(i, 1);
> +        return;
> +      }
> +    }
> +    // This indicates we installed a mapping in either the
> +    // parent of local child process, but never received an

nit - s/of/or

Comment 116

[Kris Maglione [:kmag]]

Comment on attachment 8869275 [details]
Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent process;

https://reviewboard.mozilla.org/r/140834/#review156468

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:298
(Diff revision 12)
> +    nsCOMPtr<nsIInputStream> nullStream;
> +    OnStream(nullStream);

`OnStream(nullptr)`

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:305
(Diff revision 12)
> +  nsCOMPtr<nsIStreamListener> listener;
> +  listener.swap(mListener);

`nsCOMPtr<nsIStreamListener> listener = mListener.forget()`

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:308
(Diff revision 12)
> +  // We must keep an owning reference to the listener
> +  // until we pass it on to AsyncOpen2.
> +  nsCOMPtr<nsIStreamListener> listener;
> +  listener.swap(mListener);
> +
> +  FileDescriptorFile *fdFile = new FileDescriptorFile(aFD, mJarFile);

Two issues: 1) the C++ style guide requires the `*` to be attached to the type rather than the variable name, and 2) please use a RefPtr rather than a raw pointer for this.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:329
(Diff revision 12)
> +{
> +  if (!sSingleton) {
> +    sSingleton = new ExtensionProtocolHandler();
> +    ClearOnShutdown(&sSingleton);
> +  }
> +  return do_AddRef(sSingleton.get());

Nit: No need for `.get()`

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:333
(Diff revision 12)
> +  }
> +  return do_AddRef(sSingleton.get());
> +}
> +
> +ExtensionProtocolHandler::ExtensionProtocolHandler() :
> +  SubstitutingProtocolHandler(EXTENSION_SCHEME)

Nit: The `:` belongs on the same line as the first initializer.

Comment 117

[Haik Aftandilian [:haik]]

Comment on attachment 8869275 [details]
Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent process;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140834/diff/12-13/

Comment 118

[Haik Aftandilian [:haik]]

Comment on attachment 8869276 [details]
Bug 1334550 - Part 2 - Add nsISubstitutionObserver and update test_extensionURL.html;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140836/diff/12-13/

Comment 119

[Haik Aftandilian [:haik]]

Comment on attachment 8872427 [details]
Bug 1334550 - Part 3 - Workaround for addon.uninstall() file locking issue;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143928/diff/8-9/

Comment 120

[Haik Aftandilian [:haik]]

Comment on attachment 8869275 [details]
Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent process;

https://reviewboard.mozilla.org/r/140834/#review156468

> Nit: No need for `.get()`

We need the get() because sSingleton is a StaticRefPtr.

Comment 121

[Haik Aftandilian [:haik]]

Comment on attachment 8869276 [details]
Bug 1334550 - Part 2 - Add nsISubstitutionObserver and update test_extensionURL.html;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140836/diff/13-14/

Comment 122

[Haik Aftandilian [:haik]]

Comment on attachment 8872427 [details]
Bug 1334550 - Part 3 - Workaround for addon.uninstall() file locking issue;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143928/diff/9-10/

Comment 123

[Haik Aftandilian [:haik]]

Comment on attachment 8869275 [details]
Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent process;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140834/diff/13-14/

Comment 124

[Haik Aftandilian [:haik]]

Comment on attachment 8869276 [details]
Bug 1334550 - Part 2 - Add nsISubstitutionObserver and update test_extensionURL.html;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140836/diff/14-15/

Comment 125

[Haik Aftandilian [:haik]]

Comment on attachment 8872427 [details]
Bug 1334550 - Part 3 - Workaround for addon.uninstall() file locking issue;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143928/diff/10-11/

Comment 126

[Haik Aftandilian [:haik]]

Honza, could you confirm you're OK with this landing? I've made some changes since you reviewed. Thanks.

Comment 127

[Honza Bambas (:mayhemer)]

Comment on attachment 8869275 [details]
Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent process;

https://reviewboard.mozilla.org/r/140834/#review156778

::: netwerk/protocol/res/ExtensionProtocolHandler.h:50
(Diff revision 14)
> +   *         in the extension and doesn't result in |aTerminateSender| being
> +   *         set to true.
> +   */
> +  Result<nsCOMPtr<nsIInputStream>, nsresult> NewStream(nsIURI* aChildURI,
> +                                                       nsILoadInfo* aChildLoadInfo,
> +                                                       bool& aTerminateSender);

out params are generally passed by pointer, not reference.

::: netwerk/protocol/res/ExtensionProtocolHandler.h:82
(Diff revision 14)
>  
>  protected:
>    ~ExtensionProtocolHandler() {}
>  
> +private:
> +  ExtensionProtocolHandler();

make it explicit?

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:173
(Diff revision 14)
> +
> +    nsCOMPtr<nsIRunnable> event =
> +      mozilla::NewRunnableMethod("ExtensionJarFileFDResolver",
> +        this, &ExtensionJARFileOpener::SendBackFD);
> +
> +    rv = NS_DispatchToMainThread(event, nsIEventTarget::DISPATCH_NORMAL);

please file a followup bug to use a labeled target for this instead.

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:285
(Diff revision 14)
> +  }
> +
> +  rv = pump->AsyncRead(listener, nullptr);
> +  if (NS_FAILED(rv)) {
> +    mChannel->Cancel(NS_BINDING_ABORTED);
> +    return;

this return; is useless

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:393
(Diff revision 14)
> +                                                  nsILoadInfo* aLoadInfo,
> +                                                  nsIChannel** aRetVal)
>  {
> -  if (NS_FAILED(aRv)) {
> -    return Err(aRv);
> -  }
> +  MOZ_ASSERT(IsNeckoChild());
> +  MOZ_ASSERT(aURI);
> +  MOZ_ASSERT(aLoadInfo);

rather let this method fail when these are missing

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:493
(Diff revision 14)
> +                                    nsILoadInfo* aChildLoadInfo,
> +                                    bool& aTerminateSender)
> +{
> +  MOZ_ASSERT(!IsNeckoChild());
> +  MOZ_ASSERT(aChildURI);
> +  MOZ_ASSERT(aChildLoadInfo);

fail on missing these...

Comment 128

[Honza Bambas (:mayhemer)]

(comment 127)

Comment 129

[Haik Aftandilian [:haik]]

Comment on attachment 8869275 [details]
Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent process;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140834/diff/14-15/

Comment 130

[Haik Aftandilian [:haik]]

Comment on attachment 8869276 [details]
Bug 1334550 - Part 2 - Add nsISubstitutionObserver and update test_extensionURL.html;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140836/diff/15-16/

Comment 131

[Haik Aftandilian [:haik]]

Comment on attachment 8872427 [details]
Bug 1334550 - Part 3 - Workaround for addon.uninstall() file locking issue;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143928/diff/11-12/

Comment 132

[Haik Aftandilian [:haik]]

Comment on attachment 8869275 [details]
Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent process;

https://reviewboard.mozilla.org/r/140834/#review156778

> out params are generally passed by pointer, not reference.

Another review comment requested this be turned into a reference. It's uncommon, but better because we don't need NULL checks.

> make it explicit?

Done.

> please file a followup bug to use a labeled target for this instead.

I will be filing a follow up bug to include a few other things and will include this.

> this return; is useless

Removed it.

> rather let this method fail when these are missing

Done.

> fail on missing these...

Done.

Comment 133

[Honza Bambas (:mayhemer)]

(In reply to Haik Aftandilian [:haik] from comment #132)
> Comment on attachment 8869275 [details]
> Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent
> process;
> 
> https://reviewboard.mozilla.org/r/140834/#review156778
> 
> > out params are generally passed by pointer, not reference.
> 
> Another review comment requested this be turned into a reference. It's
> uncommon, but better because we don't need NULL checks.

It's a style requirement to make out params like these be passed by ptr.  It's more readable in the code it's an outparam then.

Comment 134

[Haik Aftandilian [:haik]]

(In reply to Honza Bambas (:mayhemer) from comment #133)
> (In reply to Haik Aftandilian [:haik] from comment #132)
> > Comment on attachment 8869275 [details]
> > Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent
> > process;
> > 
> > https://reviewboard.mozilla.org/r/140834/#review156778
> > 
> > > out params are generally passed by pointer, not reference.
> > 
> > Another review comment requested this be turned into a reference. It's
> > uncommon, but better because we don't need NULL checks.
> 
> It's a style requirement to make out params like these be passed by ptr. 
> It's more readable in the code it's an outparam then.

Sorry, I didn't realize that. I will fix before pushing.

The guide states "Use pointers instead of references for function out parameters, even for primitive types."

Comment 135

[Haik Aftandilian [:haik]]

Comment on attachment 8869275 [details]
Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent process;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140834/diff/15-16/

Comment 136

[Haik Aftandilian [:haik]]

Comment on attachment 8869276 [details]
Bug 1334550 - Part 2 - Add nsISubstitutionObserver and update test_extensionURL.html;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140836/diff/16-17/

Comment 137

[Haik Aftandilian [:haik]]

Comment on attachment 8872427 [details]
Bug 1334550 - Part 3 - Workaround for addon.uninstall() file locking issue;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143928/diff/12-13/

Comment 138

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4319f15e2496
Part 1 - Proxy moz-extension protocol requests to the parent process; r=jimm,mayhemer
https://hg.mozilla.org/integration/autoland/rev/d3b0db4e50c5
Part 2 - Add nsISubstitutionObserver and update test_extensionURL.html; r=jimm
https://hg.mozilla.org/integration/autoland/rev/17c74c7634a2
Part 3 - Workaround for addon.uninstall() file locking issue; r=jimm

Comment 139

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/7e2c3de97685 for Windows timeouts in test-oop-extensions/test_ext_unlimitedStorage.html, e.g. https://treeherder.mozilla.org/logviewer.html#?job_id=109381411&repo=autoland

Comment 140

[Haik Aftandilian [:haik]]

Comment on attachment 8869275 [details]
Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent process;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140834/diff/16-17/

Comment 141

[Haik Aftandilian [:haik]]

Comment on attachment 8869276 [details]
Bug 1334550 - Part 2 - Add nsISubstitutionObserver and update test_extensionURL.html;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140836/diff/17-18/

Comment 142

[Haik Aftandilian [:haik]]

Comment on attachment 8869275 [details]
Bug 1334550 - Part 1 - Proxy moz-extension protocol requests to the parent process;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140834/diff/17-18/

Comment 143

[Haik Aftandilian [:haik]]

Comment on attachment 8869276 [details]
Bug 1334550 - Part 2 - Add nsISubstitutionObserver and update test_extensionURL.html;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140836/diff/18-19/

Comment 144

[Haik Aftandilian [:haik]]

(In reply to Phil Ringnalda (:philor) from comment #139)
> Backed out in https://hg.mozilla.org/integration/autoland/rev/7e2c3de97685
> for Windows timeouts in test-oop-extensions/test_ext_unlimitedStorage.html,
> e.g.
> https://treeherder.mozilla.org/logviewer.html#?job_id=109381411&repo=autoland

The problem causing these text_ext_unlimitedStorage*.html tests to fail was the same problem I was attempting to workaround in patch 3 (now removed). The failure was that the parent process can't move an addon JAR to the special trash dir in order to uninstall it because the file is locked. This failure was only occurring on Windows.

Uninstalling an addon triggers a JAR-cache-flush in the parent and child processes before the extension files are moved to a trash directory, but the parent doesn't wait for the child JAR cache to have closed file descriptors before continuing. And the remoting code doesn't open the file using the Windows FILE_SHARE_DELETE flag when it opens the file to get the FD to send to the child. As a result, when the file is re-opened later to be moved, we sometimes hit a failure due to the file being locked. The fix is to open the file using FILE_SHARE_DELETE.

These failures weren't hit in my earlier test runs because the tests landed just before the Autoland push.

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

Comment 145

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/69cb0b4c0b2b
Part 1 - Proxy moz-extension protocol requests to the parent process; r=jimm,mayhemer
https://hg.mozilla.org/integration/autoland/rev/3358bef7a047
Part 2 - Add nsISubstitutionObserver and update test_extensionURL.html; r=jimm

Comment 146

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/69cb0b4c0b2b
https://hg.mozilla.org/mozilla-central/rev/3358bef7a047

Comment 147

[Jason Duell]

Does this patch do main thread I/O on the parent?  My guess from looking at NeckoParent::RecvGetExtensionStream() is that it does:  it calls ExtensionProtocolHandler::NewStream(), which does stuff like IsDirectory(), which calls into GetFileAttributesW() (I only looked at the Windows code path--I assume that's a blocking I/O call?).  The call to extensionDir->Contains() also seems a likely suspect.

Comment 148

[Kris Maglione [:kmag]]

(In reply to Jason Duell [:jduell] (needinfo me) from comment #147)
> Does this patch do main thread I/O on the parent?  My guess from looking at
> NeckoParent::RecvGetExtensionStream() is that it does:  it calls
> ExtensionProtocolHandler::NewStream(), which does stuff like IsDirectory(),
> which calls into GetFileAttributesW() (I only looked at the Windows code
> path--I assume that's a blocking I/O call?).  The call to
> extensionDir->Contains() also seems a likely suspect.

Contains() doesn't do IO, but Normalize() does (at least on Unix-ish systems).

Comment 149

[Haik Aftandilian [:haik]]

(In reply to Jason Duell [:jduell] (needinfo me) from comment #147)
> Does this patch do main thread I/O on the parent?  My guess from looking at
> NeckoParent::RecvGetExtensionStream() is that it does:  it calls
> ExtensionProtocolHandler::NewStream(), which does stuff like IsDirectory(),
> which calls into GetFileAttributesW() (I only looked at the Windows code
> path--I assume that's a blocking I/O call?).  The call to
> extensionDir->Contains() also seems a likely suspect.

It does do some I/O on the main thread for the filesystem checks, but only for the unpacked (development) extension case. The common case is a JAR load which uses ExtensionProtocolHandler::NewFD() which doesn't do I/O on the main thread. I'll look into doing this off the main thread. I'm working on some follow-ups in Bug 1376496 and will either tackle this in 1376496 or file a new bug for it.

FYI, for Windows, Normalize() doesn't resolve symlinks, but bug 1369670 just landed with WinUtils::ResolveJunctionPointsAndSymLinks(nsIFile) which I'm using in the follow-up patches.

Comment 150

[Thomas Wisniewski]

This seems to have broken temporarily loading an unpacked WebExtension (according to mozregression). For instance, in my manifest:

  "content_scripts": [{
    "js": ["content.js"],
    "matches": ["<all_urls>"]
  }]

And with the content.js just being a file with "use strict;" at the top, when I visit google, I see this in the browser console:

>uncaught exception: Unable to load script: moz-extension://29f0b3a1-1f92-cc4c-a525-34171074b4f9/content.js

Comment 151

[Andy McKay]

I was able to reproduce this using beastify when loading from web-ext. Making it into an .xpi and installing through about:addons worked just fine.

Comment 152

[Haik Aftandilian [:haik]]

(In reply to Thomas Wisniewski from comment #150)
> This seems to have broken temporarily loading an unpacked WebExtension
> (according to mozregression). For instance, in my manifest:
> 
>   "content_scripts": [{
>     "js": ["content.js"],
>     "matches": ["<all_urls>"]
>   }]
> 
> And with the content.js just being a file with "use strict;" at the top,
> when I visit google, I see this in the browser console:
> 
> >uncaught exception: Unable to load script: moz-extension://29f0b3a1-1f92-cc4c-a525-34171074b4f9/content.js

Please file a bug. Looking into this now.

Comment 153

[Haik Aftandilian [:haik]]

I did a quick test and don't think this was introduced by the fix for this bug.

With the following revision from mozilla-central, the unpacked beastify extension works correctly for me.

  $ hg parent
  changeset:   366154:3358bef7a047
  user:        Haik Aftandilian <[e-mail]>
  date:        Wed Jun 21 16:13:23 2017 -0700
  summary:     Bug 1334550 - Part 2 - Add nsISubstitutionObserver and update test_extensionURL.html; r=jimm

However, settings pref extensions.webextensions.protocol.remote=false (and restarting the browser) did work for me as a workaround.

I'll try to narrow down what introduced the problem tonight.

Comment 154

[Thomas Wisniewski]

> Please file a bug.

Sure, I just filed bug 1377355.

Comment 155

[Haik Aftandilian [:haik]]

(In reply to Haik Aftandilian [:haik] from comment #153)
> I did a quick test and don't think this was introduced by the fix for this
> bug.

Correction: this problem was introduced by the fix for this bug. My test used artifact builds which weren't reliable in this situation.

The culprit is the principal check added in ExtensionProtocolHandler::NewStream().

  nsCOMPtr<nsIPrincipal> childPrincipal;
  NS_TRY(aChildLoadInfo->GetLoadingPrincipal(getter_AddRefs(childPrincipal)));
  if (nsContentUtils::IsSystemPrincipal(childPrincipal)) {
    return Err(NS_ERROR_FILE_ACCESS_DENIED);
  }