This bug covers some follow-up work to bug 1334550. 1) Change the child process to not request an FD from the parent process when the JAR is already in the child's JAR cache. 2) Use a labeled target for the offloading used in the parent process to open JAR files off the main thread to get a file descriptor. 3) Make the added security checks (where we make sure an unpacked moz-extension load returns a file that is within the extension directory) also apply to regular moz-extension code paths in the parent. 4) Make FileDescriptorFile.{cpp,h} conform to the Mozilla coding style, specifically argument names.

5) Make use of WinUtils::ResolveJunctionPointsAndSymLinks() in the directory security check on Windows.

(In reply to Haik Aftandilian [:haik] from comment #0) > 2) Use a labeled target for the offloading used in the parent process to > open JAR files off the main thread to get a file descriptor. I'm dropping this item. Per IRC discussion with :kershaw about this one, we only need to use labelled runnables for dispatching tasks in the child process. In ExtensionProtocolHandler, the task dispatching is only used in the parent process because that's where we open JAR files to get their file descriptor.

Will address bug 1377569 "moz-extension remoting doesn't need to pass LoadInfos to the parent" with this bug too.

Comment on attachment 8887365 [details] Bug 1376496 - Part 5 - Don't send child nsILoadInfo's to the parent for requests. https://reviewboard.mozilla.org/r/158206/#review163478 thanks,r=me ::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:599 (Diff revision 1) > */ > > nsCOMPtr<nsIChannel> channel; > - NS_TRY(NS_NewChannelInternal(getter_AddRefs(channel), > + NS_TRY(NS_NewChannel(getter_AddRefs(channel), > - aChildURI, > + aChildURI, > - aChildLoadInfo)); > + nsContentUtils::GetSystemPrincipal(), Now that we have checked it's a EXTENSION_SCHEME and that the it is a valid directory we can use systemprincipal.

Comment on attachment 8887361 [details] Bug 1376496 - Part 1 - Resolve Windows symlinks in unpacked load security check. https://reviewboard.mozilla.org/r/158198/#review163890

Comment on attachment 8887364 [details] Bug 1376496 - Part 4 - FileDescriptorFile.cpp coding style fixes. https://reviewboard.mozilla.org/r/158204/#review163892

Comment on attachment 8887362 [details] Bug 1376496 - Part 2 - Don't request FD from parent when JAR file is cached. https://reviewboard.mozilla.org/r/158200/#review164694 ::: modules/libjar/nsIZipReader.idl:244 (Diff revision 1) > + * error NS_ERROR_CACHE_KEY_NOT_FOUND rather than creating a new reader. > + * > + * @note If someone called close() on the shared nsIZipReader, this method > + * will return the closed zip reader. > + */ > + nsIZipReader getZipFailOnMiss(in nsIFile zipFile); I'd slightly update the name: getZipIfExists. ::: modules/libjar/nsJARChannel.cpp:276 (Diff revision 1) > return rv; > } > > nsCOMPtr<nsIZipReader> reader; > - if (jarCache) { > + if (mJarReaderOverride) { > + reader = mJarReaderOverride; indent 4 spaces ::: modules/libjar/nsJARChannel.cpp:917 (Diff revision 1) > + > +NS_IMETHODIMP > +nsJARChannel::SetJarReader(nsIZipReader *aJarReader) > +{ > + if (mOpened) { > + return NS_ERROR_IN_PROGRESS; I think we have NS_ERROR_ALREADY_OPEN but, this one is probably OK too. ::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:801 (Diff revision 1) > + NS_TRY(jarHandler->GetJARCache(getter_AddRefs(jarCache))); > + if (jarCache) { > + nsCOMPtr<nsIZipReader> reader; > + rv = jarCache->GetZipFailOnMiss(jarFile, getter_AddRefs(reader)); > + if (NS_SUCCEEDED(rv)) { > + jarChannel->SetJarReader(reader); why can't the jar channel do this internally itself? or do you want to isolate this change only to the web extension code?

Comment on attachment 8887362 [details] Bug 1376496 - Part 2 - Don't request FD from parent when JAR file is cached. https://reviewboard.mozilla.org/r/158200/#review164742 ::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:801 (Diff revision 1) > + NS_TRY(jarHandler->GetJARCache(getter_AddRefs(jarCache))); > + if (jarCache) { > + nsCOMPtr<nsIZipReader> reader; > + rv = jarCache->GetZipFailOnMiss(jarFile, getter_AddRefs(reader)); > + if (NS_SUCCEEDED(rv)) { > + jarChannel->SetJarReader(reader); I just hadn't thought of that approach. I'll try that. I think I could just add a new method JARChannel::EnsureCached(bool \*aIsCached) that would do this internally and return whether or not the JAR is cached. I wouldn't change the default caching behavior when this new method isn't used. I do want to minimize changes to JAR code, but we're already changing JARChannel here and this change would be small. Thanks.

Comment on attachment 8887363 [details] Bug 1376496 - Part 3 - MOZ_LOG JAR cache hits/misses in DEBUG. https://reviewboard.mozilla.org/r/158202/#review164736 I just don't understand why this should only be available in debug builds. I really don't like when logs are not consistent this way. Make it release too, use the widely used LOG macro instead of DBG. ::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:829 (Diff revision 1) > + nsAutoCString uriSpec, jarFile; > + Unused << aURI->GetSpec(uriSpec); > + Unused << innerFileURI->GetSpec(jarFile); > + DBG("Cache MISS: %s (%s)", uriSpec.get(), jarFile.get()); > + } > +#endif would be good and doable to share the code please reference jarChannel.get() in the log message for posterity.

Comment on attachment 8887365 [details] Bug 1376496 - Part 5 - Don't send child nsILoadInfo's to the parent for requests. https://reviewboard.mozilla.org/r/158206/#review163478 > Now that we have checked it's a EXTENSION_SCHEME and that the it is a valid directory we can use systemprincipal. Added a comment documenting this.

Comment on attachment 8887363 [details] Bug 1376496 - Part 3 - MOZ_LOG JAR cache hits/misses in DEBUG. https://reviewboard.mozilla.org/r/158202/#review164736 The updated code makes the logging available in non-debug also and I renamed the macro to LOG instead of DBG. (I originally used DBG because these messages are LogLevel::Debug.) > would be good and doable to share the code > please reference jarChannel.get() in the log message for posterity. Done.

@mayhemer, you've already r+'d, but I wanted to make sure you're OK with the changes I've made to move the cache check logic into JARChannel code.

(In reply to Haik Aftandilian [:haik] from comment #23) > @mayhemer, you've already r+'d, but I wanted to make sure you're OK with the > changes I've made to move the cache check logic into JARChannel code. LGTM! Thanks.

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 45f5ca0a7590 -d 4f821dab4306: rebasing 409262:45f5ca0a7590 "Bug 1376496 - Part 1 - Resolve Windows symlinks in unpacked load security check. r=jimm" merging netwerk/protocol/res/ExtensionProtocolHandler.cpp rebasing 409263:9b868702b452 "Bug 1376496 - Part 2 - Don't request FD from parent when JAR file is cached. r=mayhemer" merging modules/libjar/nsJAR.cpp merging netwerk/protocol/res/ExtensionProtocolHandler.cpp rebasing 409264:5f029a687ac9 "Bug 1376496 - Part 3 - MOZ_LOG JAR cache hits/misses in DEBUG. r=mayhemer" merging netwerk/protocol/res/ExtensionProtocolHandler.cpp rebasing 409265:49e3628df752 "Bug 1376496 - Part 4 - FileDescriptorFile.cpp coding style fixes. r=jimm" rebasing 409266:dbd3e9dd7b51 "Bug 1376496 - Part 5 - Don't send child nsILoadInfo's to the parent for requests. r=ckerschb" (tip) merging netwerk/protocol/res/ExtensionProtocolHandler.cpp warning: conflicts while merging netwerk/protocol/res/ExtensionProtocolHandler.cpp! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)

Pushed by haftandilian@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e4e70a229f5e Part 1 - Resolve Windows symlinks in unpacked load security check. r=jimm https://hg.mozilla.org/integration/autoland/rev/4a4f23c6fb22 Part 2 - Don't request FD from parent when JAR file is cached. r=mayhemer https://hg.mozilla.org/integration/autoland/rev/20114f663be1 Part 3 - MOZ_LOG JAR cache hits/misses in DEBUG. r=mayhemer https://hg.mozilla.org/integration/autoland/rev/53d735d83404 Part 4 - FileDescriptorFile.cpp coding style fixes. r=jimm https://hg.mozilla.org/integration/autoland/rev/a3a129c768b6 Part 5 - Don't send child nsILoadInfo's to the parent for requests. r=ckerschb

(In reply to Pulsebot from comment #31) > Pushed by haftandilian@mozilla.com: > https://hg.mozilla.org/integration/autoland/rev/20114f663be1 > Part 3 - MOZ_LOG JAR cache hits/misses in DEBUG. r=mayhemer This commit message shouldn't include "in DEBUG". The logging is available in non-debug and DEBUG. I forgot to update the commit message after changing the patch during review.

https://hg.mozilla.org/mozilla-central/rev/e4e70a229f5e https://hg.mozilla.org/mozilla-central/rev/4a4f23c6fb22 https://hg.mozilla.org/mozilla-central/rev/20114f663be1 https://hg.mozilla.org/mozilla-central/rev/53d735d83404 https://hg.mozilla.org/mozilla-central/rev/a3a129c768b6

Manually verified this issue following the steps from the duplicated Bug 1382271 on Firefox 56.0a1 (2017-08-01) under Windows 10 64-bit, Ubuntu 16.04 32-bit and Mac OS X 10.12.3. Awesome Screenshot webextension seems to work as expected.