Bug 835575 - 'PRemoteOpenFile could be more IPC-efficient'. r=jduell.
authorBen Turner <bent.mozilla@gmail.com>
Thu, 07 Feb 2013 13:06:58 +0000
changeset 122137 331e2dcd93d7fc93d4eae8f7c2c00920886c1f00
parent 122136 f98f9cf410fe30d5bb382209071bea488af7d602
child 122138 fa1e5d532c9e37a569ebd49a29ef6dbcbe425fe0
push id24319
push userryanvm@gmail.com
push dateSat, 16 Feb 2013 23:49:46 +0000
treeherdermozilla-central@c4de6de47382 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjduell
bugs835575
milestone21.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 835575 - 'PRemoteOpenFile could be more IPC-efficient'. r=jduell.
netwerk/ipc/NeckoParent.cpp
netwerk/ipc/NeckoParent.h
netwerk/ipc/PNecko.ipdl
netwerk/ipc/PRemoteOpenFile.ipdl
netwerk/ipc/RemoteOpenFileChild.cpp
netwerk/ipc/RemoteOpenFileChild.h
netwerk/ipc/RemoteOpenFileParent.cpp
netwerk/ipc/RemoteOpenFileParent.h
--- a/netwerk/ipc/NeckoParent.cpp
+++ b/netwerk/ipc/NeckoParent.cpp
@@ -16,16 +16,17 @@
 #include "mozilla/dom/TabParent.h"
 #include "mozilla/dom/network/TCPSocketParent.h"
 #include "mozilla/ipc/URIUtils.h"
 #include "mozilla/LoadContext.h"
 #include "nsPrintfCString.h"
 #include "nsHTMLDNSPrefetch.h"
 #include "nsIAppsService.h"
 #include "nsEscape.h"
+#include "RemoteOpenFileParent.h"
 
 using mozilla::dom::TabParent;
 using mozilla::net::PTCPSocketParent;
 using mozilla::dom::TCPSocketParent;
 using IPC::SerializedLoadContext;
 
 namespace mozilla {
 namespace net {
@@ -399,16 +400,24 @@ NeckoParent::AllocPRemoteOpenFile(const 
     }
   }
 
   RemoteOpenFileParent* parent = new RemoteOpenFileParent(fileURL);
   return parent;
 }
 
 bool
+NeckoParent::RecvPRemoteOpenFileConstructor(PRemoteOpenFileParent* aActor,
+                                            const URIParams& aFileURI,
+                                            PBrowserParent* aBrowser)
+{
+  return static_cast<RemoteOpenFileParent*>(aActor)->OpenSendCloseDelete();
+}
+
+bool
 NeckoParent::DeallocPRemoteOpenFile(PRemoteOpenFileParent* actor)
 {
   delete actor;
   return true;
 }
 
 bool
 NeckoParent::RecvHTMLDNSPrefetch(const nsString& hostname,
@@ -423,9 +432,8 @@ NeckoParent::RecvCancelHTMLDNSPrefetch(c
                                  const uint16_t& flags,
                                  const nsresult& reason)
 {
   nsHTMLDNSPrefetch::CancelPrefetch(hostname, flags, reason);
   return true;
 }
 
 }} // mozilla::net
-
--- a/netwerk/ipc/NeckoParent.h
+++ b/netwerk/ipc/NeckoParent.h
@@ -64,20 +64,26 @@ protected:
   virtual PWebSocketParent* AllocPWebSocket(PBrowserParent* browser,
                                             const SerializedLoadContext& aSerialized);
   virtual bool DeallocPWebSocket(PWebSocketParent*);
   virtual PTCPSocketParent* AllocPTCPSocket(const nsString& aHost,
                                             const uint16_t& aPort,
                                             const bool& useSSL,
                                             const nsString& aBinaryType,
                                             PBrowserParent* aBrowser);
