Delete the temporary file when generating MHTML with the extension API.

We now delete the temporary file created when generating MHTML from the
extension API.

BUG=97489
TEST=The MHTML extension API should still work.


Review URL: http://codereview.chromium.org/8530003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@111180 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/extensions/extension_function.cc

@@ -26,8 +26,10 @@ void ExtensionFunctionDeleteTraits::Destruct(const ExtensionFunction* x) {
 }
 
 UIThreadExtensionFunction::RenderViewHostTracker::RenderViewHostTracker(
-    UIThreadExtensionFunction* function)
-    : function_(function) {
+    UIThreadExtensionFunction* function,
+    RenderViewHost* render_view_host)
+    : RenderViewHostObserver(render_view_host),
+      function_(function) {
   registrar_.Add(this,
                  content::NOTIFICATION_RENDER_VIEW_HOST_DELETED,
                  content::Source<RenderViewHost>(function->render_view_host()));
@@ -43,6 +45,18 @@ void UIThreadExtensionFunction::RenderViewHostTracker::Observe(
   function_->SetRenderViewHost(NULL);
 }
 
+void UIThreadExtensionFunction::RenderViewHostTracker::RenderViewHostDestroyed(
+    RenderViewHost* render_view_host) {
+  // Overidding the default behavior of RenderViewHostObserver which is to
+  // delete this. In our case, we'll be deleted when the
+  // UIThreadExtensionFunction that contains us goes away.
+}
+
+bool UIThreadExtensionFunction::RenderViewHostTracker::OnMessageReceived(
+    const IPC::Message& message) {
+  return function_->OnMessageReceivedFromRenderView(message);
+}
+
 ExtensionFunction::ExtensionFunction()
     : request_id_(-1),
       profile_id_(NULL),
@@ -139,6 +153,11 @@ UIThreadExtensionFunction::AsUIThreadExtensionFunction() {
   return this;
 }
 
+bool UIThreadExtensionFunction::OnMessageReceivedFromRenderView(
+    const IPC::Message& message) {
+  return false;
+}
+
 void UIThreadExtensionFunction::Destruct() const {
   BrowserThread::DeleteOnUIThread::Destruct(this);
 }
@@ -146,7 +165,8 @@ void UIThreadExtensionFunction::Destruct() const {
 void UIThreadExtensionFunction::SetRenderViewHost(
     RenderViewHost* render_view_host) {
   render_view_host_ = render_view_host;
-  tracker_.reset(render_view_host ? new RenderViewHostTracker(this) : NULL);
+  tracker_.reset(render_view_host ?
+      new RenderViewHostTracker(this, render_view_host) : NULL);
 }
 
 Browser* UIThreadExtensionFunction::GetCurrentBrowser() {

chrome/browser/extensions/extension_function.h

@@ -16,6 +16,7 @@
 #include "base/process.h"
 #include "chrome/browser/extensions/extension_info_map.h"
 #include "chrome/common/extensions/extension.h"
+#include "content/browser/renderer_host/render_view_host_observer.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
@@ -222,6 +223,11 @@ class UIThreadExtensionFunction : public ExtensionFunction {
   void set_test_delegate(DelegateForTests* delegate) {
     delegate_ = delegate;
   }
+
+  // Called when a message was received.
+  // Should return true if it processed the message.
+  virtual bool OnMessageReceivedFromRenderView(const IPC::Message& message);
+
   // Set the profile which contains the extension that has originated this
   // function call.
   void set_profile(Profile* profile) { profile_ = profile; }
@@ -276,20 +282,30 @@ class UIThreadExtensionFunction : public ExtensionFunction {
 
  private:
   // Helper class to track the lifetime of ExtensionFunction's RenderViewHost
-  // pointer and NULL it out when it dies. We use this separate class (instead
-  // of implementing NotificationObserver on ExtensionFunction) because it is
-  // common for subclasses of ExtensionFunction to be NotificationObservers, and
-  // it would be an easy error to forget to call the base class's Observe()
-  // method.
-  class RenderViewHostTracker : public content::NotificationObserver {
+  // pointer and NULL it out when it dies. It also allows us to filter IPC
+  // messages comming from the RenderViewHost. We use this separate class
+  // (instead of implementing NotificationObserver on ExtensionFunction) because
+  // it is/ common for subclasses of ExtensionFunction to be
+  // NotificationObservers, and it would be an easy error to forget to call the
+  // base class's Observe() method.
+  class RenderViewHostTracker : public content::NotificationObserver,
+                                public RenderViewHostObserver {
    public:
-    explicit RenderViewHostTracker(UIThreadExtensionFunction* function);
+    RenderViewHostTracker(UIThreadExtensionFunction* function,
+                          RenderViewHost* render_view_host);
    private:
     virtual void Observe(int type,
                          const content::NotificationSource& source,
                          const content::NotificationDetails& details) OVERRIDE;
+
+    virtual void RenderViewHostDestroyed(
+        RenderViewHost* render_view_host) OVERRIDE;
+    virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
+
     UIThreadExtensionFunction* function_;
     content::NotificationRegistrar registrar_;
+
+    DISALLOW_COPY_AND_ASSIGN(RenderViewHostTracker);
   };
 
   virtual void Destruct() const OVERRIDE;

chrome/browser/extensions/extension_save_page_api.cc

@@ -8,6 +8,7 @@
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/extensions/extension_tab_util.h"
 #include "chrome/browser/ui/tab_contents/tab_contents_wrapper.h"
+#include "chrome/common/extensions/extension_messages.h"
 #include "content/browser/child_process_security_policy.h"
 #include "content/browser/renderer_host/render_view_host.h"
 #include "content/browser/tab_contents/tab_contents.h"
@@ -26,12 +27,18 @@ const char* const kSizeRetrievalError =
 const char* const kTemporaryFileError = "Failed to create a temporary file.";
 const char* const kTabClosedError = "Cannot find the tab for thie request.";
 
+static SavePageAsMHTMLFunction::TestDelegate* test_delegate_ = NULL;
+
 SavePageAsMHTMLFunction::SavePageAsMHTMLFunction() : tab_id_(0) {
 }
 
 SavePageAsMHTMLFunction::~SavePageAsMHTMLFunction() {
 }
 
+void SavePageAsMHTMLFunction::SetTestDelegate(TestDelegate* delegate) {
+  test_delegate_ = delegate;
+}
+
 bool SavePageAsMHTMLFunction::RunImpl() {
   EXTENSION_FUNCTION_VALIDATE(args_->GetInteger(0, &tab_id_));
 
@@ -42,11 +49,30 @@ bool SavePageAsMHTMLFunction::RunImpl() {
   return true;
 }
 
+bool SavePageAsMHTMLFunction::OnMessageReceivedFromRenderView(
+    const IPC::Message& message) {
+  if (message.type() != ExtensionHostMsg_ResponseAck::ID)
+    return false;
+
+  int message_request_id;
+  void* iter = NULL;
+  if (!message.ReadInt(&iter, &message_request_id)) {
+    NOTREACHED() << "malformed extension message";
+    return true;
+  }
+
+  if (message_request_id != request_id())
+    return false;
+
+  // The extension process has processed the response and has created a
+  // reference to the blob, it is safe for us to go away.
+  Release();  // Balanced in Run()
+
+  return true;
+}
+
 void SavePageAsMHTMLFunction::CreateTemporaryFile() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  // TODO(jcivelli): http://crbug.com/97489 we don't clean-up the temporary file
-  //                 at this point.  It must be done before we can take that API
-  //                 out of experimental.
   bool success = file_util::CreateTemporaryFile(&mhtml_path_);
   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
       NewRunnableMethod(this, &SavePageAsMHTMLFunction::TemporaryFileCreated,
@@ -59,16 +85,20 @@ void SavePageAsMHTMLFunction::TemporaryFileCreated(bool success) {
     return;
   }
 
-  Browser* browser = NULL;
-  TabContentsWrapper* tab_contents_wrapper = NULL;
+  if (test_delegate_)
+    test_delegate_->OnTemporaryFileCreated(mhtml_path_);
 
-  if (!ExtensionTabUtil::GetTabById(tab_id_, profile(), include_incognito(),
-      &browser, NULL, &tab_contents_wrapper, NULL)) {
+  // Sets a DeletableFileReference so the temporary file gets deleted once it is
+  // no longer used.
+  mhtml_file_ = webkit_blob::DeletableFileReference::GetOrCreate(mhtml_path_,
+      BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE));
+
+  TabContents* tab_contents = GetTabContents();
+  if (!tab_contents) {
     ReturnFailure(kTabClosedError);
     return;
   }
 
-  TabContents* tab_contents = tab_contents_wrapper->tab_contents();
   registrar_.Add(
       this, content::NOTIFICATION_MHTML_GENERATED,
       content::Source<RenderViewHost>(tab_contents->render_view_host()));
@@ -124,6 +154,12 @@ void SavePageAsMHTMLFunction::ReturnFailure(const std::string& error) {
 void SavePageAsMHTMLFunction::ReturnSuccess(int64 file_size) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  TabContents* tab_contents = GetTabContents();
+  if (!tab_contents || !render_view_host()) {
+    ReturnFailure(kTabClosedError);
+    return;
+  }
+
   int child_id = render_view_host()->process()->GetID();
   ChildProcessSecurityPolicy::GetInstance()->GrantReadFile(
       child_id, mhtml_path_);
@@ -135,5 +171,18 @@ void SavePageAsMHTMLFunction::ReturnSuccess(int64 file_size) {
 
   SendResponse(true);
 
-  Release();  // Balanced in Run()
+  // Note that we'll wait for a response ack message received in
+  // OnMessageReceivedFromRenderView before we call Release() (to prevent the
+  // blob file from being deleted).
+}
+
+TabContents* SavePageAsMHTMLFunction::GetTabContents() {
+  Browser* browser = NULL;
+  TabContentsWrapper* tab_contents_wrapper = NULL;
+
+  if (!ExtensionTabUtil::GetTabById(tab_id_, profile(), include_incognito(),
+      &browser, NULL, &tab_contents_wrapper, NULL)) {
+    return NULL;
+  }
+  return tab_contents_wrapper->tab_contents();
 }

chrome/browser/extensions/extension_save_page_api.h

@@ -7,23 +7,35 @@
 
 #include <string>
 
+#include "base/memory/ref_counted.h"
 #include "chrome/browser/extensions/extension_function.h"
 #include "content/browser/tab_contents/tab_contents_observer.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
+#include "webkit/blob/deletable_file_reference.h"
 
 class SavePageAsMHTMLFunction : public AsyncExtensionFunction,
                                 public content::NotificationObserver {
  public:
   SavePageAsMHTMLFunction();
 
+  // Test specific delegate used to test that the temporary file gets deleted.
+  class TestDelegate {
+   public:
+    // Called on the UI thread when the temporary file that contains the
+    // generated data has been created.
+    virtual void OnTemporaryFileCreated(const FilePath& temp_file) = 0;
+  };
+  static void SetTestDelegate(TestDelegate* delegate);
+
  private:
   virtual ~SavePageAsMHTMLFunction();
-
   virtual bool RunImpl() OVERRIDE;
   virtual void Observe(int type,
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE;
+  virtual bool OnMessageReceivedFromRenderView(
+      const IPC::Message& message) OVERRIDE;
 
   // Called on the file thread.
   void CreateTemporaryFile();
@@ -33,12 +45,19 @@ class SavePageAsMHTMLFunction : public AsyncExtensionFunction,
   void ReturnFailure(const std::string& error);
   void ReturnSuccess(int64 file_size);
 
+  // Returns the TabContents we are associated with, NULL if it's been closed.
+  TabContents* GetTabContents();
+
   int tab_id_;
 
   // The path to the temporary file containing the MHTML data.
   FilePath mhtml_path_;
 
+  // The file containing the MHTML.
+  scoped_refptr<webkit_blob::DeletableFileReference> mhtml_file_;
+
   content::NotificationRegistrar registrar_;
+
   DECLARE_EXTENSION_FUNCTION_NAME("experimental.savePage.saveAsMHTML")
 };
 

chrome/browser/extensions/extension_save_page_apitest.cc

@@ -5,7 +5,9 @@
 #include "base/base_switches.h"
 #include "base/command_line.h"
 #include "chrome/browser/extensions/extension_apitest.h"
+#include "chrome/browser/extensions/extension_save_page_api.h"
 #include "chrome/common/chrome_switches.h"
+#include "chrome/test/base/ui_test_utils.h"
 #include "net/base/mock_host_resolver.h"
 
 class ExtensionSavePageApiTest : public ExtensionApiTest {
@@ -14,6 +16,7 @@ class ExtensionSavePageApiTest : public ExtensionApiTest {
   virtual void SetUpCommandLine(CommandLine* command_line) {
     ExtensionApiTest::SetUpCommandLine(command_line);
     command_line->AppendSwitch(switches::kEnableExperimentalExtensionApis);
+    command_line->AppendSwitchASCII(switches::kJavaScriptFlags, "--expose-gc");
   }
 
   virtual void SetUpInProcessBrowserTestFixture() {
@@ -32,6 +35,29 @@ class ExtensionSavePageApiTest : public ExtensionApiTest {
 #define MAYBE_SavePageAsMHTML SavePageAsMHTML
 #endif  // defined(OS_LINUX)
 
+class SavePageAsMHTMLDelegate : public SavePageAsMHTMLFunction::TestDelegate {
+ public:
+  SavePageAsMHTMLDelegate() {
+    SavePageAsMHTMLFunction::SetTestDelegate(this);
+  }
+
+  virtual ~SavePageAsMHTMLDelegate() {
+    SavePageAsMHTMLFunction::SetTestDelegate(NULL);
+  }
+
+  virtual void OnTemporaryFileCreated(const FilePath& temp_file) OVERRIDE {
+    temp_file_ = temp_file;
+  }
+
+  FilePath temp_file_;
+};
+
 IN_PROC_BROWSER_TEST_F(ExtensionSavePageApiTest, MAYBE_SavePageAsMHTML) {
+  SavePageAsMHTMLDelegate delegate;
   ASSERT_TRUE(RunExtensionTest("save_page")) << message_;
+  ASSERT_FALSE(delegate.temp_file_.empty());
+  // Flush the file message loop to make sure the delete happens.
+  ui_test_utils::RunAllPendingInMessageLoop(content::BrowserThread::FILE);
+  ASSERT_FALSE(file_util::PathExists(delegate.temp_file_));
+
 }

chrome/common/extensions/extension_messages.h

@@ -356,6 +356,11 @@ IPC_MESSAGE_ROUTED4(ExtensionHostMsg_GetAppNotifyChannel,
                     int32 /* return_route_id */,
                     int32 /* callback_id */)
 
+// Optional Ack message sent to the browser to notify that the response to a
+// function has been processed.
+IPC_MESSAGE_ROUTED1(ExtensionHostMsg_ResponseAck,
+                    int /* request_id */)
+
 // Response to the renderer for the above message.
 IPC_MESSAGE_ROUTED3(ExtensionMsg_GetAppNotifyChannelResponse,
                     std::string /* channel_id */,

chrome/renderer/extensions/schema_generated_bindings.cc

@@ -213,6 +213,9 @@ class ExtensionImpl : public ChromeV8Extension {
       return v8::FunctionTemplate::New(DecodeJPEG, v8::External::New(this));
     } else if (name->Equals(v8::String::New("CreateBlob"))) {
       return v8::FunctionTemplate::New(CreateBlob, v8::External::New(this));
+    } else if (name->Equals(v8::String::New("SendResponseAck"))) {
+      return v8::FunctionTemplate::New(SendResponseAck,
+                                       v8::External::New(this));
     }
 
     return ChromeV8Extension::GetNativeFunction(name);
@@ -397,6 +400,18 @@ class ExtensionImpl : public ChromeV8Extension {
     return blob.toV8Value();
   }
 
+  static v8::Handle<v8::Value> SendResponseAck(const v8::Arguments& args) {
+    CHECK(args.Length() == 1);
+    CHECK(args[0]->IsInt32());
+
+    content::RenderView* render_view = GetCurrentRenderView();
+    if (render_view) {
+      render_view->Send(new ExtensionHostMsg_ResponseAck(
+          render_view->GetRoutingId(), args[0]->Int32Value()));
+    }
+    return v8::Undefined();
+  }
+
   // Creates a new messaging channel to the tab with the given ID.
   static v8::Handle<v8::Value> OpenChannelToTab(const v8::Arguments& args) {
     // Get the current RenderView so that we can send a routed IPC message from

chrome/renderer/resources/extensions/schema_generated_bindings.js

@@ -24,6 +24,7 @@ var chrome = chrome || {};
   native function GetLocalFileSystem(name, path);
   native function DecodeJPEG(jpegImage);
   native function CreateBlob(filePath);
+  native function SendResponseAck(requestId);
 
   var chromeHidden = GetChromeHidden();
 
@@ -178,6 +179,7 @@ var chrome = chrome || {};
     var nativeFunction = opt_args.nativeFunction || StartRequest;
 
     var requestId = GetNextRequestId();
+    request.id = requestId;
     requests[requestId] = request;
     var hasCallback =
         (request.callback || opt_args.customCallback) ? true : false;
@@ -795,8 +797,11 @@ var chrome = chrome || {};
 
         if (request.callback)
           request.callback(CreateBlob(path, size));
-
         request.callback = null;
+
+        // Notify the browser. Now that the blob is referenced from JavaScript,
+        // the browser can drop its reference to it.
+        SendResponseAck(request.id);
       };
 
     apiFunctions["fileBrowserPrivate.requestLocalFileSystem"].customCallback =

chrome/test/base/ui_test_utils.cc

@@ -228,22 +228,36 @@ bool ExecuteJavaScriptHelper(RenderViewHost* render_view_host,
   return true;
 }
 
+void RunAllPendingMessageAndSendQuit(content::BrowserThread::ID thread_id) {
+  MessageLoop::current()->PostTask(FROM_HERE, new MessageLoop::QuitTask());
+  RunMessageLoop();
+  content::BrowserThread::PostTask(thread_id, FROM_HERE,
+                                   new MessageLoop::QuitTask());
+}
+
 }  // namespace
 
 void RunMessageLoop() {
-  MessageLoopForUI* loop = MessageLoopForUI::current();
+  MessageLoop* loop = MessageLoop::current();
+  MessageLoopForUI* ui_loop =
+      content::BrowserThread::CurrentlyOn(content::BrowserThread::UI) ?
+          MessageLoopForUI::current() : NULL;
   bool did_allow_task_nesting = loop->NestableTasksAllowed();
   loop->SetNestableTasksAllowed(true);
+  if (ui_loop) {
 #if defined(USE_AURA)
     aura::Desktop::GetInstance()->Run();
 #elif defined(TOOLKIT_VIEWS)
     views::AcceleratorHandler handler;
-  loop->RunWithDispatcher(&handler);
+    ui_loop->RunWithDispatcher(&handler);
 #elif defined(OS_POSIX) && !defined(OS_MACOSX)
-  loop->RunWithDispatcher(NULL);
+    ui_loop->RunWithDispatcher(NULL);
 #else
-  loop->Run();
+    ui_loop->Run();
 #endif
+  } else {
+    loop->Run();
+  }
   loop->SetNestableTasksAllowed(did_allow_task_nesting);
 }
 
@@ -252,6 +266,22 @@ void RunAllPendingInMessageLoop() {
   ui_test_utils::RunMessageLoop();
 }
 
+void RunAllPendingInMessageLoop(content::BrowserThread::ID thread_id) {
+  if (content::BrowserThread::CurrentlyOn(thread_id)) {
+    RunAllPendingInMessageLoop();
+    return;
+  }
+  content::BrowserThread::ID current_thread_id;
+  if (!content::BrowserThread::GetCurrentThreadIdentifier(&current_thread_id)) {
+    NOTREACHED();
+    return;
+  }
+  content::BrowserThread::PostTask(thread_id, FROM_HERE,
+      base::Bind(&RunAllPendingMessageAndSendQuit, current_thread_id));
+
+  ui_test_utils::RunMessageLoop();
+}
+
 bool GetCurrentTabTitle(const Browser* browser, string16* title) {
   TabContents* tab_contents = browser->GetSelectedTabContents();
   if (!tab_contents)

chrome/test/base/ui_test_utils.h

@@ -18,6 +18,7 @@
 #include "base/string16.h"
 #include "chrome/browser/ui/view_ids.h"
 #include "chrome/test/automation/dom_element_proxy.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/notification_details.h"
@@ -83,6 +84,10 @@ void RunMessageLoop();
 // all pending tasks.
 void RunAllPendingInMessageLoop();
 
+// Blocks the current thread until all the pending messages in the loop of the
+// thread |thread_id| have been processed.
+void RunAllPendingInMessageLoop(content::BrowserThread::ID thread_id);
+
 // Puts the current tab title in |title|. Returns true on success.
 bool GetCurrentTabTitle(const Browser* browser, string16* title);
 

chrome/test/data/extensions/api_test/save_page/test.js

@@ -40,7 +40,9 @@ chrome.test.getConfig(function(config) {
               var text = e.target.result;
               assertTrue(text.indexOf(testUrl) != -1);
               assertTrue(text.indexOf("logo.png") != -1);
-              chrome.test.notifyPass();
+              // Run the GC so the blob is deleted.
+              window.setTimeout(function() { window.gc(); });
+              window.setTimeout(function() { chrome.test.notifyPass(); }, 0);
             };
             reader.readAsText(data);
           });

content/renderer/mhtml_generator.cc

@@ -34,6 +34,7 @@ void MHTMLGenerator::OnSavePageAsMHTML(
       IPC::PlatformFileForTransitToPlatformFile(file_for_transit);
   file_ = file;
   int64 size = GenerateMHTML();
+  base::ClosePlatformFile(file);
   NotifyBrowser(job_id, size);
 }