[:fabrice] Fabrice Desré Reporter
	Description • 12 years ago

We need that in b2g to safely access the applications packages. See bug 813468 for more details.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 1 • 12 years ago

We don't need to remote the entire protocol, just open the fd in the parent (with the right access rights) and then share with the child.  The existing code can take it from there.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 2 • 12 years ago

(In reply to Chris Jones [:cjones] [:warhammer] from comment #1)
> We don't need to remote the entire protocol, just open the fd in the parent
> (with the right access rights) and then share with the child.  The existing
> code can take it from there.

The JAR file format contains many nooks and crannies for unsigned content to be hidden (e.g. within the ZIP entry metadata, within ZIP "comments", within the unsigned parts of META-INF/*.RSA). So, either we'd have to rewrite the JAR to a known-clean state, or remote the JAR protocol so the parent process can limit the child process to just the entry data.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 3 • 12 years ago

(In reply to Brian Smith (:bsmith) from comment #2)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #1)
> > We don't need to remote the entire protocol, just open the fd in the parent
> > (with the right access rights) and then share with the child.  The existing
> > code can take it from there.
> 
> The JAR file format contains many nooks and crannies for unsigned content to
> be hidden (e.g. within the ZIP entry metadata, within ZIP "comments", within
> the unsigned parts of META-INF/*.RSA). So, either we'd have to rewrite the
> JAR to a known-clean state, or remote the JAR protocol so the parent process
> can limit the child process to just the entry data.

s/remote the JAR protocol/remote the app protocol/

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 4 • 12 years ago

I agree with you, but the goal of this bug is simpler: avoid having to give app processes rights to read user data directly off the file system (and god forbid write).

I'm in favor of doing what you suggest but I think it should be a followup.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 5 • 12 years ago

The jar: protocol doesn't do any actual reading itself. It simply wrapps other protocols and extracts data that they return. So remoting jar: doesn't do a whole lot.

I think we have at least four options here:

* Remote app:// so that the file-reading an unpacking happens in the parent.
* Mark the /data/local/webapps directory world-readable.
* Map app:// to jar://someprotocol://path/to/webapp.zip and remote "someprotocol"
  such that it does the file reading in the parent after doing appropriate security
  checks to check that webapp.zip is the package for the app which is doing the
  load. "someprotocol" could be "file", but only if we're able to do those security
  checks.
* Map app:// to jar://someprotocol://path/to/webapp.zip and make "someprotocol"
  ask the parent process for a file handle and then read data from that. Again,
  the parent would have to do appropriate security checks to make sure that the
  requested file is the package for the app.

There's likely other options too though.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 6 • 12 years ago

At this point (for v1), I'm not in favor of a solution where we do actual sending of data across process boundaries.  That would be a rather destabilizing change performance-wise.

What type of files can go in /data/local/webapps?

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 7 • 12 years ago

(In reply to Jonas Sicking (:sicking) from comment #5)
> * Mark the /data/local/webapps directory world-readable.

This would mean that any (compromised) app would be able to enumerate what versions of what apps the user has installed. that is, it creates a problem similar to the one that motivated us to restrict app:// to same-app.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 8 • 12 years ago

(In reply to Chris Jones [:cjones] [:warhammer] from comment #6)
> What type of files can go in /data/local/webapps?

Instead of playing ping-pong, let me write out my concern.  bsmith's point in comment 7 is one problem.  Enumerating apps is pretty bad, but "just" a fingerprinting issue.

What concerns me is being able to read specific *files* of custom-generated apps.  Imagine I go to mybank.com in the browser, and it says "Ohai cjones we have a b2g app for you" that's generated on the fly and customized for my info.  Then allowing arbitrary apps to read those files can expose my private data.  We of course all think this is a bad design for an app, but nothing prevents it.

This isn't a problem with /system/b2g/webapps because an XHR to github will retrieve the same bits.

	[:fabrice] Fabrice Desré Reporter
	Comment 9 • 12 years ago

(In reply to Jonas Sicking (:sicking) from comment #5)
> * Map app:// to jar://someprotocol://path/to/webapp.zip and remote
> "someprotocol"
>   such that it does the file reading in the parent after doing appropriate
> security
>   checks to check that webapp.zip is the package for the app which is doing
> the
>   load. "someprotocol" could be "file", but only if we're able to do those
> security
>   checks.

Currently app://APP_ID/resource maps to jar:file:///data/local/webapps/APP_ID/application.zip!/resource, so this is what we should try.
As for checking that the package is only requested by the app itself, we already added that in bug 773886.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 10 • 12 years ago

(In reply to Fabrice Desré [:fabrice] from comment #9)
> As for checking that the package is only requested by the app itself, we
> already added that in bug 773886.

IIRC, that check happens in the content process, which means that a compromised content process can subvert the check. We need it to be enforced even when the content process is compromised.

	Antonio Manuel Amaya Calvo (:amac)
	Comment 11 • 12 years ago

(In reply to Brian Smith (:bsmith) from comment #10)
> (In reply to Fabrice Desré [:fabrice] from comment #9)
> > As for checking that the package is only requested by the app itself, we
> > already added that in bug 773886.
> 
> IIRC, that check happens in the content process, which means that a
> compromised content process can subvert the check. We need it to be enforced
> even when the content process is compromised.

The check currently happens on the content process because all the process is done on the content process... but in any case I don't think we can fix this problem without having a 1-on-1 relationship between processes and apps. The current test is something like 'is the calling principal authorized to load the to-be-loaded content?'. 

But if the relationship isn't 1-on-1 I don't believe the parent can truly know who the calling principal is... because that information must come from the content process itself, which we're assuming can be compromised. If there's a 1-on-1 relationship then the parent process can know which app is loaded on which process. But since there isn't, the best it can know is which set of apps is loaded on which process... but to know which app is doing the invoking it has to trust the content process. And if we trusted the content process we wouldn't need to do this check on the parent.  Kind of a catch-22 there :)

Cris raised a good point on comment #8, but again it's something that I don't think can be fully solved unless there's a 1-on-1 relationship between apps and processes, and for the same reason. If I can compromise the content process but the content process doesn't have direct access to a directory I can try and trick the parent process into giving me the content of said directory. It makes the exploiting harder, assuming the parent process controls which apps are running on which processes (since then attacker would have to be running on the same process than the victim), but that's all.

I've been giving it some thought, and thinking on a way we could use setfsuid along with having two identities for processes (since you can set a fsuid on a per thread basis) to isolate content threads inside the process... the problem is that the permission boundary to change fsuid is the process, not the thread. So if thread A has permission to change thread's B fsuid, thread B has permission to change it back to the original value. And the only way to have a permission per app would be to run the permission setter as root. 

So, in short, I think that to correctly prevent an exploit scenario as the one described by Cris on comment#9 we can either:

a) Have one uid (and thus one process) per app, as Android does 

or

b) Just tell the developers that they should *not* include anything they consider private as part of a packaged app, since it isn't really private.

Option b solves the problem (for some very relaxed value of "solve", true :P) and has the advantage of not needing us to change anything besides giving read (evidently never write!) permissions on the app directory to the content processes.

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 12 • 12 years ago

Antonio, looks like you're working on this. Assigning to you.

	Antonio Manuel Amaya Calvo (:amac)
	Comment 13 • 12 years ago

I don't think option a is viable on the timeframe we have right now (nor I do know if it's viable at all to have one content process per app, considering they're not exactly lightweight). And without implementing option a, anything that we implement doesn't actually add any security layer against a possible exploit(it adds more complexity to an exploit, but just that). So option b was doing nothing. If that's what's required then sure, I can take that :P.

Otherwise, if you want to actually remote the protocols, I don't know how the communication between parent and content processes work well enough to take this, I'm afraid. Reassigning it to nobody for the time being.

	Andrew Overholt [:overholt]
	Comment 14 • 12 years ago

Jason, can you take this or are you too overloaded?

	Andrew Overholt [:overholt]
	Comment 15 • 12 years ago

Setting priority based on triage discussions.  Feel free to decrease priority if you disagree.

	Jason Duell Assignee
	Comment 16 • 12 years ago

I'm on this.

	Jason Smith [:jsmith]
	Comment 17 • 12 years ago

Increasing priority up to P1 critical smoketest blocker - even with the workaround about to be implemented in bug 813468, we'll still need this to implement true fix that the smoke test would test for installing of packaged apps.

	Alex Keybl [:akeybl]
	Comment 18 • 12 years ago

Mass Modify: All un-milestoned, unresolved blocking-basecamp+ bugs are being moved into the C3 milestone. Note that the target milestone does not mean that these bugs can't be resolved prior to 12/10, rather C2 bugs should be prioritized ahead of C3 bugs.

	Jason Duell Assignee
	Comment 19 • 12 years ago

I expect to have a patch for this today.

	Jason Duell Assignee
	Comment 20 • 12 years ago

The patches I'm about to land cover everything but two issues:

1) the parent process is currently opening the JAR file on the main thread from within the IPDL call.  That's not so horrible--we do a blocking open() for JARs on desktop too (see bug 817202) and it only happens once per jar (so for B2G apps will only happen once per app).  Let me know if we think this is good enough for now.  I can make the open() happen on a different thread, just haven't gotten to it yet.

2) The patches don't apply to beta yet: nsJARChannel.cpp in particular has stuff in m-c that isn't on beta.  I'm working on it.

Neither of these need to block reviewing these patches.

Hand-wavey design overview:

- We convert app:// URIs to regular 'jar:file://' URIs if process can open JAR regularly, i.e. if in-process or if a "core app" with I/O permission). 
- Else we convert to new 'remoteopenfile://" URI scheme, which JARChannels are now trained to recognize and treat specially: they launch an open of the file handle on the parent and get notified when it's done.
- filehandle is stored in a RemoteOpenFile class that looks like a regular nsIFile but doesn't support any I/O operations except OpenNSPRFileDesc().  JARChannel passes this fake nsIFile down into JAR internals (zipreader, zipcache, etc) and everything works magically w/o any changes to non-e10s code.
- Optimization skips opening file on parent if we know zipcache is caching file already
- Security: we only allow apps to open their own application.zip file.

	Jason Duell Assignee
	Comment 22 • 12 years ago

I need mwu (taras on vacation) for the JAR bits.  I'm thinking jdm can review the rest of the patch (or bent or honza).

	Jason Duell Assignee
	Comment 23 • 12 years ago

These don't cover security checks for making sure we only allow application.zip to be opened (we disable them for xpcshell).  Not sure how to write automated test for that, since we kill offending processes.

mwu: sorry, lots of context diff in this patch.  Only real changes are to use 'jar:remoteopenfile' if we're on child, and to disable all the tests for e10s testing except the simple async open of non-nested jar (see comment).  Rest of patch is mainly moving the rest of the code into an "if (!inChild)" block (indenting is what's causing the diff to be big).

	Jason Duell Assignee
	Comment 24 • 12 years ago

I think we want this part (for speed reasons) but I split it out because it's the only part of the code that affects regular desktop beta:  it adds a "IsCached" method to nsIZipReader.idl".   I doubt many addons are using that (and since I'm only adding a new method, we shouldn't be breaking JS addons).  

If we only add method to an IDL do we need to change uuid?  I did in patch, but suspect we can leave it the same (and may want to since we're changing beta).

	Jason Duell Assignee
	Comment 25 • 12 years ago

Fabrice: let me know if there's actually a way from C++ to get the full exact path match for the application.zip file.  I didn't see one (can't use message manager, right?) so I implemented a hack where we just make sure that the path ends in /APP_ID/application.zip.

	Jason Duell Assignee
	Comment 26 • 12 years ago

Comment on attachment 690874 [details] [diff] [review]
Part 2: main patch

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

::: netwerk/ipc/RemoteOpenFileChild.cpp
@@ +48,5 @@
> +RemoteOpenFileChild::Init(nsIURI* aRemoteOpenUri)
> +{
> +// TODO: require tabchild unless security disabled
> +#if 0
> +  if (!aRemoteOpenUri || !aTabChild) {

Forgot to remove this in patch series.  changed to just "if (!aRemoteURI)"

@@ +187,5 @@
> +RemoteOpenFileChild::OpenNSPRFileDesc(int32_t aFlags, int32_t aMode,
> +                                      PRFileDesc **aRetval)
> +{
> +// TODO: remove this early test code
> +//  return mFile->OpenNSPRFileDesc(flags, mode, _retval);

also removed now.

@@ +301,5 @@
> +  return mFile->GetParent(aParent);
> +}
> +
> +nsresult  
> +RemoteOpenFileChild::InitWithPath(const nsAString &filePath)

Meh--some of these functions need to be not supported instead of delegated to underlying nsIFile, else mURI and mFile could get out of sync.  Irrelevant for actual patch logic, but I'll fix them.  New patch coming up.

::: netwerk/ipc/nsIRemoteOpenFileListener.idl
@@ +8,5 @@
> +
> +/**
> + * nsIRemoteOpenFileListener: passed to RemoteOpenFileChild::AsyncRemoteFileOpen.
> + *
> + * Interface for notifying when the file has been opened as in available in

s/as in/and is/.

	[:fabrice] Fabrice Desré Reporter
	Comment 28 • 12 years ago

Comment on attachment 690873 [details] [diff] [review]
Part 1: Changes to B2G JS

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

r=me with nits fixed.

::: dom/apps/src/Webapps.jsm
@@ +773,5 @@
>          this.doInstallPackage(msg, mm);
>          break;
> +      case "Webapps:GetAppInfo":
> +        return { 'basePath':  this.webapps[msg.id].basePath,
> +                 'isCoreApp': !this.webapps[msg.id].removable };

Nit: we use double quotes for strings in this file.

::: netwerk/protocol/app/AppProtocolHandler.js
@@ +18,5 @@
>  function AppProtocolHandler() {
> +  this._appInfo = [];
> +  this._runningInParent = Cc["@mozilla.org/xre/runtime;1"].
> +                          getService(Ci.nsIXULRuntime).
> +                          processType == Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT;

Nit: we usually try to align this way:
Cc["@mozilla.org/xre/runtime;1"]
  .getService(Ci.nsIXULRuntime)
  .processType == Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT;

@@ +37,4 @@
>  
> +    if (!this._appInfo[aId]) {
> +      let reply = cpmm.sendSyncMessage("Webapps:GetAppInfo",
> +                                                 { id: aId });

Nit: align { id... with "Webapps:...

@@ +39,5 @@
> +      let reply = cpmm.sendSyncMessage("Webapps:GetAppInfo",
> +                                                 { id: aId });
> +      this._appInfo[aId] = { 'basePath':  reply[0].basePath + "/",
> +                             'isCoreApp': reply[0].isCoreApp };
> + 

Nit: trailing whitespace, and use double quotes for strings. Also, you could add the "/" in the parent and just do this._appInfo[aId] = reply[0];

@@ +67,5 @@
>      }
>  
>      // Build a jar channel and masquerade as an app:// URI.
> +    let appInfo = this.getAppInfo(appId);
> +    let uri = '';

|let uri;| is enough.

@@ +69,5 @@
>      // Build a jar channel and masquerade as an app:// URI.
> +    let appInfo = this.getAppInfo(appId);
> +    let uri = '';
> +    if (this._runningInParent || appInfo.isCoreApp) {
> +      // In-parent and CoreApps can directly access files, so use jar:file://

I'm not 100% sure the CoreApps behavior is really wanted or just a consequence of the current file permissions on /system and something we want to change. That could be a follow up anyway.

	[:fabrice] Fabrice Desré Reporter
	Comment 29 • 12 years ago

Comment on attachment 690882 [details] [diff] [review]
Part 5: security checks

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

So, if you need the basePath for an appId, we should just add that to AppsService.js which is our c++ friendly interface to Webapps.jsm

Also note that the rule is that an app can only access its own application.zip, unless it has the webapps-manage permission and then can access any application.zip (we need that for the homescreen to be able to load icons from other apps packages). There is code dealing with that in the current setup at https://mxr.mozilla.org/mozilla-central/source/caps/src/nsScriptSecurityManager.cpp#1369

::: netwerk/ipc/NeckoParent.cpp
@@ +310,5 @@
> +
> +    // at this point our URI is a regular file:// uri of jar file, aka the
> +    // original nsIJARURI.JARFile (except with scheme 'file' instead of
> +    // 'remoteopenfile'
> +    nsPrintfCString mustMatch("/%lu/application.zip", (unsigned long)appId);

the path used is not /appId/application.zip but /UUID/application.zip, where UUID is generated when installing a new app, or set up by the build system (it can be, eg. calculator.gaiamobile.org). So I think the following test will always fail.

	Michael Wu [:mwu]
	Comment 30 • 12 years ago

Comment on attachment 690896 [details] [diff] [review]
Part 2, v2 main patch

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

Mostly looks ok. It's too bad jarcache doesn't currently have some sort of call so you can check if a file is already in the jarcache. I just need an answer to the question on nsJARChannel::OnRemoteFileOpenComplete.

::: modules/libjar/nsJARChannel.cpp
@@ +339,5 @@
>          nsCOMPtr<nsIFileURL> fileURL = do_QueryInterface(mJarBaseURI);
>          if (fileURL)
>              fileURL->GetFile(getter_AddRefs(mJarFile));
>      }
> +    // if we're in child process and have special "remoteopenfile:://" scheme,

"remoteopenfile://"?

@@ +341,5 @@
>              fileURL->GetFile(getter_AddRefs(mJarFile));
>      }
> +    // if we're in child process and have special "remoteopenfile:://" scheme,
> +    // create special nsIFile that gets file handle from parent when opened.
> +    if (!mJarFile && XRE_GetProcessType() != GeckoProcessType_Default) {

Hm, so we only try this when we can't directly get the file, so trying to do file:// would work in child processes. Are there currently cases where we have to support that? Guess it won't matter once the child processes get chrooted.

@@ +904,5 @@
>  //-----------------------------------------------------------------------------
> +// nsIRemoteOpenFileListener
> +//-----------------------------------------------------------------------------
> +nsresult
> +nsJARChannel::OnRemoteFileOpenComplete(nsresult aOpenStatus)

Should we check aOpenStatus? Seems like we should fail immediately if we weren't able to open the file on the parent side.

@@ +911,5 @@
> +
> +    // files on parent are always considered safe
> +    mIsUnsafe = false;
> +
> +    nsCOMPtr<nsJARInputThunk> input;

This should probably be a nsRefPtr though I realize that a lot of the code here is using nsCOMPtr for nsJARInputThunk..

	Josh Matthews [:jdm]
	Comment 31 • 12 years ago

Comment on attachment 690896 [details] [diff] [review]
Part 2, v2 main patch

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

::: netwerk/ipc/Makefile.in
@@ +30,5 @@
>    NeckoCommon.h       \
>    NeckoMessageUtils.h \
>    ChannelEventQueue.h \
> +  RemoteOpenFileParent.h \
> +  RemoteOpenFileChild.h \

Are these actually used outside of netwerk/ipc?

::: netwerk/ipc/PNecko.ipdl
@@ +46,5 @@
>    PFTPChannel(PBrowser browser, SerializedLoadContext loadContext);
>    PWebSocket(PBrowser browser, SerializedLoadContext loadContext);
>    PTCPSocket(nsString host, uint16_t port, bool useSSL, nsString binaryType,
>               nullable PBrowser browser);
> +  PRemoteOpenFile(URIParams fileuri, nullable PBrowser browser);

Do we have tests that exercise this? If not, let's not allow a null browser until it's absolutely required.

::: netwerk/ipc/RemoteOpenFileChild.cpp
@@ +64,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // Note: this doesn't do any actual File I/O, so OK to do on child.
> +  rv = NS_NewLocalFile(NS_ConvertUTF8toUTF16(path), false,
> +                                             getter_AddRefs(mFile));

nit: weird indentation

@@ +110,5 @@
> +  gNeckoChild->SendPRemoteOpenFileConstructor(this, uri, tabChild);
> +
> +  // Can't seem to reply from within IPDL Parent constructor, so send open as
> +  // separate message
> +  SendAsyncOpenFile();

You could probably avoid this by calling the code that needs to reply from NeckoParent::RecvPRemoteOpenFileConstructor.

@@ +136,5 @@
> +  }
> +
> +  MOZ_ASSERT(mListener);
> +
> +  mListener->OnRemoteFileOpenComplete(aRV);

I think we should have an ActorDestroyed method that calls OnRemoteFileOpenComplete with a failure code when an IPDL error occurs, otherwise the nsIChannel contract doesn't get fulfilled for the JAR channel's listener.

::: netwerk/ipc/RemoteOpenFileChild.h
@@ +78,5 @@
> +
> +  // regular nsIFile object, that we forward most calls to.
> +  nsCOMPtr<nsIFile> mFile;
> +  nsCOMPtr<nsIURI> mURI;
> +  nsCOMPtr<nsIRemoteOpenFileListener> mListener;

I'm worried that there's a cycle here that keeps the JAR channel and this object alive forever. Please check this?

::: netwerk/ipc/RemoteOpenFileParent.cpp
@@ +12,5 @@
> +#if !defined(XP_WIN) && !defined(MOZ_WIDGET_COCOA)
> +#include <fcntl.h>
> +#endif
> +
> +#if 0

Remove this block.

	Michael Wu [:mwu]
	Comment 32 • 12 years ago

Comment on attachment 690877 [details] [diff] [review]
Part 4: skip parent file open if JAR is cached on child.

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

::: modules/libjar/nsJAR.cpp
@@ +1071,5 @@
> +  MutexAutoLock lock(mLock);
> +
> +  nsAutoCString uri;
> +  rv = zipFile->GetNativePath(uri);
> +  if (NS_FAILED(rv)) return rv;

nit: put return on a new line.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 33 • 12 years ago

(In reply to Brian Smith (:bsmith) from comment #2)
> The JAR file format contains many nooks and crannies for unsigned content to
> be hidden (e.g. within the ZIP entry metadata, within ZIP "comments", within
> the unsigned parts of META-INF/*.RSA). So, either we'd have to rewrite the
> JAR to a known-clean state, or remote the JAR protocol so the parent process
> can limit the child process to just the entry data.

I will address this concern in bug 822072.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 34 • 12 years ago

I don't think it's at all important that the child process doesn't have access to reading the non-signed parts of the app package. I.e. I think it's totally fine if the implementation here always opens the package file in the parent process and sends a cloned filehandle to the child process.

What we should have is app:-protocol level checks which prevents the child process from opening files from the zip which aren't unsigned. However those checks can run in the child process.

If someone hacks the child process then it can do anything it wants (within the confines of the child process) and so it doesn't matter if it can load javascript or anything else from the app package. It can still only read contents from its own package anyway, and so no privacy sensitive information could be read.

	Andrew Overholt [:overholt]
	Comment 35 • 12 years ago

mwu is travelling this week.  Can we get someone other than him to review:

- Part 2, v2 main patch 
- Part3: xpcshell tests

?

	Josh Matthews [:jdm]
	Comment 36 • 12 years ago

Comment on attachment 690875 [details] [diff] [review]
Part3: xpcshell tests

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

::: modules/libjar/test/unit/test_jarchannel.js
@@ +26,5 @@
>                                 );
>  
>  const fileBase = "test_bug637286.zip";
>  const file = do_get_file("data/" + fileBase);
> +// on child we'll test with jar:ipcfile:// instead of jar:file://

remoteopenfile

::: modules/libjar/test/unit/xpcshell.ini
@@ +3,4 @@
>  tail = 
>  
>  [test_jarchannel.js]
> +[test_jarchannel_e10s.js]

This probably needs skip-if for OS X; we don't run IPC xpcshell tests on mac.

	Michael Wu [:mwu]
	Comment 37 • 12 years ago

Comment on attachment 690896 [details] [diff] [review]
Part 2, v2 main patch

r=me if I get an answer about why we're not checking aOpenStatus, or a new patch that checks it. (doing this now in case I'm not around for the response)

	Jason Duell Assignee
	Comment 38 • 12 years ago

with fabrice's nits fixed

	Jason Duell Assignee
	Comment 39 • 12 years ago

for your browsing convenience...

	Jason Duell Assignee
	Comment 40 • 12 years ago

Updated main patch.  I only have one real review question for jdm:

> I think we should have an ActorDestroyed method that calls
> OnRemoteFileOpenComplete with a failure code when an IPDL error occurs,
> otherwise the nsIChannel contract doesn't get fulfilled for the JAR channel's
> listener.

jdm: but doesn't an IPDL error occurring mean that the whole child process gets killed, so we don't need to worry about this?

> > +  nsCOMPtr<nsIRemoteOpenFileListener> mListener;
> 
> I'm worried that there's a cycle here that keeps the JAR channel and 
> this object alive forever. Please check this?

We do the standard necko thing of releasing the mListener after we call OnRemoteFileOpenComplete().  IIUC we're essentially guaranteed to either be called by the parent at some point do that, or be killed due to an IPDL error.  But just in case I've added a call to OnRemoteFileOpenComplete and release the listener in RemoteOpenFileChild::ReleaseIPDLReference().  Is that basically the equivalent of the ActorDestroyed you were suggesting? 

Nits:

> > +nsJARChannel::OnRemoteFileOpenComplete(nsresult aOpenStatus)
> 
> Should we check aOpenStatus? Seems like we should fail immediately if 
> we weren't able to open the file on the parent side.

Thanks for catching this.  It turns out that we'd already be dead if NS_FAILED(aOpenStatus) as IPDL kills us for sending an invalid FileDescriptor.  But that may change someday, so I've changed the code to check and fail.

> so trying to do file:// would work in child processes. Are there currently
> cases where we have to support that? Guess it won't matter once the child
> processes get chrooted.

At the moment "core" apps are granted the power to open file:// directly on child, and my part1 patch allows them to do so for efficiency.  So yes.

> This should probably be a nsRefPtr

Yup. Changed.  I went ahead and changed all other nsCOMPtrs<thunks> in nsJARChannel.cpp to nsRefPtrs too, since they're obviously wrong.

> +  RemoteOpenFileParent.h \
> +  RemoteOpenFileChild.h \
> 
> Are these actually used outside of netwerk/ipc?

Child is (included by JARChannel).  Parent isn't, so I tried to remove it, only to discover that we seem to need to export all headers that we want to use EXPORTS_NAMESPACE with (i.e #include as "mozilla/...").  Correct me if I'm missing something.  Seems like yet another drawback of C++ namespaces :)

> PRemoteOpenFile(URIParams fileuri, nullable PBrowser browser);
>
> Do we have tests that exercise this? If not, let's not allow a null browser 

Yup.  xpcshell test in the test patch.

> nit: weird indentation

fixed.

> Can't seem to reply from within IPDL Parent constructor, so send open as
> separate message

I'm punting on this for now, and will fix if/when we switch to doing the fd open() async on the parent.  Sending another message is cheap too IIRC what cjones has told me.

> Remove this block.

done.

	Jason Duell Assignee
	Comment 42 • 12 years ago

With this patch things are working on the phone (yay).  I've traced through the debugger to make sure we're doing the open on the parent.  Performance for launching a sample packaged app in an opt build seems fine.

The only codepath that hasn't gotten much testing are the checks based on getCoreAppsBasePath().  On desktop B2G it looks like we don't use "coreAppsDir", and on the phone core apps are just accessing file:// directly.

I still need to make the patches work on Windows/OSX at least to the point where I don't break tests/builds.  Coming up soon...

	[:fabrice] Fabrice Desré Reporter
	Comment 43 • 12 years ago

Comment on attachment 693342 [details] [diff] [review]
Part5, v2: security checks

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

That's almost ready! r- because getCoreAppsBasePath() and getWebAppsBasePath() must also be implemented in AppsServiceChild.jsm and not just in Webapps.jsm. This is because AppsService.js loads AppsServiceChild.jsm when instanciated from the child. For now we don't use these calls so it may be ok to just throw or return null.

::: dom/apps/src/AppsService.js
@@ +58,5 @@
>      return DOMApplicationRegistry.getAppFromObserverMessage(aMessage);
>    },
>  
> +  getCoreAppsBasePath: function getCoreAppsBasePath() {
> +    debug("getCoreAppsBasePath(}");

Nit: s/(}/()

@@ +63,5 @@
> +    return DOMApplicationRegistry.getCoreAppsBasePath();
> +  },
> +
> +  getWebAppsBasePath: function getWebAppsBasePath() {
> +    debug("getWebAppsBasePath(}");

Ditto

::: dom/apps/src/Webapps.jsm
@@ +2053,5 @@
>      return AppsUtils.getAppFromObserverMessage(this.webapps, aMessage);
>    },
>  
> +  getCoreAppsBasePath: function() {
> +    return FileUtils.getDir("coreAppsDir", ["webapps"], true, true).path;

s/true, true/false since we can't create directories on the system partition

	Jason Duell Assignee
	Comment 46 • 12 years ago

with jdm's fixes.

	Jason Duell Assignee
	Comment 47 • 12 years ago

Setting and asserting for mWebAppsBasePath needs to be skipped if we've disabled IPC security.   Broke xpcshell tests.  Only change is an "if (!gDisableIPCSecurity) {..}" to wrap that code in the NeckoParent constructor.

	Jason Duell Assignee
	Comment 48 • 12 years ago

Simple approach: skip remoting fd open on parent for Windows/OSX, since we'll only use those platforms for desktop builds, where child processes aren't restricted from opening files.  I tried to get full remote support for OSX but couldn't get desktop B2G to work even w/o any of my patches, so that'll have to wait.

Try run here:

  https://tbpl.mozilla.org/?tree=Try&rev=fcf24bdef5d3

	Jason Duell Assignee
	Comment 49 • 12 years ago

This time with non-empty patch! :)

	Josh Matthews [:jdm]
	Comment 50 • 12 years ago

Comment on attachment 693325 [details] [diff] [review]
Part 2, v3 main patch

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

My fears about ActorDestroy have been assuaged.

	[:fabrice] Fabrice Desré Reporter
	Comment 51 • 12 years ago

Comment on attachment 693821 [details] [diff] [review]
Part5, v4: security checks

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

r=me with interfaces' UUID updated.

::: dom/interfaces/apps/mozIApplication.idl
@@ +15,1 @@
>  interface mozIApplication: mozIDOMApplication

Please update the UUID

::: dom/interfaces/apps/nsIAppsService.idl
@@ +58,5 @@
> +
> +  /**
> +   * Returns the basepath for regular packaged apps
> +   */
> +  DOMString getWebAppsBasePath();

Please update the UUID.

	Jason Duell Assignee
	Comment 52 • 12 years ago

Comment on attachment 693825 [details] [diff] [review]
Part 6: fixes for Windows/OSX

I'm running into weirdness on Windows here--seeing null nsCOMPtr deref somehow connected with the nsZipReaderCache:IsCached function I added.  Possible workaround is to just not test on windows, since OSX/Linux/B2G seem fine, and the codepath won't be run on production on windows (just an xpcshell test).  But I'd like to know what's going on, so investigating some more.

	JP Rosevear [:jpr]
	Comment 53 • 12 years ago

Given the time constraints here, I suggest we think about landing this and filing a follow up bug.

	Lucas Adamski [:ladamski]
	Comment 54 • 12 years ago

(In reply to JP Rosevear [:jpr] from comment #53)
> Given the time constraints here, I suggest we think about landing this and
> filing a follow up bug.

Agreed.

	Jason Duell Assignee
	Comment 55 • 12 years ago

Comment on attachment 693825 [details] [diff] [review]
Part 6: fixes for Windows/OSX

OK this works everywhere but windows, and I'll disable the one test there that fails.

	Josh Matthews [:jdm]
	Comment 56 • 12 years ago

Comment on attachment 693825 [details] [diff] [review]
Part 6: fixes for Windows/OSX

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

::: netwerk/ipc/RemoteOpenFileParent.h
@@ +27,5 @@
>  private:
>    nsCOMPtr<nsIFileURL> mURI;
> +
> +  // clang on OSX will fail with warning if if sees 'unused' private member, so
> +  // hide mFd

I don't think this comment is necessary.

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 57 • 12 years ago

What is the expected performance impact of this change on app startup time? I saw comment 42, just wondering if you have any more specific expectations.

	Jason Duell Assignee
	Comment 58 • 12 years ago

> What is the expected performance impact of this change on app startup time?

At application launch time, we will now open the file handle for the application.zip on the parent instead of the child.  This will add an IPDL round trip to the time for application launch, which I don't expect to be significant compared with the actual I/O for loading the app.  That said I don't have numbers or a good plan to get them.  If someone does, that would be swell.  Even a side-by-side eyeball comparison with two phones, one with these patches and one w/o, would be useful as a sanity check.  Do we have resources to do that?

A possible optimization could be to have the parent keep open filehandles (or better, zipcache mmapped regions) to the most commonly used apps, and when forking a child process close all of them except the one that belongs to the app.  I'd wager it's not worth the effort, but that's the most obvious path forward to me if there's a perf issue here.

	Jason Duell Assignee
	Comment 59 • 12 years ago

uuids updated

	Jason Duell Assignee
	Comment 60 • 12 years ago

Ripping out parts of necko sec patch that I need to land this, since there's no reason to stop progress :)  Keeping separate from rest of patch in case we wind up backing this patch out and then the necko-sec patch needs to land before this re-lands.

	Jason Duell Assignee
	Comment 62 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ff2c4b415f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/108eb99a250d

	Jason Duell Assignee
	Comment 63 • 12 years ago

> What is the expected performance impact of this change on app startup time?

And the code here only affects packaged apps that are *not* core apps (which for the moment at least still have file I/O permission and just open their application.zip file normally). So to test perf, download one of the packaged apps from 

  http://owapps.cloudfoundry.com/

	Jason Duell Assignee
	Comment 64 • 12 years ago

skip-if syntax needs os == "win", not "windows":

  https://hg.mozilla.org/integration/mozilla-inbound/rev/38fbb50e6038

	Ryan VanderMeulen [:RyanVM]
	Comment 65 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/4ff2c4b415f8
https://hg.mozilla.org/mozilla-central/rev/108eb99a250d
https://hg.mozilla.org/mozilla-central/rev/38fbb50e6038

	Ryan VanderMeulen [:RyanVM]
	Comment 66 • 12 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/99ce093a9bf7
https://hg.mozilla.org/releases/mozilla-aurora/rev/453549de39da
https://hg.mozilla.org/releases/mozilla-aurora/rev/e364c2c32501

https://hg.mozilla.org/releases/mozilla-b2g18/rev/7c3293422c17
https://hg.mozilla.org/releases/mozilla-b2g18/rev/c5057efaa0ec
https://hg.mozilla.org/releases/mozilla-b2g18/rev/f96d19c5ded0