Delay creation of BrowserPluginMsg_X messages until after attach.

Delay creation of BrowserPluginMsg_X messages until after attach.

Currently, if BrowserPluginGuest receives messages for a BrowserPlugin
prior to attachment, the message may be created with an invalid (0)
value of |browser_plugin_instance_id|. After attachment, when queued
messages are sent, these messages disappear into the ether.

This cl delays the actual message creation until after the correct
value for |browser_plugin_instance_id| is known through use of lambda
functionals.

This cl only implements this for BrowserPluginMsg_SetContentsOpaque,
with the expectation that all other BrowserPluginMsg_X types will be
converted to this methodology in a subsequent CL.

BUG=534839

[wjmaclean, 2015-10-15 20:58:23]

wjmaclean@chromium.org changed reviewers:
+ lazyboy@chromium.org, lfg@chromium.org

[wjmaclean, 2015-10-15 20:58:23]

Does this approach seem reasonable? I've noticed that <functional> is being used
elsewhere in Chrome, and since IPC messages are hard to change once created,
delaying creation seemed like a good option.

If it does look resonable, shall we try to do all message types at once? Or
shall we split it over several CLs?

[wjmaclean, 2015-10-15 20:58:30]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-15 20:58:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1404353004/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1404353004/1

[lfg, 2015-10-15 21:03:37]

I believe std::function is still not available because Mac still doesn't have a
C++11 library.

[wjmaclean, 2015-10-15 21:03:48]

On 2015/10/15 20:58:23, wjmaclean wrote:
> Does this approach seem reasonable? I've noticed that <functional> is being
used
> elsewhere in Chrome, and since IPC messages are hard to change once created,
> delaying creation seemed like a good option.
> 
> If it does look resonable, shall we try to do all message types at once? Or
> shall we split it over several CLs?

Btw, I'll add a DCHECK() that browser_plugin_instance_id != kBrowserPluginNone
inside the lambda, or perhaps were the lambda is invoked ...

[commit-bot: I haz the power, 2015-10-15 21:19:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-15 21:19:33]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[lazyboy, 2015-10-16 00:15:48]

https://chromiumcodereview.appspot.com/1404353004/diff/1/content/browser/brow...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/1404353004/diff/1/content/browser/brow...
content/browser/browser_plugin/browser_plugin_guest.cc:527: // During tests,
attache() may be true when there is no owner_web_contents_;
attached()

https://chromiumcodereview.appspot.com/1404353004/diff/1/content/browser/brow...
content/browser/browser_plugin/browser_plugin_guest.cc:598: while
(!pending_message_funcs_.empty()) {
Having two queues will send messages out of order, is there any issue with this?

https://chromiumcodereview.appspot.com/1404353004/diff/1/content/browser/brow...
File content/browser/browser_plugin/browser_plugin_guest.h (right):

https://chromiumcodereview.appspot.com/1404353004/diff/1/content/browser/brow...
content/browser/browser_plugin/browser_plugin_guest.h:74: typedef
std::function<IPC::Message*(int browser_plugin_id)>
Guess the mac bots didn't like it. Lucas and I couldn't find any instance of
std::function in chrome/content/blink.

[Fady Samuel, 2015-10-16 02:48:42]

fsamuel@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Fady Samuel, 2015-10-16 02:48:43]

https://codereview.chromium.org/1404353004/diff/1/content/browser/browser_plu...
File content/browser/browser_plugin/browser_plugin_guest.cc (left):

https://codereview.chromium.org/1404353004/diff/1/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_guest.cc:572:
linked_ptr<IPC::Message> message_ptr = pending_messages_.front();
Perhaps you can grab the payload of the IPC::Message here and modify it instead,
creating a new IPC::Message with the modified payload? Naively you could do that
for all queued IPCs, which might not be too bad, I don't expect the queue to be
huge. Alternatively, you can read the instance ID and if it's kInstanceIDNone,
then you can create a new IPC::Message with a modified payload.
browser_plugin_instance_id field is always in the same place in the payload
buffer:

https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/b...

[wjmaclean, 2015-10-16 13:05:32]

On 2015/10/16 02:48:43, Fady Samuel wrote:
>
https://codereview.chromium.org/1404353004/diff/1/content/browser/browser_plu...
> File content/browser/browser_plugin/browser_plugin_guest.cc (left):
> 
>
https://codereview.chromium.org/1404353004/diff/1/content/browser/browser_plu...
> content/browser/browser_plugin/browser_plugin_guest.cc:572:
> linked_ptr<IPC::Message> message_ptr = pending_messages_.front();
> Perhaps you can grab the payload of the IPC::Message here and modify it
instead,
> creating a new IPC::Message with the modified payload? Naively you could do
that
> for all queued IPCs, which might not be too bad, I don't expect the queue to
be
> huge. Alternatively, you can read the instance ID and if it's kInstanceIDNone,
> then you can create a new IPC::Message with a modified payload.
> browser_plugin_instance_id field is always in the same place in the payload
> buffer:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/b...

This was the first approach I considered ... but IPC messages are not really
designed to be mutable, and the idea of creating a modified payload in a new
message, while feasible, just seemed messy. However, it may well be what we end
up doing.

The functional route looked much cleaner, alas it's a bit ahead of its time :-(

[wjmaclean, 2015-10-16 15:54:54]

Closing this issue in favour of the approach at
https://codereview.chromium.org/1410753003.