MediaGalleriesCustomBindings: Ensure blobs aren't GC'd when getMetadata runs.

chrome/renderer/resources/extensions/media_galleries_custom_bindings.js

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Custom binding for the Media Gallery API.
 
 var binding = require('binding').Binding.create('mediaGalleries');
 var blobNatives = requireNative('blob_natives');
 var mediaGalleriesNatives = requireNative('mediaGalleries');
 
+var mediaFilesMetadata = {};

[tommycli, 2014/09/29 21:50:00]

This makes it sound like you're storing the metadata of the media file rather
than a reference to a Blob you need to hold onto because of a pending
getMetadata request.

Off the top of my head, 'blobsWithPendingMetadataRequests'? Ugh, I hope you can
think of a better name than that one...

[Oren Blasberg, 2014/09/29 22:05:48]

Done.

 var mediaGalleriesMetadata = {};
 
 function createFileSystemObjectsAndUpdateMetadata(response) {
   var result = [];
   mediaGalleriesMetadata = {};  // Clear any previous metadata.
   if (response) {
     for (var i = 0; i < response.length; i++) {
       var filesystem = mediaGalleriesNatives.GetMediaFileSystemObject(
           response[i].fsid);
       $Array.push(result, filesystem);
@@ skipping 73 matching lines @@
       'galleryId': '',
       'isRemovable': false,
       'isMediaDevice': false,
       'isAvailable': false,
     };
   });
 
   apiFunctions.setUpdateArgumentsPostValidate('getMetadata',
       function(mediaFile, options, callback) {
     var blobUuid = blobNatives.GetBlobUuid(mediaFile)
+    // Store the blob in a global object to keep its refcount nonzero -- this

[tommycli, 2014/09/29 21:50:00]

Perfect comment :)

[Oren Blasberg, 2014/09/29 22:05:48]

Thanks Tommy!

+    // prevents the object from being garbage collected before any metadata
+    // parsing gets to occur (see crbug.com/415792).
+    mediaFilesMetadata[blobUuid] = mediaFile;
     return [blobUuid, options, callback];
   });
 
   apiFunctions.setCustomCallback('getMetadata',
                                  function(name, request, response) {
     if (response.attachedImagesBlobInfo) {
       for (var i = 0; i < response.attachedImagesBlobInfo.length; i++) {
         var blobInfo = response.attachedImagesBlobInfo[i];
         var blob = blobNatives.TakeBrowserProcessBlob(
             blobInfo.blobUUID, blobInfo.type, blobInfo.size);
         response.metadata.attachedImages.push(blob);
       }
     }
 
     if (request.callback)
       request.callback(response.metadata);
     request.callback = null;
+
+    var uuid = request.args[0];

[tommycli, 2014/09/29 21:50:00]

Maybe a short one-line comment since this is not super obvious.

And don't worry, there's precedent for doing this:
https://code.google.com/p/chromium/codesearch/#search/&q=args%20custom_bindin...

[Oren Blasberg, 2014/09/29 22:05:48]

What sort of comment would be reasonable here? Remember, I'm still not sure how
args[0] ends up being the uuid after all.

[tommycli, 2014/09/29 22:09:25]

It's the uuid because of line 109: return [blobUuid, options, callback].

Maybe this doesn't even need a comment. I don't have a strong feeling about
this. You're the maintainer, you can make a judgement call.

+    delete mediaFilesMetadata[uuid];
   });
 });
 
 exports.binding = binding.generate();