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.

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.

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

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

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

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?

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

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

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.

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.

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.

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

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

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

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 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 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 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 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 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 on attachment 8862184 [details] Bug 1334550 - Part 3 - Open extension JAR files off the main thread; https://reviewboard.mozilla.org/r/115848/#review138514

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

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

(In reply to Andrea Marchesini [:baku] from comment #35) Thanks baku for these details!

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

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

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

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

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

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.

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.

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

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

Bad NI... sorry.

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.

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

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

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

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

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

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 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?

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

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

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\test-webext-debugging-reload@test.mozilla.com.xpi 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\test-webext-debugging-reload@test.mozilla.com.xpi 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]

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

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 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 on attachment 8872427 [details] Bug 1334550 - Part 3 - Workaround for addon.uninstall() file locking issue; https://reviewboard.mozilla.org/r/143928/#review155704

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.

>> 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));

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

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

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

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

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 127)

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.

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

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

Pushed by haftandilian@mozilla.com: 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

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

(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

Pushed by haftandilian@mozilla.com: 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

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

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.

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

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

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

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.

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

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 <haftandilian@mozilla.com> 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.

> Please file a bug. Sure, I just filed bug 1377355.

(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); }