-  virtual PRemoteOpenFileParent* AllocPRemoteOpenFile(
-                                            const URIParams& fileuri,
-                                            PBrowserParent* browser);
-  virtual bool DeallocPRemoteOpenFile(PRemoteOpenFileParent* actor);
+
+  virtual PRemoteOpenFileParent* AllocPRemoteOpenFile(const URIParams& aFileURI,
+                                                      PBrowserParent* aBrowser)
+                                                      MOZ_OVERRIDE;
+  virtual bool RecvPRemoteOpenFileConstructor(PRemoteOpenFileParent* aActor,
+                                              const URIParams& aFileURI,
+                                              PBrowserParent* aBrowser)
+                                              MOZ_OVERRIDE;
+  virtual bool DeallocPRemoteOpenFile(PRemoteOpenFileParent* aActor)
+                                      MOZ_OVERRIDE;
 
   virtual bool RecvPTCPSocketConstructor(PTCPSocketParent*,
                                          const nsString& aHost,
                                          const uint16_t& aPort,
                                          const bool& useSSL,
                                          const nsString& aBinaryType,
                                          PBrowserParent* aBrowser);
   virtual bool DeallocPTCPSocket(PTCPSocketParent*);
--- a/netwerk/ipc/PNecko.ipdl
+++ b/netwerk/ipc/PNecko.ipdl
@@ -42,16 +42,18 @@ parent:
   PCookieService();
   PHttpChannel(nullable PBrowser browser,
                SerializedLoadContext loadContext);
   PWyciwygChannel();
   PFTPChannel(PBrowser browser, SerializedLoadContext loadContext);
   PWebSocket(PBrowser browser, SerializedLoadContext loadContext);
   PTCPSocket(nsString host, uint16_t port, bool useSSL, nsString binaryType,
              nullable PBrowser browser);
+
+  // Request that the parent open a file.
   PRemoteOpenFile(URIParams fileuri, nullable PBrowser browser);
 
   HTMLDNSPrefetch(nsString hostname, uint16_t flags);
   CancelHTMLDNSPrefetch(nsString hostname, uint16_t flags, nsresult reason);
 
 };
 
 
--- a/netwerk/ipc/PRemoteOpenFile.ipdl
+++ b/netwerk/ipc/PRemoteOpenFile.ipdl
@@ -13,25 +13,16 @@ namespace net {
 /**
  * Protocol to support RemoteOpenFile, an nsIFile that opens it's file handle on
  * the parent instead of the child (since child lacks permission to do so).
  */
 protocol PRemoteOpenFile
 {
   manager PNecko;
 
-parent:
-  // Tell parent to open file. URI to open was passed and vetted for security in
-  // IPDL constructor: see NeckoParent::AllocPRemoteOpenFile()
-  AsyncOpenFile();
-
-  __delete__();
-
 child:
   // Your file handle is ready, Sir...
-  FileOpened(FileDescriptor fd);
-  // Trying to send invalid fd crashes, so we need separate method for failure
-  FileDidNotOpen();
+  __delete__(FileDescriptor fd);
 };
 
 
 } // namespace net
 } // namespace mozilla
--- a/netwerk/ipc/RemoteOpenFileChild.cpp
+++ b/netwerk/ipc/RemoteOpenFileChild.cpp
@@ -181,21 +181,17 @@ RemoteOpenFileChild::AsyncRemoteFileOpen
     }
   }
 
   URIParams uri;
   SerializeURI(mURI, uri);
 
   gNeckoChild->SendPRemoteOpenFileConstructor(this, uri, mTabChild);
 
-  // Can't seem to reply from within IPDL Parent constructor, so send open as
-  // separate message
-  SendAsyncOpenFile();
-
-  // The chrome process now has a logical ref to us until we call Send__delete
+  // The chrome process now has a logical ref to us until it calls Send__delete.
   AddIPDLReference();
 
   mListener = aListener;
   mAsyncOpenCalled = true;
   return NS_OK;
 #endif
 }
 
@@ -211,23 +207,23 @@ RemoteOpenFileChild::OnCachedFileDescrip
     if (NS_FAILED(mFile->GetPath(path))) {
       MOZ_NOT_REACHED("Couldn't get path from file!");
     }
 
     MOZ_ASSERT(path == aPath, "Paths don't match!");
   }
 #endif
 
