Use HTTP response headers for PNaCl caching instead of bitcode hash.

ppapi/native_client/src/trusted/plugin/file_downloader.cc

Index: ppapi/native_client/src/trusted/plugin/file_downloader.cc
diff --git a/ppapi/native_client/src/trusted/plugin/file_downloader.cc b/ppapi/native_client/src/trusted/plugin/file_downloader.cc
index 170812421cc3e2b3b1ddeebe8295aa4415917d6b..f542bd9f0588325050510978a257c9486735831b 100644
--- a/ppapi/native_client/src/trusted/plugin/file_downloader.cc
+++ b/ppapi/native_client/src/trusted/plugin/file_downloader.cc
@@ -55,6 +55,7 @@ bool FileDownloader::OpenStream(
     const nacl::string& url,
     const pp::CompletionCallback& callback,
     StreamCallbackSource* stream_callback_source) {
+  open_and_stream_ = false;

[dmichael (off chromium), 2013/06/14 16:21:43]

It looks like you never initialized open_and_stream_

[jvoung (off chromium), 2013/06/14 20:38:03]

It is initialized to true in the constructor (.h file), I believe.

   data_stream_callback_source_ = stream_callback_source;
   return Open(url, DOWNLOAD_STREAM, callback, true, NULL);
 }
@@ -223,26 +224,29 @@ bool FileDownloader::InitialResponseIsValid(int32_t pp_error) {
   }
 
   // Process the response, validating the headers to confirm successful loading.
-  pp::URLResponseInfo url_response(url_loader_.GetResponseInfo());
-  if (url_response.is_null()) {
+  url_response_ = url_loader_.GetResponseInfo();
+  if (url_response_.is_null()) {
     PLUGIN_PRINTF((
-        "FileDownloader::InitialResponseIsValid (url_response=NULL)\n"));
+        "FileDownloader::InitialResponseIsValid (url_response_=NULL)\n"));

[dmichael (off chromium), 2013/06/14 16:21:43]

nit: maybe == instead of = to be clearer

[jvoung (off chromium), 2013/06/14 20:38:03]

I think most of the PLUGIN_PRINTFs use = instead of == (e.g., "status_code=..."
below).  I could change them if you prefer, or stick with the original.  Maybe
this print doesn't need to have the trailing "_" either.

     file_open_notify_callback_.Run(PP_ERROR_FAILED);
     return false;
   }
-  // Note that URLs in the chrome-extension scheme produce different error
-  // codes than other schemes.  This is because chrome-extension URLs are
-  // really a special kind of file scheme, and therefore do not produce HTTP
-  // status codes.
-  pp::Var full_url = url_response.GetURL();
+
+  pp::Var full_url = url_response_.GetURL();
   if (!full_url.is_string()) {
     PLUGIN_PRINTF((
         "FileDownloader::InitialResponseIsValid (url is not a string)\n"));
     file_open_notify_callback_.Run(PP_ERROR_FAILED);
     return false;
   }
+  url_ = full_url.AsString();
+
+  // Note that URLs in the data-URI scheme produce different error
+  // codes than other schemes.  This is because data-URI are really a
+  // special kind of file scheme, and therefore do not produce HTTP
+  // status codes.
   bool status_ok = false;
