PPAPI: Make Messaging not PostTask and copy when out-of-process.

PPAPI: Make Messaging not PostTask and copy when out-of-process.

This is a modification of dmichael's change at:
  https://codereview.chromium.org/11264049/
...except that it passes unit tests.

To prevent posting messages while the plugin element is being
initialized, we queue events early on, and don't post them to javascript
until our plugin is set up in the DOM.

From dmichael's change description:
"""
I ran John McCutchan's benchmark
(https://github.com/johnmccutchan/NaClComputeEngineBenchmark); this
improves the NaCl IPC proxy performance. Without this CL, the benchmark
will actually run out of memory and crash on the last test.
Results are here: https://docs.google.com/a/google.com/spreadsheet/ccc?key=0AuwLeGmMcfz0dDBXc1dmLUl6Q3dsSW1fUWVZWE5vaHc
"""

BUG=147597, 116317
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=172620

[dmichael (off chromium), 2012-12-10 22:02:52]

https://codereview.chromium.org/11490014/diff/2001/webkit/plugins/ppapi/messa...
File webkit/plugins/ppapi/message_channel.cc (right):

https://codereview.chromium.org/11490014/diff/2001/webkit/plugins/ppapi/messa...
webkit/plugins/ppapi/message_channel.cc:365: //  unblocked. Therefore, there's
no need to PostTask.
This comment probably belongs in the first branch of the if below now

https://codereview.chromium.org/11490014/diff/2001/webkit/plugins/ppapi/messa...
webkit/plugins/ppapi/message_channel.cc:370:
early_message_queue_.push_back(serialized_val);
Probably worth a brief comment why we have to defer sending (here or on the
member in the header)

And a comment here about why we can't just PostTask

