815523 - Remote the app: and jar: protocols

Status: RESOLVED FIXED

(Core :: Networking: JAR, defect, P1)

Description

[[:fabrice] Fabrice Desré]

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

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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?

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 9

[[:fabrice] Fabrice Desré]

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

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

Comment 11

[Antonio Manuel Amaya Calvo (:amac)]

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

Comment 12

[Lawrence Mandel [:lmandel] (use needinfo)]

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

Comment 13

[Antonio Manuel Amaya Calvo (:amac)]

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.

Comment 14

[Andrew Overholt [:overholt]]

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

Comment 15

[Andrew Overholt [:overholt]]

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

Comment 16

[Jason Duell]

I'm on this.

Comment 17

[Jason Smith [:jsmith]]

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.

Comment 18

[Alex Keybl [:akeybl]]

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.

Comment 19

[Jason Duell]

I expect to have a patch for this today.

Comment 20

[Jason Duell]

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.

Comment 21

[Jason Duell]

Attachment: Part 1: Changes to B2G JS

Comment 22

[Jason Duell]

Attachment: Part 2: main patch

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

Comment 23

[Jason Duell]

Attachment: Part3: xpcshell tests

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

Comment 24

[Jason Duell]

Attachment: Part 4: skip parent file open if JAR is cached on child.

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

Comment 25

[Jason Duell]

Attachment: Part 5: security checks

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.

Comment 26

[Jason Duell]

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

Comment 27

[Jason Duell]

Attachment: Part 2, v2 main patch

Comment 28

[[:fabrice] Fabrice Desré]

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.

Comment 29

[[:fabrice] Fabrice Desré]

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.

Comment 30

[Michael Wu [:mwu]]

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

Comment 31

[Josh Matthews [:jdm]]

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.

Comment 32

[Michael Wu [:mwu]]

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.

Comment 33

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

Comment 34

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 35

[Andrew Overholt [:overholt]]

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

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

?

Comment 36

[Josh Matthews [:jdm]]

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.

Comment 37

[Michael Wu [:mwu]]

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)

Comment 38

[Jason Duell]

Attachment: Part 1, v2 Changes to Webapps, etc.

with fabrice's nits fixed

Comment 39

[Jason Duell]

Attachment: interdiff v2->v3 of Part 2: main patch

for your browsing convenience...

Comment 40

[Jason Duell]

Attachment: Part 2, v3 main patch

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.

Comment 41

[Jason Duell]

Attachment: part 4, v2: skip parent file open if JAR cached on child

Comment 42

[Jason Duell]

Attachment: Part5, v2: security checks

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

Comment 43

[[:fabrice] Fabrice Desré]

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

Comment 44

[Jason Duell]

Attachment: interdiff v2->v3 of Part 5: security checks

Comment 45

[Jason Duell]

Attachment: Part5, v3: security checks

Comment 46

[Jason Duell]

Attachment: Part3 v2: xpcshell tests

with jdm's fixes.

Comment 47

[Jason Duell]

Attachment: Part5, v4: security checks

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.

Comment 48

[Jason Duell]

Attachment: Part 6: fixes for Windows/OSX

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

Comment 49

[Jason Duell]

Attachment: Part 6: fixes for Windows/OSX

This time with non-empty patch! :)

Comment 50

[Josh Matthews [:jdm]]

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.

Comment 51

[[:fabrice] Fabrice Desré]

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.

Comment 52

[Jason Duell]

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.

Comment 53

[JP Rosevear [:jpr]]

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

Comment 54

[Lucas Adamski [:ladamski]]

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

Comment 55

[Jason Duell]

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.

Comment 56

[Josh Matthews [:jdm]]

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.

Comment 57

[Zibi Braniecki [:zbraniecki][:gandalf]]

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.

Comment 58

[Jason Duell]

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

Comment 59

[Jason Duell]

Attachment: Part5, v5: security checks

uuids updated

Comment 60

[Jason Duell]

Attachment: omnibus patch landed on inbound part 1: patches stolen from bug 782542 Parts 1,2,3: Necko IPC security pref, and disable for xpcshell tests

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.

Comment 61

[Jason Duell]

Attachment: 695181: omnibus patch landed on inbound part 2: patches from this bug

Comment 62

[Jason Duell]

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

Comment 63

[Jason Duell]

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

Comment 64

[Jason Duell]

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

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

Comment 65

[Ryan VanderMeulen [:RyanVM]]

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

Comment 66

[Ryan VanderMeulen [:RyanVM]]

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