-  HandleFileDescriptorAndNotifyListener(aFD, /* aFromRecvFileOpened */ false);
+  HandleFileDescriptorAndNotifyListener(aFD, /* aFromRecvDelete */ false);
 }
 
 void
 RemoteOpenFileChild::HandleFileDescriptorAndNotifyListener(
                                                       const FileDescriptor& aFD,
-                                                      bool aFromRecvFileOpened)
+                                                      bool aFromRecvDelete)
 {
 #if defined(XP_WIN) || defined(MOZ_WIDGET_COCOA)
   MOZ_NOT_REACHED("OS X and Windows shouldn't be doing IPDL here");
 #else
   if (!mListener) {
     // We already notified our listener (either in response to a cached file
     // descriptor callback or through the normal messaging mechanism). Close the
     // file descriptor if it is valid.
@@ -238,19 +234,21 @@ RemoteOpenFileChild::HandleFileDescripto
     return;
   }
 
   MOZ_ASSERT(!mNSPRFileDesc);
 
   nsRefPtr<TabChild> tabChild;
   mTabChild.swap(tabChild);
 
-  // If there is a pending callback and we're being called from IPDL then we
-  // need to cancel it.
-  if (tabChild && aFromRecvFileOpened) {
+  // If RemoteOpenFile reply (Recv__delete__) for app's application.zip comes
+  // back sooner than the parent-pushed fd (TabChild::RecvCacheFileDescriptor())
+  // have TabChild cancel running callbacks, since we'll call them in
+  // NotifyListener.
+  if (tabChild && aFromRecvDelete) {
     nsString path;
     if (NS_FAILED(mFile->GetPath(path))) {
       MOZ_NOT_REACHED("Couldn't get path from file!");
     }
 
     tabChild->CancelCachedFileDescriptorCallback(path, this);
   }
 
@@ -280,43 +278,22 @@ RemoteOpenFileChild::NotifyListener(nsre
   }
 }
 
 //-----------------------------------------------------------------------------
 // RemoteOpenFileChild::PRemoteOpenFileChild
 //-----------------------------------------------------------------------------
 
 bool
-RemoteOpenFileChild::RecvFileOpened(const FileDescriptor& aFD)
+RemoteOpenFileChild::Recv__delete__(const FileDescriptor& aFD)
 {
 #if defined(XP_WIN) || defined(MOZ_WIDGET_COCOA)
   NS_NOTREACHED("OS X and Windows shouldn't be doing IPDL here");
 #else
-  HandleFileDescriptorAndNotifyListener(aFD, /* aFromRecvFileOpened */ true);
-
-  // This calls NeckoChild::DeallocPRemoteOpenFile(), which deletes |this| if
-  // IPDL holds the last reference.  Don't rely on |this| existing after here!
-  Send__delete__(this);
-#endif
-
-  return true;
-}
-
-bool
-RemoteOpenFileChild::RecvFileDidNotOpen()
-{
-#if defined(XP_WIN) || defined(MOZ_WIDGET_COCOA)
-  NS_NOTREACHED("OS X and Windows shouldn't be doing IPDL here");
-#else
-  HandleFileDescriptorAndNotifyListener(FileDescriptor(),
-                                        /* aFromRecvFileOpened */ true);
-
-  // This calls NeckoChild::DeallocPRemoteOpenFile(), which deletes |this| if
-  // IPDL holds the last reference.  Don't rely on |this| existing after here!
-  Send__delete__(this);
+  HandleFileDescriptorAndNotifyListener(aFD, /* aFromRecvDelete */ true);
 #endif
 
   return true;
 }
 
 //-----------------------------------------------------------------------------
 // RemoteOpenFileChild::nsIFile functions that we override logic for
 //-----------------------------------------------------------------------------
--- a/netwerk/ipc/RemoteOpenFileChild.h
+++ b/netwerk/ipc/RemoteOpenFileChild.h
@@ -2,16 +2,17 @@
 /* vim: set sw=2 ts=8 et tw=80 : */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef _RemoteOpenFileChild_h
 #define _RemoteOpenFileChild_h
 
+#include "mozilla/Attributes.h"
 #include "mozilla/dom/TabChild.h"
 #include "mozilla/net/PRemoteOpenFileChild.h"
 #include "nsICachedFileDescriptorListener.h"
 #include "nsILocalFile.h"
 #include "nsIRemoteOpenFileListener.h"
 
 namespace mozilla {
 
@@ -81,24 +82,23 @@ private:
   RemoteOpenFileChild(const RemoteOpenFileChild& other);
 
 protected:
   void AddIPDLReference()
   {
     AddRef();
   }
 
-  virtual bool RecvFileOpened(const FileDescriptor&);
-  virtual bool RecvFileDidNotOpen();
+  virtual bool Recv__delete__(const FileDescriptor&) MOZ_OVERRIDE;
 
   virtual void OnCachedFileDescriptor(const nsAString& aPath,
                                       const FileDescriptor& aFD) MOZ_OVERRIDE;
 
   void HandleFileDescriptorAndNotifyListener(const FileDescriptor&,
-                                             bool aFromRecvFileOpened);
+                                             bool aFromRecvDelete);
 
   void NotifyListener(nsresult aResult);
 
   // regular nsIFile object, that we forward most calls to.
   nsCOMPtr<nsIFile> mFile;
   nsCOMPtr<nsIURI> mURI;
   nsCOMPtr<nsIRemoteOpenFileListener> mListener;
   nsRefPtr<TabChild> mTabChild;
--- a/netwerk/ipc/RemoteOpenFileParent.cpp
+++ b/netwerk/ipc/RemoteOpenFileParent.cpp
@@ -12,60 +12,51 @@
 #if !defined(XP_WIN) && !defined(MOZ_WIDGET_COCOA)
 #include <fcntl.h>
 #include <unistd.h>
 #endif
 
 namespace mozilla {
 namespace net {
 
-RemoteOpenFileParent::RemoteOpenFileParent(nsIFileURL *aURI)
-  : mURI(aURI)
-#if !defined(XP_WIN) && !defined(MOZ_WIDGET_COCOA)
-  , mFd(-1)
-#endif
-{}
-
-RemoteOpenFileParent::~RemoteOpenFileParent()
-{
-#if !defined(XP_WIN) && !defined(MOZ_WIDGET_COCOA)
-  if (mFd != -1) {
-    // close file handle now that other process has it open, else we'll leak
-    // file handles in parent process
-    close(mFd);
-  }
-#endif
-}
-
 bool
-RemoteOpenFileParent::RecvAsyncOpenFile()
+RemoteOpenFileParent::OpenSendCloseDelete()
 {
 #if defined(XP_WIN) || defined(MOZ_WIDGET_COCOA)
-  NS_NOTREACHED("osX and Windows shouldn't be doing IPDL here");
+  MOZ_NOT_REACHED("OS X and Windows shouldn't be doing IPDL here");
 #else
 
   // TODO: make this async!
 
+  FileDescriptor fileDescriptor;
+
   nsAutoCString path;
   nsresult rv = mURI->GetFilePath(path);
+  NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "GetFilePath failed!");
+
   NS_UnescapeURL(path);
+
   if (NS_SUCCEEDED(rv)) {
     int fd = open(path.get(), O_RDONLY);
-    if (fd != -1) {
-      unused << SendFileOpened(FileDescriptor(fd));
-      // file handle needs to stay open until it's shared with child (and IPDL
-      // is async, so hasn't happened yet). Close in destructor.
-      mFd = fd;
-      return true;
+    if (fd == -1) {
+      printf_stderr("RemoteOpenFileParent: file '%s' was not found!\n",
+                    path.get());
+    } else {
+      fileDescriptor = FileDescriptor(fd);
     }
   }
 
-  // Note: sending an invalid file descriptor currently kills the child process:
-  // but that's ok for our use case (failing to open application.jar).
-  printf_stderr("RemoteOpenFileParent: file '%s' was not found!\n", path.get());
-  unused << SendFileDidNotOpen();
+  // Sending a potentially invalid file descriptor is just fine.
+  unused << Send__delete__(this, fileDescriptor);
+
+  if (fileDescriptor.IsValid()) {
+    // close file now that other process has it open, else we'll leak fds in the
+    // parent process.
+    close(fileDescriptor.PlatformHandle());
+  }
+
 #endif // OS_TYPE
 
   return true;
 }
 
 } // namespace net
 } // namespace mozilla
--- a/netwerk/ipc/RemoteOpenFileParent.h
+++ b/netwerk/ipc/RemoteOpenFileParent.h
@@ -13,26 +13,22 @@
 #include "nsIFileURL.h"
 
 namespace mozilla {
 namespace net {
 
 class RemoteOpenFileParent : public PRemoteOpenFileParent
 {
 public:
-  RemoteOpenFileParent(nsIFileURL* aURI);
+  RemoteOpenFileParent(nsIFileURL* aURI)
+  : mURI(aURI)
+  {}
 
- ~RemoteOpenFileParent();
-
-  virtual bool RecvAsyncOpenFile();
+  bool OpenSendCloseDelete();
 
 private:
   nsCOMPtr<nsIFileURL> mURI;
-
-#if !defined(XP_WIN) && !defined(MOZ_WIDGET_COCOA)
-  int mFd;
-#endif
 };
 
 } // namespace net
 } // namespace mozilla
 
 #endif // mozilla_net_RemoteOpenFileParent_h