Closed Bug 988816 Opened 6 years ago Closed 5 years ago

Remote file open for memory mapped array buffer in content process

Categories

(Core :: Networking: JAR, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: swu, Assigned: swu)

References

Details

Attachments

(3 files, 11 obsolete files)

11.21 KB, patch
swu
: review+
Details | Diff | Splinter Review
19.48 KB, patch
swu
: review+
Details | Diff | Splinter Review
3.70 KB, patch
swu
: review+
Details | Diff | Splinter Review
Follow up from bug 945152 comment 59.
For testing remote open with keyboard app as OOP.
Blocks: 945152
Component: DOM → Networking: JAR
Hi Jason,

To support OOP case for bug 945152, we want to get fd of remote JAR file on parent side, and use it to mmap to the remote JAR file.

Attached is the patch which makes use of remote app/jar protocol implementation by you on bug 815523.

Testing on B2G phone works.  May I have your feedback on this patch?
Attachment #8419302 - Attachment is obsolete: true
Attachment #8434062 - Flags: feedback?
Attachment #8434062 - Flags: feedback? → feedback?(jduell.mcbugs)
Comment on attachment 8434062 [details] [diff] [review]
Patch: Get fd from child process by remote file open.

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

(Aaron--see what you think of my proposed tweak to this patch's mod of nsIJARChannel--comment below)

RemoteOpenFile is pretty hacky, so this is layering technical debt upon technical debt...  But given that the approach here actually seems pretty good.  A couple of things I think need to be fixed or would be cleaner a different way.

The biggest potential issue is what files we want to allow to be opened using this method. Right now the code that allocates RemoteFileOpen's only allows an app to open its own application.zip file, unless it has 'webapps-manage' permission:

  http://mxr.mozilla.org/mozilla-central/source/netwerk/ipc/NeckoParent.cpp#566

Maybe the keyboard mappings, etc, are in application.zip?  Then we're fine. But for other files we'd need to change the permission model.

::: content/base/src/nsXMLHttpRequest.cpp
@@ +4075,5 @@
>  
> +  // Get file descriptor of JAR file.
> +  // For remote open JAR file(jar:remoteopenfile://), there is a current limitation
> +  // that file could be opened just once.  Clone the file here to make sure we
> +  // can open the file more than once.

Ah, so we can call dup(fd) on the child?  If true, then I think the cleaner fix is to change RemoteOpenFileChild to call fdup() during Clone,
and also every time OpenNSPRFileDesc is called, so we can automatically clone/open as many times as we like.  We would need to make sure that we close() the master fd at some point though--maybe after OnStopRequest has been called?  The destructor might work instead, but sometimes channels get kept around a long time, and I'd hope that after OnStopRequest would be good enough.

::: modules/libjar/nsIJARChannel.idl
@@ +31,5 @@
> +    /**
> +     * When aPreserveRemoteFD enabled, the JAR cache will not be used, and
> +     * the fd from remote file open will be preserved for further need.
> +     */
> +    void setPreserveRemoteFD(in bool aPreserveRemoteFD);

This flag also causes the JARChannel to not deliver any data (i.e. only OnStart/Stop are called, no OnDataAvailable).  So it's really a special mode where we only want to get the fd and not the data.  Let's call it something like

  /**
   * When called, the JAR channel is to be used only to get the underlying
   * file descriptor.  OnStart/OnStopRequest will be called, but not 
   * OnDataAvailable.
   *
  void SetFdOnlyMode()  // not sure it even needs a bool? It's not meant to be set/unset multiple times

This seems better to me, but I'm not a JAR channel peer--so I guess we should ask :aklotz or :mwu.

As I mention below, I'm also unclear as to whether the patches this depends on (non-e10s version of this code) also omit calling OnDataAvailable.  It would be good to make them behave the same way.

::: modules/libjar/nsJARChannel.cpp
@@ +354,5 @@
>              NS_ENSURE_SUCCESS(rv, rv);
>              mJarFile = remoteFile;
>  
>              nsIZipReaderCache *jarCache = gJarHandler->JarCache();
> +            if (jarCache && !mPreserveRemoteFD) {

You also need to check for !mPreserveRemoteFD (or mFdOnlyMode, after the IDL change) below where

  if (gJarHandler->RemoteOpenFileInProgress(...)

gets called, right?  Otherwise you could somehow get a JAR cache hit (and thus no fd) if you request the same JAR once w/o FdOnlyMode, and then with it.  That's actually likely if we're loading from the same application.zip file that the app bootstraps from.

@@ +962,5 @@
> +    if (mPreserveRemoteFD) {
> +        rv = OnStartRequest(nullptr, nullptr);
> +        if (NS_SUCCEEDED(rv)) {
> +            OnStopRequest(nullptr, nullptr, NS_OK);
> +        }

So how did this code work in the non-e10s version?  Did you change the JAR file code there to somehow not call OnDataAvailable?  There's a lot of patches in bug 945152, but I didn't seem to see anything that removes OnDataAvailable from being delivered?

::: netwerk/ipc/RemoteOpenFileChild.cpp
@@ +244,5 @@
> +nsresult
> +RemoteOpenFileChild::SetPreserveRemoteFD(bool aPreserveRemoteFD)
> +{
> +  mPreserveRemoteFD = aPreserveRemoteFD;
> +  return NS_OK;

So if we can dup() the fd as I mentioned, I think we can get rid of SetPreserveRemoteFD in RemoteOpenFileChild, and only need a special flag for JARChannel (the FdOnlyMode I proposed).  RemoteOpenFileChild would just open the file as many times as you want automatically.
Attachment #8434062 - Flags: feedback?(jduell.mcbugs)
Attachment #8434062 - Flags: feedback?(aklotz)
Attachment #8434062 - Flags: feedback+
(In reply to Jason Duell (:jduell) from comment #4)
> Comment on attachment 8434062 [details] [diff] [review]
> Patch: Get fd from child process by remote file open.
> 
> Review of attachment 8434062 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (Aaron--see what you think of my proposed tweak to this patch's mod of
> nsIJARChannel--comment below)
> 
> RemoteOpenFile is pretty hacky, so this is layering technical debt upon
> technical debt...  But given that the approach here actually seems pretty
> good.  A couple of things I think need to be fixed or would be cleaner a
> different way.
> 
> The biggest potential issue is what files we want to allow to be opened
> using this method. Right now the code that allocates RemoteFileOpen's only
> allows an app to open its own application.zip file, unless it has
> 'webapps-manage' permission:
> 
>  
> http://mxr.mozilla.org/mozilla-central/source/netwerk/ipc/NeckoParent.cpp#566
> 
> Maybe the keyboard mappings, etc, are in application.zip?  Then we're fine.
> But for other files we'd need to change the permission model.

Yes, we are reading files in same application.zip, so it's not an issue for now.  I haven't thought about the case of reading different application.zip yet.  This is a good point, there could be scenarios worth thinking about cross application.zip support later on.

> 
> ::: content/base/src/nsXMLHttpRequest.cpp
> @@ +4075,5 @@
> >  
> > +  // Get file descriptor of JAR file.
> > +  // For remote open JAR file(jar:remoteopenfile://), there is a current limitation
> > +  // that file could be opened just once.  Clone the file here to make sure we
> > +  // can open the file more than once.
> 
> Ah, so we can call dup(fd) on the child?

Based on the comment below we can use dup() on the child.
https://bugzilla.mozilla.org/show_bug.cgi?id=790923#c86

> If true, then I think the cleaner fix is to change
> RemoteOpenFileChild to call fdup() during Clone,

Do you mean dup() instead of fdup() here?

> and also every time OpenNSPRFileDesc is called, so we can automatically
> clone/open as many times as we like.  We would need to make sure that we
> close() the master fd at some point though--maybe after OnStopRequest has
> been called?  The destructor might work instead, but sometimes channels get
> kept around a long time, and I'd hope that after OnStopRequest would be good
> enough.
> 
> ::: modules/libjar/nsIJARChannel.idl
> @@ +31,5 @@
> > +    /**
> > +     * When aPreserveRemoteFD enabled, the JAR cache will not be used, and
> > +     * the fd from remote file open will be preserved for further need.
> > +     */
> > +    void setPreserveRemoteFD(in bool aPreserveRemoteFD);
> 
> This flag also causes the JARChannel to not deliver any data (i.e. only
> OnStart/Stop are called, no OnDataAvailable).  So it's really a special mode
> where we only want to get the fd and not the data.  Let's call it something
> like
> 
>   /**
>    * When called, the JAR channel is to be used only to get the underlying
>    * file descriptor.  OnStart/OnStopRequest will be called, but not 
>    * OnDataAvailable.
>    *
>   void SetFdOnlyMode()  // not sure it even needs a bool? It's not meant to
> be set/unset multiple times
> 
> This seems better to me, but I'm not a JAR channel peer--so I guess we
> should ask :aklotz or :mwu.
> 
> As I mention below, I'm also unclear as to whether the patches this depends
> on (non-e10s version of this code) also omit calling OnDataAvailable.  It
> would be good to make them behave the same way.
> 
> ::: modules/libjar/nsJARChannel.cpp
> @@ +354,5 @@
> >              NS_ENSURE_SUCCESS(rv, rv);
> >              mJarFile = remoteFile;
> >  
> >              nsIZipReaderCache *jarCache = gJarHandler->JarCache();
> > +            if (jarCache && !mPreserveRemoteFD) {
> 
> You also need to check for !mPreserveRemoteFD (or mFdOnlyMode, after the IDL
> change) below where
> 
>   if (gJarHandler->RemoteOpenFileInProgress(...)
> 
> gets called, right?  Otherwise you could somehow get a JAR cache hit (and
> thus no fd) if you request the same JAR once w/o FdOnlyMode, and then with
> it.  That's actually likely if we're loading from the same application.zip
> file that the app bootstraps from.

Right, I missed this one.

> 
> @@ +962,5 @@
> > +    if (mPreserveRemoteFD) {
> > +        rv = OnStartRequest(nullptr, nullptr);
> > +        if (NS_SUCCEEDED(rv)) {
> > +            OnStopRequest(nullptr, nullptr, NS_OK);
> > +        }
> 
> So how did this code work in the non-e10s version?  Did you change the JAR
> file code there to somehow not call OnDataAvailable?  There's a lot of
> patches in bug 945152, but I didn't seem to see anything that removes
> OnDataAvailable from being delivered?

In this patch, we only switch to SetPreserveRemoteFD(or FdOnlyMode) when running in child process.

  if (XRE_GetProcessType() != GeckoProcessType_Default) {
    nsCOMPtr<nsIJARChannel> jarChannel = do_QueryInterface(mChannel);
    // For memory mapping, we need to get file descriptor of JAR file
    // opened remotely on parent process. Set this on JAR channel to
    // preserve the remote file descriptor after been opened or cloned.
    jarChannel->SetPreserveRemoteFD(true);
  }

> 
> ::: netwerk/ipc/RemoteOpenFileChild.cpp
> @@ +244,5 @@
> > +nsresult
> > +RemoteOpenFileChild::SetPreserveRemoteFD(bool aPreserveRemoteFD)
> > +{
> > +  mPreserveRemoteFD = aPreserveRemoteFD;
> > +  return NS_OK;
> 
> So if we can dup() the fd as I mentioned, I think we can get rid of
> SetPreserveRemoteFD in RemoteOpenFileChild, and only need a special flag for
> JARChannel (the FdOnlyMode I proposed).  RemoteOpenFileChild would just open
> the file as many times as you want automatically.

Based on more testing, I found that we actually need to use the same JAR channel to get fd(for mmap) as well as reading data(if mmap fails).  It appears that such thing cannot be acomplished by SetPreserveRemoteFD(or FdOnlyMode), which have to be decieded in the beginning.
So it seems we chould use the cleaner way by dup() in RemoteOpenFileChild during Clone or OpenNSPRFileDesc.  Thank you for the suggestions!
Comment on attachment 8434062 [details] [diff] [review]
Patch: Get fd from child process by remote file open.

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

(In reply to Jason Duell (:jduell) from comment #4)
> ::: modules/libjar/nsIJARChannel.idl
> @@ +31,5 @@
> > +    /**
> > +     * When aPreserveRemoteFD enabled, the JAR cache will not be used, and
> > +     * the fd from remote file open will be preserved for further need.
> > +     */
> > +    void setPreserveRemoteFD(in bool aPreserveRemoteFD);
> 
> This flag also causes the JARChannel to not deliver any data (i.e. only
> OnStart/Stop are called, no OnDataAvailable).  So it's really a special mode
> where we only want to get the fd and not the data.  Let's call it something
> like
> 
>   /**
>    * When called, the JAR channel is to be used only to get the underlying
>    * file descriptor.  OnStart/OnStopRequest will be called, but not 
>    * OnDataAvailable.
>    *
>   void SetFdOnlyMode()  // not sure it even needs a bool? It's not meant to
> be set/unset multiple times
> 
> This seems better to me, but I'm not a JAR channel peer--so I guess we
> should ask :aklotz or :mwu.

I agree -- I think this clarifies this method immensely.
Attachment #8434062 - Flags: feedback?(aklotz) → feedback+
This WIP patch implements multiple remote open file by dup() during open and clone.  Testing looks good.

The patch removed the JAR cache for remote open file.  I am not sure if we still need JAR cache when we have multiple remote open file support.  Do you think we need to have something like SetNoCacheMode()?  So the user can choose between 1) multiple open with JAR cache disabled, or 2) single open using JAR cache enabled.

TODO:
1. RemoteOpenFileChild::Close() cannot be called in nsJARChannel::OnStopRequest(), under checking.
Assignee: nobody → swu
Status: NEW → ASSIGNED
Flags: needinfo?(jduell.mcbugs)
Comment on attachment 8439176 [details] [diff] [review]
(WIP) Patch: Support multiple remote open file.

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

::: modules/libjar/nsJARChannel.cpp
@@ -359,5 @@
> -                rv = jarCache->IsCached(mJarFile, &cached);
> -                if (NS_SUCCEEDED(rv) && cached) {
> -                    // zipcache already has file mmapped: don't open on parent,
> -                    // just return and proceed to cache hit in CreateJarInput()
> -                    return NS_OK;

No, we can't remove this code! :)   If we do that then every load for every app:// URI will have to wait for the parent to (re)open the jar file, instead of getting an instant mmap hit for the resource on the child.  I.e. it would absolutely kill app performance on B2G.  With this optimization we only do RemoteOpenFile for the application.zip file once per app.

> Do you think we need to have something like SetNoCacheMode()?  
> So the user can choose between 1) multiple open with JAR cache 
> disabled, or 2) single open using JAR cache enabled.

We need to have that, on a per JARChannel basis. #1 is the default, and you'll get #2 when you set SetFdOnlyMode() mode.

One other possible architecture for a fix: we could try to teach the JAR cache to keep the fd for the JAR file open if we pass it a flag (right now it mmaps the fd, then closes the fd), and then we could have some API that queries the fd from the jar cache

::: netwerk/ipc/RemoteOpenFileChild.cpp
@@ +253,5 @@
> +{
> +   if (mNSPRFileDesc) {
> +printf_stderr("====== %s %d close fd=%x\n", __FUNCTION__, __LINE__, mNSPRFileDesc);
> +    // PR_Close both closes the file and deallocates the PRFileDesc
> +    PR_Close(mNSPRFileDesc);

Do you know when Close gets called?  I'm worried about keeping the fd open for a long time.  But I'm not sure how long we keep RemoteOpenFileChild objects alive.
Attachment #8439176 - Flags: feedback-
>  #1 is the default, and you'll get #2 when you set SetFdOnlyMode() mode.

Um,  actually I meant the opposite :)

> Based on more testing, I found that we actually need to use the same JAR channel to get fd(for mmap) as well as reading data(if mmap fails).

So you're actually seeing mmap fail?  Any idea why?  That seems weird.

>> So how did this code work in the non-e10s version?  Did you change the JAR
>> file code there to somehow not call OnDataAvailable?  There's a lot of
>> patches in bug 945152, but I didn't seem to see anything that removes
>> OnDataAvailable from being delivered?
>
> In this patch, we only switch to SetPreserveRemoteFD(or FdOnlyMode) when running 
> in child process.

OK, so then on the parent process you wind up somehow getting the fd, and also you get all the OnDataAvailable calls?  We should probably try to work the same way on both child/parent in that respect.
Flags: needinfo?(jduell.mcbugs)
(In reply to Jason Duell (:jduell) from comment #8)
> Comment on attachment 8439176 [details] [diff] [review]
> (WIP) Patch: Support multiple remote open file.
> 
> Review of attachment 8439176 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: modules/libjar/nsJARChannel.cpp
> @@ -359,5 @@
> > -                rv = jarCache->IsCached(mJarFile, &cached);
> > -                if (NS_SUCCEEDED(rv) && cached) {
> > -                    // zipcache already has file mmapped: don't open on parent,
> > -                    // just return and proceed to cache hit in CreateJarInput()
> > -                    return NS_OK;
> 
> No, we can't remove this code! :)   If we do that then every load for every
> app:// URI will have to wait for the parent to (re)open the jar file,
> instead of getting an instant mmap hit for the resource on the child.  I.e.
> it would absolutely kill app performance on B2G.  With this optimization we
> only do RemoteOpenFile for the application.zip file once per app.

Ah, got it!  Re-opening JAR file on parent every time is definitely not what we want.
 
> One other possible architecture for a fix: we could try to teach the JAR
> cache to keep the fd for the JAR file open if we pass it a flag (right now
> it mmaps the fd, then closes the fd), and then we could have some API that
> queries the fd from the jar cache

Yes, this is what we need.  
I have been thinking about something like nsJARChannel::GetNSPRFileDesc() to get fd from JAR cache.
What I hope to further do, instead, is getting fd by OpenNSPRFileDesc() from mJarFile of nsJARChannel, while the fd is dupped from JAR cache instead of re-opening on parent side.  In such way we'll have a clean solution.

> 
> ::: netwerk/ipc/RemoteOpenFileChild.cpp
> @@ +253,5 @@
> > +{
> > +   if (mNSPRFileDesc) {
> > +printf_stderr("====== %s %d close fd=%x\n", __FUNCTION__, __LINE__, mNSPRFileDesc);
> > +    // PR_Close both closes the file and deallocates the PRFileDesc
> > +    PR_Close(mNSPRFileDesc);
> 
> Do you know when Close gets called?  I'm worried about keeping the fd open
> for a long time.  But I'm not sure how long we keep RemoteOpenFileChild
> objects alive.

It is called in nsJARChannel::OnStopRequest() of this patch.  Somehow it's not working yet, because I always got null remoteFile when querying from non-null mJarFile.
Maybe you have some idea?

  if (mJarFile && mOpeningRemote) {
    nsRefPtr<RemoteOpenFileChild> remoteFile = do_QueryObject(mJarFile);
    if (remoteFile) {
      remoteFile->Close();
    }
  }

By the way, if we keep fd in JAR cache, then for each application.zip there will be only one master fd shared by multiple JAR channels.  In such condition, we don't need to close it in OnStopRequest(), and don't even close in RemoteOpenFileChild destructor, just let the fd be closed when JAR cache timeout, does it make sense?
(In reply to Jason Duell (:jduell) from comment #9)
> > Based on more testing, I found that we actually need to use the same JAR channel to get fd(for mmap) as well as reading data(if mmap fails).
> 
> So you're actually seeing mmap fail?  Any idea why?  That seems weird.

The implementation was landed as below patch, FYI.
https://hg.mozilla.org/mozilla-central/rev/ff88f3065432

We need to fallback non-mmap way if we found the target file is compressed, or any other during mapping, for example: target file offset in application.zip is not aligned.

To summerize procedure as below:
1. Create JAR channel.
2. AsyncOpen() target file on JAR channel.
3. After opened, get nsIFile pointer of target file(mJarFile) from JAR channel.
4. Open and read target file information by nsZipArchive::OpenArchive(), which would call OpenNSPRFileDesc.
5. If the file is not compresssed, use to offset and size information to do mmap.  (Otherwise return failure)
6. Call OpenNSPRFileDesc() on target file to get fd.
7. Do mmap by fd/offset/size.  (It could fail if offset is not aligned)

We need to call OpenNSPRFileDesc twice(step 4 & 6), and we need to fallback and still been able to read target file from cache when it fails in step 5 or 7.

> 
> >> So how did this code work in the non-e10s version?  Did you change the JAR
> >> file code there to somehow not call OnDataAvailable?  There's a lot of
> >> patches in bug 945152, but I didn't seem to see anything that removes
> >> OnDataAvailable from being delivered?
> >
> > In this patch, we only switch to SetPreserveRemoteFD(or FdOnlyMode) when running 
> > in child process.
> 
> OK, so then on the parent process you wind up somehow getting the fd, and
> also you get all the OnDataAvailable calls?  We should probably try to work
> the same way on both child/parent in that respect.

On the parrent process, we don't need special way to get fd.  The OpenNSPRFileDesc will be resulted in a local file open, so it can be called many times.  For child process, if we allow multiple OpenNSPRFileDesc calls, then we can have same implementation for both child/parent.
Flags: needinfo?(jduell.mcbugs)
> We need to fallback non-mmap way if we found the target file is compressed

Ah, ok. 

> On the parrent process, we don't need special way to get fd.  The
> OpenNSPRFileDesc will be resulted in a local file open, so it can be called
> many times.  For child process, if we allow multiple OpenNSPRFileDesc calls,
> then we can have same implementation for both child/parent.

It seems like you are getting close to a patch that makes child able to open multiple times. So that sounds good.

> I have been thinking about something like nsJARChannel::GetNSPRFileDesc() to
> get fd from JAR cache.  What I hope to further do, instead, is getting fd by
> OpenNSPRFileDesc() from mJarFile of nsJARChannel, while the fd is dupped from
> JAR cache instead of re-opening on parent side.  In such way we'll have a
> clean solution.

So of the two approaches, 1) caching fd's in the jar cache, or 2) using dup() to allow child to just get mJarFile and open it, there are tradeoffs and I'm not sure which is better.  #1 could potentially allow jar cache to keep a lot of open file descriptors.  OTOH we're mainly just opening one JAR file (application.zip), and if we keep the fd in the jar cache then we wouldn't need to even do any IPDL or I/O on the parent, so it would be faster.  Hmm, maybe #1 is better then :)  But if performance here is not such a big deal (i.e we're not doing this a lot, and not blocking the whole child app on the load) then either #1 or #2 should be fine, and #2 is less changes to the API and probably less code.  Implementors choice? :)
Flags: needinfo?(jduell.mcbugs)
It's much safer to dup fds than dabble with JAR Cache.
(In reply to Jason Duell (:jduell) from comment #12)
> So of the two approaches, 1) caching fd's in the jar cache, or 2) using
> dup() to allow child to just get mJarFile and open it, there are tradeoffs
> and I'm not sure which is better.  #1 could potentially allow jar cache to
> keep a lot of open file descriptors.  OTOH we're mainly just opening one JAR
> file (application.zip), and if we keep the fd in the jar cache then we
> wouldn't need to even do any IPDL or I/O on the parent, so it would be
> faster.  Hmm, maybe #1 is better then :)  But if performance here is not
> such a big deal (i.e we're not doing this a lot, and not blocking the whole
> child app on the load) then either #1 or #2 should be fine, and #2 is less
> changes to the API and probably less code.  Implementors choice? :)

Both #1 and #2 require using dup() in RemoteOpenFileChild::OpenNSPRFileDesc(), because OpenNSPRFileDesc() might be called more than once, and it's caller's responsibility to close after using.  The difference between these two is "dup from fd of JAR cache" or "dup from fd of a new remote open on parent", in either case we don't want master fd to be closed by caller.

Originally I thought we could not make new remote open and read data from JAR cache at the same time.  But with deeper understanding about the code, I realized that both can be done in same JAR channel.  So, the remote open is only for the purpose of getting fd for mmap and checking zip file information on offset/size/compression to decide whether to do mmap.  If later it falls back to non-mmap way, the data can still being read from JAR cache.

Given all these considerations and comment 13, it seems better to go with #2 for now, unless we found any performance concerns later.
The patch implemented method #2 as discussed.  I changed SetFdOnlyMode() to SetFdMode() to better reflect the behavior, because we still read data from JAR cache, please advice if the naming is ok.
Attachment #8419303 - Attachment is obsolete: true
Attachment #8434062 - Attachment is obsolete: true
Attachment #8439176 - Attachment is obsolete: true
Attachment #8440652 - Flags: feedback?(jduell.mcbugs)
Attachment #8440652 - Flags: feedback?(aklotz)
Marco, in bug 998243, you teached me how to mock WebappOSUtils.getPackagePath() for mochitest to load packaged app in an iframe.  However when launching the app with "remote=true", the mock skill doesn't work and I have to temporarily harding-code it in WebappOSUtils.jsm for testing.  Do you have any suggestions?

Jason, besides this mochitest, any other test in your mind that we should do?
Flags: needinfo?(mar.castelluccio)
Flags: needinfo?(jduell.mcbugs)
(In reply to Shian-Yow Wu [:shianyow] from comment #16)
> Created attachment 8440659 [details] [diff] [review]
> (WIP) Patch: Test case to read memory-mapped array buffer in OOP.
> 
> Marco, in bug 998243, you teached me how to mock
> WebappOSUtils.getPackagePath() for mochitest to load packaged app in an
> iframe.  However when launching the app with "remote=true", the mock skill
> doesn't work and I have to temporarily harding-code it in WebappOSUtils.jsm
> for testing.  Do you have any suggestions?

I'm not sure how to do that, I guess the problem is that mocking the
object in one process leaves the object in the other process intact.

I think bug 910473 will help here, because only the parent will call
|WebappOSUtils.getInstallPath| and will hand the path to the child
processes.
Flags: needinfo?(mar.castelluccio)
(In reply to Marco Castelluccio [:marco] from comment #17)
> I think bug 910473 will help here, because only the parent will call
> |WebappOSUtils.getInstallPath| and will hand the path to the child
> processes.

Thank you for this information, I'll reopen bug 998243 for tracking and further discussion.
Depends on: 998243
Comment on attachment 8440652 [details] [diff] [review]
Patch: Support multiple remote open file.

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

> It's much safer to dup fds than dabble with JAR Cache.

I'm not sure I agree with Taras here--it can't be that hard to not close the fd when we mmap a jar cache entry, and then just return it.  If this code is in a perf-critical pathway (esp if we call it lots of times) it would be much faster than going back to the parent again and again.

That said--if we're going to go with the remote approach, this patch looks good!  I have only one question.  If I'm misguided about it, feel free to mark the patch +r from me without another review. (also +r if I'm right and you make the simple change).

::: modules/libjar/nsIJARChannel.idl
@@ +32,5 @@
> +     * For child process, set this to make sure that a valid file descriptor of
> +     * JAR file is always provided when calling NSPRFileDesc().
> +     * Must be set before Open() or AsyncOpen() to be effective.
> +     */
> +    void setFdMode();

I think I'd call this 'ensureChildFd()'.  Not a big deal.

::: netwerk/ipc/RemoteOpenFileChild.cpp
@@ +77,5 @@
>  {
> +#if defined(XP_WIN) || defined(MOZ_WIDGET_COCOA)
> +#else
> +  if (mNSPRFileDesc) {
> +    PROsfd osfd = dup(PR_FileDesc2NativeHandle(mNSPRFileDesc));

Don't you want to be checking/dup()'ing 'other.mNSPRFileDesc'?  I.e. the fd should be valid in the 'other' object that we're initializing from.
Attachment #8440652 - Flags: feedback?(jduell.mcbugs) → feedback+
> Jason, besides this mochitest, any other test in your mind that we should do?

I don't actually know exactly how you're using the fd on the child, so I don't have an idea for a test.
Flags: needinfo?(jduell.mcbugs)
Sorry, my last comment got cut off--it sounds like we need to test with typedArrays somehow, as is needed for bug 945152.
(In reply to Jason Duell (:jduell) from comment #19)
> Comment on attachment 8440652 [details] [diff] [review]
> Patch: Support multiple remote open file.
> 
> Review of attachment 8440652 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > It's much safer to dup fds than dabble with JAR Cache.
> 
> I'm not sure I agree with Taras here--it can't be that hard to not close the
> fd when we mmap a jar cache entry, and then just return it.  If this code is
> in a perf-critical pathway (esp if we call it lots of times) it would be
> much faster than going back to the parent again and again.
> 
> That said--if we're going to go with the remote approach, this patch looks
> good!  I have only one question.  If I'm misguided about it, feel free to
> mark the patch +r from me without another review. (also +r if I'm right and
> you make the simple change).

Thank you for the review.  I am also working on a patch on top of this one, which stores fd in JAR cache.  Later we can see whether to land this patch first or wait for the patch to use fd from JAR cache.

> 
> ::: modules/libjar/nsIJARChannel.idl
> @@ +32,5 @@
> > +     * For child process, set this to make sure that a valid file descriptor of
> > +     * JAR file is always provided when calling NSPRFileDesc().
> > +     * Must be set before Open() or AsyncOpen() to be effective.
> > +     */
> > +    void setFdMode();
> 
> I think I'd call this 'ensureChildFd()'.  Not a big deal.

It seems more clear with ensureChildFd(), thanks for your suggestion.

> 
> ::: netwerk/ipc/RemoteOpenFileChild.cpp
> @@ +77,5 @@
> >  {
> > +#if defined(XP_WIN) || defined(MOZ_WIDGET_COCOA)
> > +#else
> > +  if (mNSPRFileDesc) {
> > +    PROsfd osfd = dup(PR_FileDesc2NativeHandle(mNSPRFileDesc));
> 
> Don't you want to be checking/dup()'ing 'other.mNSPRFileDesc'?  I.e. the fd
> should be valid in the 'other' object that we're initializing from.

The mNSPRFileDesc was initialized from other.mNSPRFileDesc, so the result will 
be the same.  If I understand correctly, you mean that 'other.mNSPRFileDesc' will
be more clear for code reading, that makes sense.
(In reply to Jason Duell (:jduell) from comment #21)
> Sorry, my last comment got cut off--it sounds like we need to test with
> typedArrays somehow, as is needed for bug 945152.

Yes, array buffer test is covered in the WIP patch of the test case as attached,
which is doing the samething as in bug 945152, but in OOP.
This patch addressed the comments reviewed by Jason.

Aaron, could you review on the IDL changes on JAR channel?
Attachment #8440652 - Attachment is obsolete: true
Attachment #8440652 - Flags: feedback?(aklotz)
Attachment #8443462 - Flags: review?(aklotz)
Corrected some typos.
Attachment #8443462 - Attachment is obsolete: true
Attachment #8443462 - Flags: review?(aklotz)
Attachment #8443464 - Flags: review?(aklotz)
Comment on attachment 8443464 [details] [diff] [review]
Patch: Support multiple OpenNSPRFileOpen() on RemoteOpenFile. (r=jduell)

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

r+ for IDL bits.
Attachment #8443464 - Flags: review?(aklotz) → review+
(In reply to Aaron Klotz [:aklotz] from comment #26)
> Comment on attachment 8443464 [details] [diff] [review]
> Patch: Support multiple OpenNSPRFileOpen() on RemoteOpenFile. (r=jduell)
> 
> Review of attachment 8443464 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ for IDL bits.

Thanks, Aaron.
(In reply to Shian-Yow Wu [:shianyow] from comment #22)
> (In reply to Jason Duell (:jduell) from comment #19)
> > Comment on attachment 8440652 [details] [diff] [review]
> > Patch: Support multiple remote open file.
> > 
> > Review of attachment 8440652 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > > It's much safer to dup fds than dabble with JAR Cache.
> > 
> > I'm not sure I agree with Taras here--it can't be that hard to not close the
> > fd when we mmap a jar cache entry, and then just return it.  If this code is
> > in a perf-critical pathway (esp if we call it lots of times) it would be
> > much faster than going back to the parent again and again.
> > 
> > That said--if we're going to go with the remote approach, this patch looks
> > good!  I have only one question.  If I'm misguided about it, feel free to
> > mark the patch +r from me without another review. (also +r if I'm right and
> > you make the simple change).
> 
> Thank you for the review.  I am also working on a patch on top of this one,
> which stores fd in JAR cache.  Later we can see whether to land this patch
> first or wait for the patch to use fd from JAR cache.
> 
> > 
> > ::: modules/libjar/nsIJARChannel.idl
> > @@ +32,5 @@
> > > +     * For child process, set this to make sure that a valid file descriptor of
> > > +     * JAR file is always provided when calling NSPRFileDesc().
> > > +     * Must be set before Open() or AsyncOpen() to be effective.
> > > +     */
> > > +    void setFdMode();
> > 
> > I think I'd call this 'ensureChildFd()'.  Not a big deal.
> 
> It seems more clear with ensureChildFd(), thanks for your suggestion.
> 
> > 
> > ::: netwerk/ipc/RemoteOpenFileChild.cpp
> > @@ +77,5 @@
> > >  {
> > > +#if defined(XP_WIN) || defined(MOZ_WIDGET_COCOA)
> > > +#else
> > > +  if (mNSPRFileDesc) {
> > > +    PROsfd osfd = dup(PR_FileDesc2NativeHandle(mNSPRFileDesc));
> > 
> > Don't you want to be checking/dup()'ing 'other.mNSPRFileDesc'?  I.e. the fd
> > should be valid in the 'other' object that we're initializing from.
> 
> The mNSPRFileDesc was initialized from other.mNSPRFileDesc, so the result
> will 
> be the same.  If I understand correctly, you mean that 'other.mNSPRFileDesc'
> will
> be more clear for code reading, that makes sense.

Oh, forget to mention, I added comment and assertion in Windows/OSX #ifdef
condition to make it clear.
As my understanding, Windows/OSX don't do remote open, so there will never be
the file descriptor in RemoteOpenFile class.  Please let me know if you think
otherwise, thanks!

#if defined(XP_WIN) || defined(MOZ_WIDGET_COCOA)
  // Windows/OSX desktop builds skip remoting, so the file descriptor should
  // be nullptr here.
  MOZ_ASSERT(!other.mNSPRFileDesc);
#else
  if (other.mNSPRFileDesc) {
    PROsfd osfd = dup(PR_FileDesc2NativeHandle(other.mNSPRFileDesc));
    mNSPRFileDesc = PR_ImportFile(osfd);
  }
#endif
Flags: needinfo?(jduell.mcbugs)
Comment on attachment 8443464 [details] [diff] [review]
Patch: Support multiple OpenNSPRFileOpen() on RemoteOpenFile. (r=jduell)

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

> The mNSPRFileDesc was initialized from other.mNSPRFileDesc, so the result will
> be the same.  If I understand correctly, you mean that 'other.mNSPRFileDesc'
> will be more clear for code reading,

Ah, I didn't follow that.  Yes, I think the way you have it now--where we use
other.fd--is easier to read.

::: netwerk/ipc/RemoteOpenFileChild.cpp
@@ +71,5 @@
>                    nsICachedFileDescriptorListener)
>  
>  RemoteOpenFileChild::RemoteOpenFileChild(const RemoteOpenFileChild& other)
>    : mTabChild(other.mTabChild)
>    , mNSPRFileDesc(other.mNSPRFileDesc)

I'd just take out the initializer for mNSPRFileDesc here, since this initialization does nothing useful (we replace it with the dup'd fd) it just makes the logic harder to follow.
Attachment #8443464 - Flags: review+
Flags: needinfo?(jduell.mcbugs)
(In reply to Jason Duell (:jduell) from comment #29)
> >  RemoteOpenFileChild::RemoteOpenFileChild(const RemoteOpenFileChild& other)
> >    : mTabChild(other.mTabChild)
> >    , mNSPRFileDesc(other.mNSPRFileDesc)
> 
> I'd just take out the initializer for mNSPRFileDesc here, since this
> initialization does nothing useful (we replace it with the dup'd fd) it just
> makes the logic harder to follow.

Got it, thanks.  Then the best way would be initalizing it to nullptr(needed for Windows/OSX).
Attachment #8440659 - Attachment is obsolete: true
Attachment #8447872 - Flags: review?(jduell.mcbugs)
The patch applies on top of previously multiple remote open file patch, it allows to keep fd in JAR cache when the flag in nsZipReaderCache::KeepFd() is enabled, and uses nsZipReaderCache::GetFd() to obtain the fd from JAR cache.

I spent some time thinking whether we need KeepFd().  For each process, the JAR cache size is 32.  If the total process number isn't too much, it might be OK to enable keeping fd in JAR cache by system default.  Since only remote open condition needs fd in JAR cache, to save fd allocation, I think it's better to have KeepFd() to enable it only when needed.  This feature is enabled per JAR cache, it makes the design simpler than enabling per JAR file.

Note that the feature currently doesn't support Windows platform.  Because in nsZipHandle:Init(), the file descriptor will be returned for other purpose
on Windows, so the mKeepFd flag is ignored by this patch for safety.  And we don't need fd for remote open on Windows platform.

Aaron and Jason, could you help to review it?  I hope the comments in the code are explained enough.  If anything unclear, please let me know, thank you!
Attachment #8447874 - Flags: review?(jduell.mcbugs)
Attachment #8447874 - Flags: review?(aklotz)
Comment on attachment 8447874 [details] [diff] [review]
Patch: Allow to keep file descriptor in JAR cache.

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

Looks good for the most part. I would like to see it once more, and of course jduell should still take a look as well.

::: modules/libjar/nsIZipReader.idl
@@ +240,5 @@
> +     * When aKeepFd is enabled and a file is given, the file will be flushed
> +     * from the cache if its file descriptor was not cached.
> +     * Note: currently not supported on Windows platform.
> +     */
> +    void keepFd(in nsIFile zipFile, in bool aKeepFd);

I think we need a better name for this. The doc comments are clear, but when somebody looks at the implementation without reading these doc comments they will see data being flushed instead of being kept, which is kind of confusing. How about setMustCacheFd ? Other suggestions welcome.

::: modules/libjar/nsJAR.cpp
@@ +393,5 @@
>  
> +nsresult
> +nsJAR::GetNSPRFileDesc(PRFileDesc** aNSPRFileDesc)
> +{
> +  if (mZip) {

Can you please change the control flow in this function to return immediately on error as per style guidelines?
if (!mZip) {
  return NS_ERROR_FAILURE;
}
etc.

@@ +1182,5 @@
> +  return NS_ERROR_NOT_IMPLEMENTED;
> +#else
> +  mKeepFd = aKeepFd;
> +
> +  if (aKeepFd) {

Can you change this to return on !aKeepFd so that we don't need to have most of this function's code inside an if block?

@@ +1183,5 @@
> +#else
> +  mKeepFd = aKeepFd;
> +
> +  if (aKeepFd) {
> +    NS_ENSURE_ARG_POINTER(zipFile);

No NS_ENSURE* macros in new code, please.

@@ +1196,5 @@
> +    MutexAutoLock lock(mLock);
> +    nsRefPtr<nsJAR> zip;
> +    mZips.Get(uri, getter_AddRefs(zip));
> +    // Flush the file from the cache if its file descriptor was not cached.
> +    if (zip) {

Return on !zip instead

@@ +1220,5 @@
> +#if defined(XP_WIN)
> +  MOZ_CRASH("Not implemented");
> +  return NS_ERROR_NOT_IMPLEMENTED;
> +#else
> +  NS_ENSURE_ARG_POINTER(zipFile);

NS_ENSURE again

@@ +1233,5 @@
> +
> +  MutexAutoLock lock(mLock);
> +  nsRefPtr<nsJAR> zip;
> +  mZips.Get(uri, getter_AddRefs(zip));
> +  if (zip) {

Change this to return on !zip instead

::: modules/libjar/nsJAR.h
@@ +196,5 @@
>    nsZipReaderCache();
>  
>    nsresult ReleaseZip(nsJAR* reader);
>  
> +  bool IsKeepFdEnabled() {

IsMustCacheFdEnabled() or whatever is necessary to keep this function's name in sync with the name of the setter.

::: modules/libjar/nsJARChannel.cpp
@@ +373,3 @@
>                      return NS_OK;
> +                    #else
> +                    if (mEnsureChildFd) {

if (!mEnsureChildFd) {
  return NS_OK;
}
Then you can simplify this block a bit.

::: modules/libjar/nsZipArchive.cpp
@@ +264,5 @@
>    }
> +  if (mNSPRFileDesc) {
> +    PR_Close(mNSPRFileDesc);
> +    mNSPRFileDesc = nullptr;
> +  }

This is fine, however if you declare mNSPRFileDesc as a mozilla::AutoFDClose from xpcom/glue/FileUtils.h you could simplify this. Since nsZipArchive is already using AutoFDClose elsewhere, I'd suggest you do the same here.

::: modules/libjar/nsZipArchive.h
@@ +402,5 @@
>    nsZipHandle();
>    ~nsZipHandle();
>  
>    PRFileMap *                       mMap;    /* nspr datastructure for mmap */
> +  PRFileDesc*                       mNSPRFileDesc;

Use mozilla::AutoFDClose here as I already mentioned.
Attachment #8447874 - Flags: review?(aklotz) → review-
(In reply to Aaron Klotz [:aklotz] from comment #34)
> I think we need a better name for this. The doc comments are clear, but when
> somebody looks at the implementation without reading these doc comments they
> will see data being flushed instead of being kept, which is kind of
> confusing. How about setMustCacheFd ? Other suggestions welcome.

The name setMustCacheFd is more clear to me and I cannot think a better name.
Thank you. :)

> Can you please change the control flow in this function to return
> immediately on error as per style guidelines?
> if (!mZip) {
>   return NS_ERROR_FAILURE;
> }
> etc.

Thank you for reminding this here and elsewhere.

> No NS_ENSURE* macros in new code, please.

Got it.

> ::: modules/libjar/nsZipArchive.cpp
> @@ +264,5 @@
> >    }
> > +  if (mNSPRFileDesc) {
> > +    PR_Close(mNSPRFileDesc);
> > +    mNSPRFileDesc = nullptr;
> > +  }
> 
> This is fine, however if you declare mNSPRFileDesc as a mozilla::AutoFDClose
> from xpcom/glue/FileUtils.h you could simplify this. Since nsZipArchive is
> already using AutoFDClose elsewhere, I'd suggest you do the same here.

Good to have this simplified, thank you for this suggestion.
Addressed previous review comments.
Attachment #8447874 - Attachment is obsolete: true
Attachment #8447874 - Flags: review?(jduell.mcbugs)
Attachment #8450729 - Flags: review?(jduell.mcbugs)
Attachment #8450729 - Flags: review?(aklotz)
Comment on attachment 8450729 [details] [diff] [review]
Patch(v2): Allow to keep file descriptor in JAR cache.

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

I'm happy with this, just a few nits.

::: modules/libjar/nsJAR.cpp
@@ +392,5 @@
>  }
>  
> +nsresult
> +nsJAR::GetNSPRFileDesc(PRFileDesc** aNSPRFileDesc)
> +{

Can you add a check that aNSPRFileDesc is not nullptr here, and return NS_ERROR_ILLEGAL_VALUE if it is?
After this check, please then set *aNSPRFileDesc = nullptr.

@@ +1196,5 @@
> +
> +  nsresult rv;
> +  nsAutoCString uri;
> +  rv = zipFile->GetNativePath(uri);
> +  if (NS_FAILED(rv))

Nit: Please add curly braces around if statement.

@@ +1208,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  // Flush the file from the cache if its file descriptor was not cached.
> +  PRFileDesc* fd;

Initialize to nullptr

::: modules/libjar/nsJARChannel.cpp
@@ +377,5 @@
> +                    }
> +                    PRFileDesc *fd = nullptr;
> +                    jarCache->GetFd(mJarFile, &fd);
> +                    if (fd) {
> +                        PROsfd osfd = dup(PR_FileDesc2NativeHandle(fd));

Please add a check to make sure that dup didn't fail and return accordingly if it did.

::: modules/libjar/nsZipArchive.cpp
@@ +162,5 @@
>  nsZipHandle::nsZipHandle()
>    : mFileData(nullptr)
>    , mLen(0)
>    , mMap(nullptr)
> +  , mNSPRFileDesc(nullptr)

This isn't necessary when using AutoFDClose.

@@ +250,5 @@
>      return mLen;
>  }
>  
> +nsresult nsZipHandle::GetNSPRFileDesc(PRFileDesc** aNSPRFileDesc)
> +{

Check that aNSPRFileDesc != nullptr
Attachment #8450729 - Flags: review?(aklotz) → review+
Comment on attachment 8450729 [details] [diff] [review]
Patch(v2): Allow to keep file descriptor in JAR cache.

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

This looks good to me.
Attachment #8450729 - Flags: review?(jduell.mcbugs) → review+
Comment on attachment 8447872 [details] [diff] [review]
Patch: Test case to read memory-mapped array buffer in OOP.

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

Sorry, I don't know this test code at all, and I'm going on PTO for a week.  Best to find another reviewer (hg log says fabrice/smaug, maybe aklotz can do it?).
Attachment #8447872 - Flags: review?(jduell.mcbugs)
Comment on attachment 8447872 [details] [diff] [review]
Patch: Test case to read memory-mapped array buffer in OOP.

Fabrice, could you help to review this test case?
Attachment #8447872 - Flags: review?(fabrice)
Addressed previous review comments and carrying r+ from aklotz and jduell.
Attachment #8450729 - Attachment is obsolete: true
Attachment #8457688 - Flags: review+
Comment on attachment 8447872 [details] [diff] [review]
Patch: Test case to read memory-mapped array buffer in OOP.

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

::: dom/apps/tests/test_bug_945152.html
@@ +91,5 @@
>      };
>      yield undefined;
>  
> +    // Launch app non-OOP.
> +    ok(true, "Launch app with non-OOP");

use info("Launch app with non-OOP") instead.

@@ +96,5 @@
> +    launchApp(app, continueTest, false);
> +    yield undefined;
> +
> +    // Launch app OOP.
> +    ok(true, "Launch app with OOP");

same here.
Attachment #8447872 - Flags: review?(fabrice) → review+
Addressed review comments and carrying r+ from fabrice.
Attachment #8447872 - Attachment is obsolete: true
Attachment #8458398 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/8912d2088eee
https://hg.mozilla.org/mozilla-central/rev/f7de657a37ed
https://hg.mozilla.org/mozilla-central/rev/a5446bb5fa7d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Depends on: 1048615
Depends on: 1055113
The file descriptor returned by dup() shares its file offset with the original — i.e., lseek/read/write on one descriptor will affect all duplicates, which is not the case for separate open() calls.  This means that RemoteFileOpenChild behaves differently from nsLocalFile in the case of multiple opens, and can be safely used only with operations that take absolute offsets (mmap, pread, pwrite — or however NSPR presents those).

As far as I can see, there are no bug comments or code comments warning about this limitation.  Is it deliberate?  Will it break things in the future?  Is the mustCacheFd patch the only reason it's not already breaking things?
Flags: needinfo?(jduell.mcbugs)
Flags: needinfo?(aklotz)
See comment 47.
Flags: needinfo?(aklotz) → needinfo?(swu)
Jed:  Very nice catch--thanks. I hasn't realized dup() winds up sharing the same open file descriptor.  I think that was OK (i.e. no surprises) for the initial use of RemoteOpenFile: the JAR channel Clones() whatever nsIFile we give it for thread-safety reasons:

   http://mxr.mozilla.org/mozilla-central/source/modules/libjar/nsJARChannel.cpp#268

but the the fd was ultimately opened only once, to do an mmap in the JAR cache, and then the fd was closed.

But the patch I just +r'd in bug 1048615 is caching the fd for later, unrelated callers to do things like the use case in bug 945152, and that might well not be kosher.  I've rescinded my review while we figure that out.

And yes, at a minimum we need to document the deviation from regular nsIFile's here.
Flags: needinfo?(jduell.mcbugs)
Note that RemoteOpenFileChild only implements a small subset of nsIFile, so it's already kind of a gross hack.  So the issue is that we get the code working and documented, not that we need to necessarily have the same semantics as regular nsIFile.
Depends on: 1055966
The fd from dup() here is used by RemoteOpenFileChild in conjunction with JARChannel, which makes 
RemoteOpenFileChild::OpenNSPRFileDesc() deviate from the regular semantic.  As far as I know, it's currently only been used in nsXMLHttpRequest.cpp for mmap purpose.

Filed bug 1055966 to specify the limiation.
Flags: needinfo?(swu)
You need to log in before you can comment on or make changes to this bug.