Bug 835575, 'PRemoteOpenFile could be more IPC-efficient'. r=jduell.
☠☠ backed out by 6d5b6ce4a2ae ☠ ☠
authorben turner <bent.mozilla>
Fri, 01 Feb 2013 10:20:00 +0000
changeset 121154 84db7473edfb4a47f861b09594a576d9c4ea88b2
parent 121153 1a2e34c9ebc783eb780ea9de178fd1f91aaffdb6
child 121155 6d5b6ce4a2ae90edd2d1206ef602dcebd84595bc
push id24276
push userryanvm@gmail.com
push dateThu, 07 Feb 2013 21:40:43 +0000
treeherdermozilla-central@eb8f60c782da [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_NOTREACHED("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