https://codereview.chromium.org/11490014/diff/2001/webkit/plugins/ppapi/messa...
webkit/plugins/ppapi/message_channel.cc:391: void
MessageChannel::DrainEarlyMessageQueue() {
Might be worth doing:
DCHECK(!did_create_finished_);

https://codereview.chromium.org/11490014/diff/2001/webkit/plugins/ppapi/messa...
webkit/plugins/ppapi/message_channel.cc:394: early_message_queue_.pop_front();
If there's any chance of re-entry, you'd probably want to do something more
like:
WebSerializedScriptValue val = early_message_queue_.front();
early_message_queue_.pop_front();
PostMessageToJavaScriptImpl(val);

...it's unlikely, but it actually could happen for trusted in-process plugins,
if they do some sync scripting call in their JavaScript message event handler.

https://codereview.chromium.org/11490014/diff/2001/webkit/plugins/ppapi/messa...
File webkit/plugins/ppapi/message_channel.h (right):

https://codereview.chromium.org/11490014/diff/2001/webkit/plugins/ppapi/messa...
webkit/plugins/ppapi/message_channel.h:75: void DidCreateFinished();
suggestion: Might be easier to read as "FinishedDidCreate"? Since you can parse
"DidCreateFinished" as meaning "I created a Finished". Similar for
did_create_finished_.

[teravest, 2012-12-10 22:30:59]

https://codereview.chromium.org/11490014/diff/2001/webkit/plugins/ppapi/messa...
File webkit/plugins/ppapi/message_channel.cc (right):

https://codereview.chromium.org/11490014/diff/2001/webkit/plugins/ppapi/messa...
webkit/plugins/ppapi/message_channel.cc:365: //  unblocked. Therefore, there's
no need to PostTask.
On 2012/12/10 22:02:52, dmichael wrote:
> This comment probably belongs in the first branch of the if below now

Done.

https://codereview.chromium.org/11490014/diff/2001/webkit/plugins/ppapi/messa...
webkit/plugins/ppapi/message_channel.cc:370:
early_message_queue_.push_back(serialized_val);
On 2012/12/10 22:02:52, dmichael wrote:
> Probably worth a brief comment why we have to defer sending (here or on the
> member in the header)
> 
> And a comment here about why we can't just PostTask

Done.

https://codereview.chromium.org/11490014/diff/2001/webkit/plugins/ppapi/messa...
webkit/plugins/ppapi/message_channel.cc:391: void
MessageChannel::DrainEarlyMessageQueue() {
On 2012/12/10 22:02:52, dmichael wrote:
> Might be worth doing:
> DCHECK(!did_create_finished_);

Done.

https://codereview.chromium.org/11490014/diff/2001/webkit/plugins/ppapi/messa...
webkit/plugins/ppapi/message_channel.cc:394: early_message_queue_.pop_front();
I think it's safe as is...

On 2012/12/10 22:02:52, dmichael wrote:
> If there's any chance of re-entry, you'd probably want to do something more
> like:
> WebSerializedScriptValue val = early_message_queue_.front();
> early_message_queue_.pop_front();
> PostMessageToJavaScriptImpl(val);
> 
> ...it's unlikely, but it actually could happen for trusted in-process plugins,
> if they do some sync scripting call in their JavaScript message event handler.

https://codereview.chromium.org/11490014/diff/2001/webkit/plugins/ppapi/messa...
File webkit/plugins/ppapi/message_channel.h (right):

https://codereview.chromium.org/11490014/diff/2001/webkit/plugins/ppapi/messa...
webkit/plugins/ppapi/message_channel.h:75: void DidCreateFinished();
On 2012/12/10 22:02:52, dmichael wrote:
> suggestion: Might be easier to read as "FinishedDidCreate"? Since you can
parse
> "DidCreateFinished" as meaning "I created a Finished". Similar for
> did_create_finished_.

Done.

[dmichael (off chromium), 2012-12-10 22:49:47]

lgtm

https://codereview.chromium.org/11490014/diff/2001/webkit/plugins/ppapi/messa...
File webkit/plugins/ppapi/message_channel.cc (right):

https://codereview.chromium.org/11490014/diff/2001/webkit/plugins/ppapi/messa...
webkit/plugins/ppapi/message_channel.cc:394: early_message_queue_.pop_front();
On 2012/12/10 22:30:59, teravest wrote:
> I think it's safe as is...
> 
> On 2012/12/10 22:02:52, dmichael wrote:
> > If there's any chance of re-entry, you'd probably want to do something more
> > like:
> > WebSerializedScriptValue val = early_message_queue_.front();
> > early_message_queue_.pop_front();
> > PostMessageToJavaScriptImpl(val);
> > 
> > ...it's unlikely, but it actually could happen for trusted in-process
plugins,
> > if they do some sync scripting call in their JavaScript message event
handler.
> 
Yes, I wasn't thinking straight; the problem I was imagining would happen if
there was re-entry in to *this* method, which should never happen.

[commit-bot: I haz the power, 2012-12-10 23:00:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/11490014/5001

[commit-bot: I haz the power, 2012-12-10 23:53:46]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
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-12-10 23:53:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/11490014/15002

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

Retried try job too often on linux_rel for step(s) browser_tests

[teravest, 2012-12-11 17:42:47]

Fixed the CL so it passes tests for in-process, nacl plugins. The method names
are wordy now (QueueJavaScriptMessages/EndQueueingJavaScriptMessages), but they
should be called rarely, and at least they're descriptive now.

[commit-bot: I haz the power, 2012-12-11 17:48:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/11490014/17001

[dmichael (off chromium), 2012-12-11 18:55:55]

https://chromiumcodereview.appspot.com/11490014/diff/17001/webkit/plugins/ppa...
File webkit/plugins/ppapi/message_channel.cc (right):

https://chromiumcodereview.appspot.com/11490014/diff/17001/webkit/plugins/ppa...
webkit/plugins/ppapi/message_channel.cc:388: DCHECK(queue_js_messages_);
Do you think there's any chance of calling QueueJavaScriptMessages before the
Drain... task runs?

https://chromiumcodereview.appspot.com/11490014/diff/17001/webkit/plugins/ppa...
File webkit/plugins/ppapi/message_channel.h (right):

https://chromiumcodereview.appspot.com/11490014/diff/17001/webkit/plugins/ppa...
webkit/plugins/ppapi/message_channel.h:77: void EndQueueingJavaScriptMessages();
StopQueueingJavaScriptMessages? Slightly more imperative.

Or...  StopQueueingJavaScriptMessagesSoon(), to indicate it won't happen
immediately. (But that's probably getting too long)

https://chromiumcodereview.appspot.com/11490014/diff/17001/webkit/plugins/ppa...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

https://chromiumcodereview.appspot.com/11490014/diff/17001/webkit/plugins/ppa...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2392:
message_channel_->QueueJavaScriptMessages();
Probably worth a comment?

[teravest1, 2012-12-11 19:13:00]

On Tue, Dec 11, 2012 at 11:55 AM,  <dmichael@chromium.org> wrote:
>
>
https://chromiumcodereview.appspot.com/11490014/diff/17001/webkit/plugins/ppa...
> File webkit/plugins/ppapi/message_channel.cc (right):
>
>
https://chromiumcodereview.appspot.com/11490014/diff/17001/webkit/plugins/ppa...
> webkit/plugins/ppapi/message_channel.cc:388: DCHECK(queue_js_messages_);
> Do you think there's any chance of calling QueueJavaScriptMessages
> before the Drain... task runs?

Gross. I guess that's not impossible. I'll have to think for a bit
about how to deal with that.

>
>
https://chromiumcodereview.appspot.com/11490014/diff/17001/webkit/plugins/ppa...
> File webkit/plugins/ppapi/message_channel.h (right):
>
>
https://chromiumcodereview.appspot.com/11490014/diff/17001/webkit/plugins/ppa...
> webkit/plugins/ppapi/message_channel.h:77: void
> EndQueueingJavaScriptMessages();
> StopQueueingJavaScriptMessages? Slightly more imperative.
>
> Or...  StopQueueingJavaScriptMessagesSoon(), to indicate it won't happen
> immediately. (But that's probably getting too long)
>
>
https://chromiumcodereview.appspot.com/11490014/diff/17001/webkit/plugins/ppa...
> File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):
>
>
https://chromiumcodereview.appspot.com/11490014/diff/17001/webkit/plugins/ppa...
> webkit/plugins/ppapi/ppapi_plugin_instance.cc:2392:
> message_channel_->QueueJavaScriptMessages();
> Probably worth a comment?
>
> https://chromiumcodereview.appspot.com/11490014/

[teravest, 2012-12-11 21:32:12]

https://chromiumcodereview.appspot.com/11490014/diff/17001/webkit/plugins/ppa...
File webkit/plugins/ppapi/message_channel.cc (right):

https://chromiumcodereview.appspot.com/11490014/diff/17001/webkit/plugins/ppa...
webkit/plugins/ppapi/message_channel.cc:388: DCHECK(queue_js_messages_);
On 2012/12/11 18:55:55, dmichael wrote:
> Do you think there's any chance of calling QueueJavaScriptMessages before the
> Drain... task runs?

I cleaned this up by making an enum of the possible states. We can now cancel a
pending drain task.

https://chromiumcodereview.appspot.com/11490014/diff/17001/webkit/plugins/ppa...
File webkit/plugins/ppapi/message_channel.h (right):

https://chromiumcodereview.appspot.com/11490014/diff/17001/webkit/plugins/ppa...
webkit/plugins/ppapi/message_channel.h:77: void EndQueueingJavaScriptMessages();
On 2012/12/11 18:55:55, dmichael wrote:
> StopQueueingJavaScriptMessages? Slightly more imperative.
> 
> Or...  StopQueueingJavaScriptMessagesSoon(), to indicate it won't happen
> immediately. (But that's probably getting too long)

Done.

https://chromiumcodereview.appspot.com/11490014/diff/17001/webkit/plugins/ppa...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

https://chromiumcodereview.appspot.com/11490014/diff/17001/webkit/plugins/ppa...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2392:
message_channel_->QueueJavaScriptMessages();
On 2012/12/11 18:55:55, dmichael wrote:
> Probably worth a comment?

Done.

[dmichael (off chromium), 2012-12-11 22:35:30]

https://codereview.chromium.org/11490014/diff/26002/webkit/plugins/ppapi/mess...
File webkit/plugins/ppapi/message_channel.h (right):

https://codereview.chromium.org/11490014/diff/26002/webkit/plugins/ppapi/mess...
webkit/plugins/ppapi/message_channel.h:108: QUEUE,        // Queue JS messages.
I think it might be easier to understand with slightly more descriptive names...
QUEUE_MESSAGES, SEND_DIRECTLY, DRAIN_PENDING, DRAIN_CANCELLED (or CANCEL_DRAIN?)

https://codereview.chromium.org/11490014/diff/26002/webkit/plugins/ppapi/mess...
webkit/plugins/ppapi/message_channel.h:113: EarlyMessageQueueState
early_message_queue_state_;
Can you add a TODO or file a bug to remove this stuff?

https://codereview.chromium.org/11490014/diff/26002/webkit/plugins/ppapi/ppap...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

https://codereview.chromium.org/11490014/diff/26002/webkit/plugins/ppapi/ppap...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2392: // We're changing the
plugin's relationship to the DOM; don't send any
I think you can just remove the part before the semicolon...

maybe append something like ", otherwise we might be re-entered while we are in
an invalid state."
or something.

[teravest, 2012-12-11 22:43:39]

https://codereview.chromium.org/11490014/diff/26002/webkit/plugins/ppapi/mess...
File webkit/plugins/ppapi/message_channel.h (right):

https://codereview.chromium.org/11490014/diff/26002/webkit/plugins/ppapi/mess...
webkit/plugins/ppapi/message_channel.h:108: QUEUE,        // Queue JS messages.
On 2012/12/11 22:35:30, dmichael wrote:
> I think it might be easier to understand with slightly more descriptive
names...
> QUEUE_MESSAGES, SEND_DIRECTLY, DRAIN_PENDING, DRAIN_CANCELLED (or
CANCEL_DRAIN?)

Done.

https://codereview.chromium.org/11490014/diff/26002/webkit/plugins/ppapi/mess...
webkit/plugins/ppapi/message_channel.h:113: EarlyMessageQueueState
early_message_queue_state_;
On 2012/12/11 22:35:30, dmichael wrote:
> Can you add a TODO or file a bug to remove this stuff?

Done.

https://codereview.chromium.org/11490014/diff/26002/webkit/plugins/ppapi/ppap...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

https://codereview.chromium.org/11490014/diff/26002/webkit/plugins/ppapi/ppap...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2392: // We're changing the
plugin's relationship to the DOM; don't send any
On 2012/12/11 22:35:30, dmichael wrote:
> I think you can just remove the part before the semicolon...
> 
> maybe append something like ", otherwise we might be re-entered while we are
in
> an invalid state."
> or something.

Done.

[dmichael (off chromium), 2012-12-11 23:11:52]

lgtm

[commit-bot: I haz the power, 2012-12-12 01:22:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/11490014/33001

[commit-bot: I haz the power, 2012-12-12 05:23:13]

Retried try job too often on win_rel for step(s) chrome_frame_net_tests,
chrome_frame_unittests, content_browsertests, installer_util_unittests,
mini_installer_test, nacl_integration, sync_integration_tests

[commit-bot: I haz the power, 2012-12-12 15:34:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/teravest@chromium.org/11490014/33001

[commit-bot: I haz the power, 2012-12-12 17:13:46]

Change committed as 172620