Avoid leaking SerializedHandles.

Avoid leaking SerializedHandles.

This CL automatically closes SerializedHandles at the receiving side of ResourceMessageParams (the host side for ResourceMessageCallParams; the plugin side for ResourceMessageReplyParams), if they are not taken by message handlers.

BUG=None
TEST=None


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=165799

[yzshen1, 2012-10-31 17:18:10]

Hi, Brett.

Would you please take a look?

Note:
I set |handles_| in ResourceMessageParams to be mutable, in order to avoid
changing too many method signatures which have a const reference to
ResourceMessage{Call|Reply}Params.

I also considered to make a copy of the params and then take/append handles
using the copy. But that looks weird as well.

What do you think?

Thanks!

[brettw, 2012-10-31 20:15:59]

I'm not a huge fan of the mutable and I'd prefer to fix the callers. Maybe we
can do that in a followup patch?

http://codereview.chromium.org/11312017/diff/1/ppapi/proxy/resource_message_p...
File ppapi/proxy/resource_message_params.h (right):

http://codereview.chromium.org/11312017/diff/1/ppapi/proxy/resource_message_p...
ppapi/proxy/resource_message_params.h:33: // Removes and returns the handle at
the given index if it exists and is of
For all of these functions, can you be clear that the list isn't modified, just
the handle is nulled out? When you use "remove" I assume that I would keep
calling Take...(0) to eventually consume all of the handles, since it seems like
each call is removing the front one from the list.

http://codereview.chromium.org/11312017/diff/1/ppapi/proxy/resource_message_p...
ppapi/proxy/resource_message_params.h:109: mutable linked_ptr<SerializedHandles>
handles_;
I think linked_ptr is really to give you refcounting semantics when you don't
want your class to be refcounted. Since you're defining the class here, I think
it's cleaner to just make it refcounted and use a scoped_refptr here.

[yzshen1, 2012-10-31 21:15:27]

Thanks, Brett!

> I'm not a huge fan of the mutable and I'd prefer to fix the callers. Maybe we
> can do that in a followup patch?
Sure. I am happy to do so.

BTW, I have one question: do we have the need to put handles in
ResourceMessageCallParams?

http://codereview.chromium.org/11312017/diff/1/ppapi/proxy/resource_message_p...
File ppapi/proxy/resource_message_params.h (right):

http://codereview.chromium.org/11312017/diff/1/ppapi/proxy/resource_message_p...
ppapi/proxy/resource_message_params.h:33: // Removes and returns the handle at
the given index if it exists and is of
On 2012/10/31 20:15:59, brettw wrote:
> For all of these functions, can you be clear that the list isn't modified,
just
> the handle is nulled out? When you use "remove" I assume that I would keep
> calling Take...(0) to eventually consume all of the handles, since it seems
like
> each call is removing the front one from the list.

Done.

http://codereview.chromium.org/11312017/diff/1/ppapi/proxy/resource_message_p...
ppapi/proxy/resource_message_params.h:109: mutable linked_ptr<SerializedHandles>
handles_;
On 2012/10/31 20:15:59, brettw wrote:
> I think linked_ptr is really to give you refcounting semantics when you don't
> want your class to be refcounted. Since you're defining the class here, I
think
> it's cleaner to just make it refcounted and use a scoped_refptr here.

Done.

[brettw, 2012-10-31 21:22:02]

I'm hoping to avoid putting them in the call params although it's possible we
may need it at some point. In general the handles must come from "outside"
because of the sandbox and they'll be bound to a resource. For this pattern,
reply-only handles are sufficient.

[brettw, 2012-10-31 21:22:20]

lgtm

[commit-bot: I haz the power, 2012-11-01 17:11:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/11312017/3001

[commit-bot: I haz the power, 2012-11-01 17:41:13]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-02 21:02:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/11312017/12002

[commit-bot: I haz the power, 2012-11-02 21:28:59]

Retried try job too often for step(s) compile

[commit-bot: I haz the power, 2012-11-02 21:30:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/11312017/17

[commit-bot: I haz the power, 2012-11-02 23:24:58]

Change committed as 165799