-  status_code_ = url_response.GetStatusCode();
+  status_code_ = url_response_.GetStatusCode();
   switch (url_scheme_) {
     case SCHEME_CHROME_EXTENSION:
       PLUGIN_PRINTF(("FileDownloader::InitialResponseIsValid (chrome-extension "
@@ -274,19 +278,62 @@ void FileDownloader::URLLoadStartNotify(int32_t pp_error) {
                  NACL_PRId32")\n", pp_error));
 
   if (!InitialResponseIsValid(pp_error))
+    // InitialResponseIsValid() calls file_open_notify_callback_ on errors.
+    return;

[dmichael (off chromium), 2013/06/14 16:21:43]

style nit: use braces since it's >1 line now

[jvoung (off chromium), 2013/06/14 20:38:03]

Done.

+
+  if (open_and_stream_)
+    return FinishStreaming(file_open_notify_callback_);
+
+  file_open_notify_callback_.Run(pp_error);
+}
+
+void FileDownloader::URLBufferStartNotify(int32_t pp_error) {
+  PLUGIN_PRINTF(("FileDownloader::URLBufferStartNotify (pp_error=%"
+                 NACL_PRId32")\n", pp_error));
+
+  if (!InitialResponseIsValid(pp_error))
+    // InitialResponseIsValid() calls file_open_notify_callback_ on errors.
     return;

[dmichael (off chromium), 2013/06/14 16:21:43]

style nit: curly braces

[jvoung (off chromium), 2013/06/14 20:38:03]

Done.

+
+  if (open_and_stream_)
+    return FinishStreaming(file_open_notify_callback_);
+
+  file_open_notify_callback_.Run(pp_error);

[dmichael (off chromium), 2013/06/14 16:21:43]

It would be nice to clear the callback after you run it to make sure you don't
run it twice

[jvoung (off chromium), 2013/06/14 20:38:03]

Like set them to PP_BlockUntilComplete()?


I think these callbacks are created by the
ppapi/utility/completion_callback_factory, which has some partial "resetting". 
It only frees the user data, but that just means calling twice could give you a
memory bug.

Perhaps the factory could automatically set the callback, or would you prefer it
to be more explicit?

[dmichael (off chromium), 2013/06/14 21:09:03]

Right, that's what I meant:
file_open_notify_callback_ = pp::BlockUntilComplete();
You could have a RunAndClear method, maybe:
RunAndClear(pp_error, &file_open_notify_callback_);
to make it a little cleaner.

Admittedly, if you're code's all good, it won't have any effect. But it makes
your state a little clearer and might catch coding errors.
Right, a use-after-free, which we should try to avoid. It could fail in uglier,
harder-to-find ways than if the callback was NULL.
The factory doesn't have a way to set your CompletionCallback member. We could
arguably have a static RunAndClear in the pp namespace, I guess, if this comes
up often enough.

[jvoung (off chromium), 2013/06/17 18:20:27]

Okay, there was a RunAndClearCompletionCallback function in the C API, so I
added a hook to that in the cpp API and used that here.



On 2013/06/14 21:09:03, dmichael wrote:

+}
+
+void FileDownloader::FinishStreaming(
+    const pp::CompletionCallback& callback) {
+  stream_finish_callback_ = callback;
+
   // Finish streaming the body providing an optional callback.
-  pp::CompletionCallback onload_callback =
-      callback_factory_.NewOptionalCallback(
-          &FileDownloader::URLLoadFinishNotify);
-  pp_error = url_loader_.FinishStreamingToFile(onload_callback);
-  bool async_notify_ok = (pp_error == PP_OK_COMPLETIONPENDING);
-  PLUGIN_PRINTF(("FileDownloader::URLLoadStartNotify (async_notify_ok=%d)\n",
-                 async_notify_ok));
-  if (!async_notify_ok) {
-    // Call manually to free allocated memory and report errors.  This calls
-    // |file_open_notify_callback_| with |pp_error| as the parameter.
-    onload_callback.Run(pp_error);
+  if (streaming_to_file()) {
+    pp::CompletionCallback onload_callback =
+        callback_factory_.NewOptionalCallback(
+            &FileDownloader::URLLoadFinishNotify);
+    int32_t pp_error = url_loader_.FinishStreamingToFile(onload_callback);
+    bool async_notify_ok = (pp_error == PP_OK_COMPLETIONPENDING);
+    PLUGIN_PRINTF(("FileDownloader::FinishStreaming (async_notify_ok=%d)\n",
+                   async_notify_ok));
+    if (!async_notify_ok) {
+      // Call manually to free allocated memory and report errors.  This calls
+      // |stream_finish_callback_| with |pp_error| as the parameter.
+      onload_callback.Run(pp_error);
+    }
+  } else {
+    pp::CompletionCallback onread_callback =
+        callback_factory_.NewOptionalCallback(
+            &FileDownloader::URLReadBodyNotify);
+    int32_t temp_size = static_cast<int32_t>(temp_buffer_.size());
+    int32_t pp_error = url_loader_.ReadResponseBody(&temp_buffer_[0],
+                                                    temp_size,
+                                                    onread_callback);
+    bool async_notify_ok = (pp_error == PP_OK_COMPLETIONPENDING);
+    PLUGIN_PRINTF((
+        "FileDownloader::FinishStreaming (async_notify_ok=%d)\n",
+        async_notify_ok));
+    if (!async_notify_ok) {
+      onread_callback.Run(pp_error);
+    }
   }
 }
 
@@ -294,79 +341,59 @@ void FileDownloader::URLLoadFinishNotify(int32_t pp_error) {
   PLUGIN_PRINTF(("FileDownloader::URLLoadFinishNotify (pp_error=%"
                  NACL_PRId32")\n", pp_error));
   if (pp_error != PP_OK) {  // Streaming failed.
-    file_open_notify_callback_.Run(pp_error);
+    stream_finish_callback_.Run(pp_error);
     return;
   }
 
-  pp::URLResponseInfo url_response(url_loader_.GetResponseInfo());
-  // Validated on load.
-  CHECK(url_response.GetStatusCode() == NACL_HTTP_STATUS_OK ||
-        url_response.GetStatusCode() == kExtensionUrlRequestStatusOk);
+  // Validate response again on load (though it should be the same
+  // as it was during InitialResponseIsValid?).
+  url_response_ = url_loader_.GetResponseInfo();
+  CHECK(url_response_.GetStatusCode() == NACL_HTTP_STATUS_OK ||
+        url_response_.GetStatusCode() == kExtensionUrlRequestStatusOk);
 
   // Record the full url from the response.
-  pp::Var full_url = url_response.GetURL();
+  pp::Var full_url = url_response_.GetURL();
   PLUGIN_PRINTF(("FileDownloader::URLLoadFinishNotify (full_url=%s)\n",
                  full_url.DebugString().c_str()));
   if (!full_url.is_string()) {
-    file_open_notify_callback_.Run(PP_ERROR_FAILED);
+    stream_finish_callback_.Run(PP_ERROR_FAILED);
     return;
   }
   url_ = full_url.AsString();
 
   // The file is now fully downloaded.
-  pp::FileRef file(url_response.GetBodyAsFileRef());
+  pp::FileRef file(url_response_.GetBodyAsFileRef());
   if (file.is_null()) {
     PLUGIN_PRINTF(("FileDownloader::URLLoadFinishNotify (file=NULL)\n"));
-    file_open_notify_callback_.Run(PP_ERROR_FAILED);
+    stream_finish_callback_.Run(PP_ERROR_FAILED);
     return;
   }
 
   // Open the file providing an optional callback.
   pp::CompletionCallback onopen_callback =
-      callback_factory_.NewOptionalCallback(&FileDownloader::FileOpenNotify);
+      callback_factory_.NewOptionalCallback(
+          &FileDownloader::StreamFinishNotify);
   pp_error = file_reader_.Open(file, PP_FILEOPENFLAG_READ, onopen_callback);
   bool async_notify_ok = (pp_error == PP_OK_COMPLETIONPENDING);
   PLUGIN_PRINTF(("FileDownloader::URLLoadFinishNotify (async_notify_ok=%d)\n",
                  async_notify_ok));
   if (!async_notify_ok) {
     // Call manually to free allocated memory and report errors.  This calls
-    // |file_open_notify_callback_| with |pp_error| as the parameter.
+    // |stream_finish_callback_| with |pp_error| as the parameter.
     onopen_callback.Run(pp_error);
   }
 }
 
-void FileDownloader::URLBufferStartNotify(int32_t pp_error) {
-  PLUGIN_PRINTF(("FileDownloader::URLBufferStartNotify (pp_error=%"
-                 NACL_PRId32")\n", pp_error));
-
-  if (!InitialResponseIsValid(pp_error))
-    return;
-  // Finish streaming the body asynchronously providing a callback.
-  pp::CompletionCallback onread_callback =
-      callback_factory_.NewOptionalCallback(&FileDownloader::URLReadBodyNotify);
-
-  int32_t temp_size = static_cast<int32_t>(temp_buffer_.size());
-  pp_error = url_loader_.ReadResponseBody(&temp_buffer_[0],
-                                          temp_size,
-                                          onread_callback);
-  bool async_notify_ok = (pp_error == PP_OK_COMPLETIONPENDING);
-  PLUGIN_PRINTF(("FileDownloader::URLBufferStartNotify (async_notify_ok=%d)\n",
-                 async_notify_ok));
-  if (!async_notify_ok) {
-    onread_callback.Run(pp_error);
-  }
-}
-
 void FileDownloader::URLReadBodyNotify(int32_t pp_error) {
   PLUGIN_PRINTF(("FileDownloader::URLReadBodyNotify (pp_error=%"
                  NACL_PRId32")\n", pp_error));
   if (pp_error < PP_OK) {
-    file_open_notify_callback_.Run(pp_error);
+    stream_finish_callback_.Run(pp_error);
   } else if (pp_error == PP_OK) {
     if (streaming_to_user()) {
       data_stream_callback_source_->GetCallback().Run(PP_OK);
     }
-    FileOpenNotify(PP_OK);
+    StreamFinishNotify(PP_OK);
   } else {
     if (streaming_to_buffer()) {
       buffer_.insert(buffer_.end(), &temp_buffer_[0], &temp_buffer_[pp_error]);
@@ -398,10 +425,21 @@ bool FileDownloader::GetDownloadProgress(
                                          total_bytes_to_be_received);
 }
 
-void FileDownloader::FileOpenNotify(int32_t pp_error) {
-  PLUGIN_PRINTF(("FileDownloader::FileOpenNotify (pp_error=%"NACL_PRId32")\n",
-                 pp_error));
-  file_open_notify_callback_.Run(pp_error);
+nacl::string FileDownloader::GetResponseHeaders() const {
+  pp::Var headers = url_response_.GetHeaders();
+  if (!headers.is_string()) {
+    PLUGIN_PRINTF((
+        "FileDownloader::GetResponseHeaders (headers are not a string)\n"));
+    return nacl::string();
+  }
+  return headers.AsString();
+}
+
+void FileDownloader::StreamFinishNotify(int32_t pp_error) {
+  PLUGIN_PRINTF((
+      "FileDownloader::StreamFinishNotify (pp_error=%"NACL_PRId32")\n",
+      pp_error));
+  stream_finish_callback_.Run(pp_error);
 }
 
 bool FileDownloader::streaming_to_file() const {