Pepper WebSocket API: Implement new design Chrome IPC

Pepper WebSocket API: Implement new design Chrome IPC.

This change implements new Chrome IPC for PPB_WebSocket.
After this change, all mode including out of process
will work with new design. It doesn't depend on old SRPC design any more.

BUG=87310, 116317
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=160783

[Takashi Toyoshima, 2012-09-19 00:52:56]

I took over Brett's CL and start to implement it.

I just make the CL pass compilation and try to run the simplest
'WebSocket_IsWebSocket' tests.

PPAPITest, PPAPINaClNewlibTest and PPAPINaClGLibcTest passed.
But, I could not pass OutOfProcessPPAPITest.WebSocket_IsWebSocket because the
interface doesn't seem to be found.

Is it expected behavior? I'm not sure OutOfProcessPPAPITest should pass with new
design.

Also, your CL contains PROXIED_IFACE entry in interfaces_ppb_public_stable.h.
I could not find the write way to pass this line, so commented out for now. What
is the right direction?

http://codereview.chromium.org/10944005/diff/2001/ppapi/thunk/interfaces_ppb_...
File ppapi/thunk/interfaces_ppb_public_stable.h (right):

http://codereview.chromium.org/10944005/diff/2001/ppapi/thunk/interfaces_ppb_...
ppapi/thunk/interfaces_ppb_public_stable.h:80: //PROXIED_IFACE(NoAPIName,
PPB_WEBSOCKET_INTERFACE_1_0, PPB_WebSocket_1_0)
Is this entry really needed?

[brettw, 2012-09-24 18:25:36]

http://codereview.chromium.org/10944005/diff/16004/ppapi/thunk/interfaces_ppb...
File ppapi/thunk/interfaces_ppb_public_stable.h (right):

http://codereview.chromium.org/10944005/diff/16004/ppapi/thunk/interfaces_ppb...
ppapi/thunk/interfaces_ppb_public_stable.h:80: //PROXIED_IFACE(NoAPIName,
PPB_WEBSOCKET_INTERFACE_1_0, PPB_WebSocket_1_0)
This is why the interface isn't found: you need something like this line to hook
up the registration in ppapi/proxy/interface_list.cc.

I notice there's an old entry for WebSocket in interfaces_ppb_public_dev. Since
this isn't dev anymore, we should remove that (remove all lines referencing
websocket from that file). With the "new proxy" you shouldn't need a
"[UN]PROXIED_API" line like is in the other file, only the PROXIED_IFACE line.

[Takashi Toyoshima, 2012-09-25 17:46:04]

Hum... when I enabled the PROXIED_IFACE line, linking failed with the following
error.

>
/usr/local/google/home/toyoshim/chromium/src/ppapi/../ppapi/thunk/interfaces_pp
> b_public_stable.h:80: undefined reference to
`ppapi::thunk::GetPPB_WebSocket_1_
> 0_Thunk()' 

Once, I succeeded to link browser_tests, then all out of process tests passed
with patch 6.
But, after rebasing, I can not make it anymore. Also I could not reproduce
successful build with patch 6 anymore. Do you have any idea to fix this linking
problem?

[brettw, 2012-09-25 19:50:31]

On Tue, Sep 25, 2012 at 10:46 AM,  <toyoshim@chromium.org> wrote:
> Hum... when I enabled the PROXIED_IFACE line, linking failed with the
> following
> error.
>
>
>
/usr/local/google/home/toyoshim/chromium/src/ppapi/../ppapi/thunk/interfaces_pp
>>
>> b_public_stable.h:80: undefined reference to
>
> `ppapi::thunk::GetPPB_WebSocket_1_
>>
>> 0_Thunk()'

Try removing the nacl exclusion of ppb_websocket_thunk.cc in
ppapi/ppapy_proxy.gypi. I think that's what's causing your problem.
I'm guessing they don't include that file in the nacl proxy build
because it wasn't necessary until now.


> Once, I succeeded to link browser_tests, then all out of process tests
> passed
> with patch 6.
> But, after rebasing, I can not make it anymore. Also I could not reproduce
> successful build with patch 6 anymore. Do you have any idea to fix this
> linking
> problem?
>
> https://codereview.chromium.org/10944005/

Sorry, without any more information about what tests or error messages
you get, I can't give any useful suggestions.

Brett

[Takashi Toyoshima, 2012-09-26 00:06:46]

Thank you Brett.
Adding ppb_websocket_thunk.cc into ppapi_proxy.gypi works.
Now patch set 9 passed all tests again.

By the way, WebSocket proxy requires renderer side initiated IPCs, but currently
it looks not supported yet. I use workaround for now as sending request first,
then making multiple replies for that from renderer side. Is anyone going to
implement proper way to use renderer initiated IPC on PPAPI proxy?

[brettw, 2012-09-27 05:09:41]

You should just be able to send a "reply" without a call. Make the sequence
number 0 in this case. I was also thinking of a patch that needed this but I
didn't yet document that it's supported.

[Takashi Toyoshima, 2012-10-03 01:35:00]

Hi.

Now, I still have a build problem on android and windows.
But, can you start review on this change?
I'll investigate build issue in parallel and if you have any advice on this
build trouble, I appreciate it.

Brett,
This change contains a hack to realize unsolicited IPC replies with sequence 0.

Thanks.

[brettw, 2012-10-03 04:54:02]

This looks pretty good, thanks for the fast work.

http://codereview.chromium.org/10944005/diff/52001/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_websocket_host.cc (right):

http://codereview.chromium.org/10944005/diff/52001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_websocket_host.cc:74: "",
I prefer std::string() over ""

http://codereview.chromium.org/10944005/diff/52001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_websocket_host.cc:151: unhandled_buffered_amount,
Can you indent these one more space (so 4 extra from prev line).

http://codereview.chromium.org/10944005/diff/52001/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

http://codereview.chromium.org/10944005/diff/52001/ppapi/proxy/ppapi_messages...
ppapi/proxy/ppapi_messages.h:1579: // WebSocket
------------------------------------------------------------------
Can you put a blank line below all your hyphens?

http://codereview.chromium.org/10944005/diff/52001/ppapi/proxy/ppapi_messages...
ppapi/proxy/ppapi_messages.h:1580:
IPC_MESSAGE_CONTROL0(PpapiHostMsg_WebSocket_Create)
I'm trying to document the IPC messages better. We've done a bad job so far.
Most of the parameters are obvious and you don't need to explain them, but I
think it's worth mentioning when there is a reply what the reply message is for
the request, and mention what call message each reply is for.

The "out of the blue" replies should also be clear that they aren't in response
to anything in particular.

I'm also confused about the difference between CloseReply and ClosedReply.

http://codereview.chromium.org/10944005/diff/52001/ppapi/proxy/websocket_reso...
File ppapi/proxy/websocket_resource.cc (right):

http://codereview.chromium.org/10944005/diff/52001/ppapi/proxy/websocket_reso...
ppapi/proxy/websocket_resource.cc:78: PP_Var url,
Can you make this (in the _api as well) a const ref? The same in the other
functions in the abstract interface.

http://codereview.chromium.org/10944005/diff/52001/ppapi/proxy/websocket_reso...
ppapi/proxy/websocket_resource.cc:118: const uint8_t minimumProtocolCharacter =
'!';  // U+0021.
To what extent is this checking security sensitive? If it's not sensitive and
just a nice thing to check, you should mention that in the comment. But if its
important that this check is correct (which I suspect it is) we should be doing
it on the host side instead, since the plugin code isn't trusted.

I think it's OK to have an asynchronous failure if you pass a bad protocol.
Although less convenient for the plugin, it prevents duplicate code and
presumably the protocols are hardcoded, so it you screw it up you just fix your
code and then you don't have any more problems.

http://codereview.chromium.org/10944005/diff/52001/ppapi/proxy/websocket_reso...
ppapi/proxy/websocket_resource.cc:219:
CallRenderer(PpapiHostMsg_WebSocket_Close((int32_t)event_code,
Nit: use C++ casts.

http://codereview.chromium.org/10944005/diff/52001/ppapi/proxy/websocket_reso...
ppapi/proxy/websocket_resource.cc:288: // TODO(toyoshim): Check if
ppapi::proxy::SerializedVar can reduce copy.
Nope, doing this won't save a copy, so you can just remove this todo.

http://codereview.chromium.org/10944005/diff/52001/ppapi/proxy/websocket_reso...
ppapi/proxy/websocket_resource.cc:332: return StringVar::StringToPPVar("");
Can you use std::string() instead of ""? It's more accurate and theoretically
saves some work in the string constructor.

http://codereview.chromium.org/10944005/diff/52001/ppapi/proxy/websocket_reso...
ppapi/proxy/websocket_resource.cc:367: break; }
I've usually seen the closing } on the next line. So 
    break;
  }
  case PpapiPl...

http://codereview.chromium.org/10944005/diff/52001/ppapi/proxy/websocket_reso...
ppapi/proxy/websocket_resource.cc:505: 
Nit: blank line

http://codereview.chromium.org/10944005/diff/52001/ppapi/proxy/websocket_reso...
File ppapi/proxy/websocket_resource.h (right):

http://codereview.chromium.org/10944005/diff/52001/ppapi/proxy/websocket_reso...
ppapi/proxy/websocket_resource.h:86: // didReceiveMessage callback.
Is this comment out-of-date?

[Takashi Toyoshima, 2012-10-03 11:41:50]

I reflect review comments.
Could you take another look though I'm still struggling with android and win32
builds.

[Takashi Toyoshima, 2012-10-03 11:42:20]

http://codereview.chromium.org/10944005/diff/52001/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_websocket_host.cc (right):

http://codereview.chromium.org/10944005/diff/52001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_websocket_host.cc:74: "",
OK. I replaced all "" with std::string() in this change.

http://codereview.chromium.org/10944005/diff/52001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_websocket_host.cc:151: unhandled_buffered_amount,
On 2012/10/03 04:54:02, brettw wrote:
> Can you indent these one more space (so 4 extra from prev line).

Done.

http://codereview.chromium.org/10944005/diff/52001/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

http://codereview.chromium.org/10944005/diff/52001/ppapi/proxy/ppapi_messages...
ppapi/proxy/ppapi_messages.h:1579: // WebSocket
------------------------------------------------------------------
On 2012/10/03 04:54:02, brettw wrote:
> Can you put a blank line below all your hyphens?

Done.

http://codereview.chromium.org/10944005/diff/52001/ppapi/proxy/ppapi_messages...
ppapi/proxy/ppapi_messages.h:1580:
IPC_MESSAGE_CONTROL0(PpapiHostMsg_WebSocket_Create)
On 2012/10/03 04:54:02, brettw wrote:
> I'm trying to document the IPC messages better. We've done a bad job so far.
> Most of the parameters are obvious and you don't need to explain them, but I
> think it's worth mentioning when there is a reply what the reply message is
for
> the request, and mention what call message each reply is for.
> 
> The "out of the blue" replies should also be clear that they aren't in
response
> to anything in particular.
> 
> I'm also confused about the difference between CloseReply and ClosedReply.

Done.

http://codereview.chromium.org/10944005/diff/52001/ppapi/proxy/websocket_reso...
File ppapi/proxy/websocket_resource.cc (right):

http://codereview.chromium.org/10944005/diff/52001/ppapi/proxy/websocket_reso...
ppapi/proxy/websocket_resource.cc:78: PP_Var url,
I think we have not used const ref for PP_Var usually in NaCl code base. But, I
agreed that we should use const ref because it is recommended as Google/Chromium
coding styles.
OK, I fix it in the next CL.

http://codereview.chromium.org/10944005/diff/52001/ppapi/proxy/websocket_reso...
ppapi/proxy/websocket_resource.cc:219:
CallRenderer(PpapiHostMsg_WebSocket_Close((int32_t)event_code,
On 2012/10/03 04:54:02, brettw wrote:
> Nit: use C++ casts.

Done.

http://codereview.chromium.org/10944005/diff/52001/ppapi/proxy/websocket_reso...
ppapi/proxy/websocket_resource.cc:288: // TODO(toyoshim): Check if
ppapi::proxy::SerializedVar can reduce copy.
Thanks!

http://codereview.chromium.org/10944005/diff/52001/ppapi/proxy/websocket_reso...
ppapi/proxy/websocket_resource.cc:332: return StringVar::StringToPPVar("");
On 2012/10/03 04:54:02, brettw wrote:
> Can you use std::string() instead of ""? It's more accurate and theoretically
> saves some work in the string constructor.

Done.

http://codereview.chromium.org/10944005/diff/52001/ppapi/proxy/websocket_reso...
ppapi/proxy/websocket_resource.cc:367: break; }
On 2012/10/03 04:54:02, brettw wrote:
> I've usually seen the closing } on the next line. So 
>     break;
>   }
>   case PpapiPl...

Done.

http://codereview.chromium.org/10944005/diff/52001/ppapi/proxy/websocket_reso...
ppapi/proxy/websocket_resource.cc:505: 
On 2012/10/03 04:54:02, brettw wrote:
> Nit: blank line

Done.

http://codereview.chromium.org/10944005/diff/52001/ppapi/proxy/websocket_reso...
File ppapi/proxy/websocket_resource.h (right):

http://codereview.chromium.org/10944005/diff/52001/ppapi/proxy/websocket_reso...
ppapi/proxy/websocket_resource.h:86: // didReceiveMessage callback.
On 2012/10/03 04:54:02, brettw wrote:
> Is this comment out-of-date?

Done.

http://codereview.chromium.org/10944005/diff/57019/ppapi/proxy/websocket_reso...
File ppapi/proxy/websocket_resource.cc (right):

http://codereview.chromium.org/10944005/diff/57019/ppapi/proxy/websocket_reso...
ppapi/proxy/websocket_resource.cc:332: case
PpapiPluginMsg_WebSocket_ConnectReply::ID: {
Now, I stop to use sequence numbers also for ConnectReply and CloseReply. This
is because these Replies might be received before caller get request ID in the
latest build.

E.g., I confirmed that ConnectReply with sequence ID 1 is received here before
CallRenderer(PpapiHostMsg_WebSocket_Connect(...)) returns and provide sequence
ID 1. In this case, we can not catch the proper reply messages with checking
sequence ID.

[Takashi Toyoshima, 2012-10-03 11:46:28]

http://codereview.chromium.org/10944005/diff/52001/ppapi/proxy/websocket_reso...
File ppapi/proxy/websocket_resource.cc (right):

http://codereview.chromium.org/10944005/diff/52001/ppapi/proxy/websocket_reso...
ppapi/proxy/websocket_resource.cc:118: const uint8_t minimumProtocolCharacter =
'!';  // U+0021.
Firstly, I think this check is safe to run in NaCl.
But, now I notice that injecting <cr><lf> might cause security problem because
it means that user can inject arbitrary http request headers. Anyway, it must be
safe to implement this in renderer against unknown security issues.
I removed this check into renderer side.

[Takashi Toyoshima, 2012-10-03 14:14:46]

Hi Brett,
Sorry for many notifications.

Now, I resolved build issues. So Patch Set 16 could be a candidate for landing.

[brettw, 2012-10-03 20:30:24]

Thanks, I found a few more issues but I think this should be the last pass of
nontrivial changes...


One of the things with the new proxy design is that the IPC layer gives us an
easy way to do unit tests on the resources without doing all the work to load a
plugin. This makes some internals possible to test, and the tests run much
faster and more reliably. You can see an example in
ppapi/proxy/file_chooser_resource_unittest.cc and in
printing_resource_unittest.cc: these call Pepper functions and then check that
the right IPC messages were sent by the resource object.

It looks like we have quite nice tests for web sockets in
ppapi/test/test_websocket from the plugin perspective. Is there anything that's
missing from these tests that could be done using a test that uses the plugin
resource + IPC directly? Like maybe a parsing test or something that various
combinations of input get converted into the vector<string> over IPC?

I'd like to get at least one unittest for this. Even if you feel like the
current tests give us great coverage, I at least want there to be a unittest
file in the directory for when we find bugs, whoever fixes them will feel
obligated to find a test :) Hopefully you can come up with something useful
that's not covered by the existing integration tests.

http://codereview.chromium.org/10944005/diff/52001/ppapi/proxy/websocket_reso...
File ppapi/proxy/websocket_resource.cc (right):

http://codereview.chromium.org/10944005/diff/52001/ppapi/proxy/websocket_reso...
ppapi/proxy/websocket_resource.cc:78: PP_Var url,
We shouldn't change the C bindings, which are the binary layer which must be C.
But this layer of stuff, along with the thunk code, is internal functions that
can do whatever we want. I usually use const refs for the _API interfaces as per
Chrome style. It should be easy to change in this patch since you're already
touching all of the places you need to change anyway.

http://codereview.chromium.org/10944005/diff/59041/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_websocket_host.cc (right):

http://codereview.chromium.org/10944005/diff/59041/content/renderer/pepper/pe...
content/renderer/pepper/pepper_websocket_host.cc:170:
host()->SendReply(connect_reply_,
As with the comment below, you shouldn't need to explicitly call SendReply
unless you want to return non-default arguments or you need to return sometime
in the future (in which case you would return PP_OK_COMPLETIONPENDING) from the
message handler. 

At first I thought you should just replace this with "return DoConnect(url,
protocols, &spec);" but this misses the case where the completion happens in the
future, which I think is the normal way for DoConnect to succeed. In this case,
I think your current code is sending the reply twice.

What you should do is at the bottom of DoConnect where we know the call is being
sent to WebKit and will complete asynchronously, is to save the connect_reply_
and return PP_OK_COMPLETIONPENDING. Otherwise, we should return the error
immediately and not save connect_reply_, which will implicitly cause a reply to
be sent by the PpapiHost to this message.

http://codereview.chromium.org/10944005/diff/59041/content/renderer/pepper/pe...
content/renderer/pepper/pepper_websocket_host.cc:181: if (!websocket_.get()) {
In this error case, I believe you can just return PP_ERROR_FAILED, which will
send the reply back, and then you'll call the message handler with the default
args in the resource. So I don't think you need the explicit SendReply.

Also, doing this will fix the bug where we forgot to return early from this
error case and will dereference the websocket_ below.

You shouldn't set close_reply_ in the error case. I'd move the first line in
this function below the websocket_ null check.

http://codereview.chromium.org/10944005/diff/59041/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

http://codereview.chromium.org/10944005/diff/59041/ppapi/proxy/ppapi_messages...
ppapi/proxy/ppapi_messages.h:1660: // unexpected network errors will invoke this
message.
Thanks, this is much clearer to me now.

http://codereview.chromium.org/10944005/diff/59041/ppapi/proxy/websocket_reso...
File ppapi/proxy/websocket_resource.cc (right):

http://codereview.chromium.org/10944005/diff/59041/ppapi/proxy/websocket_reso...
ppapi/proxy/websocket_resource.cc:57: empty_string_(new StringVar("", 0)),
Can you use "new StringVar(std::string())," here instead?

http://codereview.chromium.org/10944005/diff/59041/ppapi/proxy/websocket_reso...
ppapi/proxy/websocket_resource.cc:279:
CallRenderer(PpapiHostMsg_WebSocket_SendBinary(message_vector));
For the cases where you don't need a callback, you should use 
PostToRenderer instead. This will avoid sending the reply. Otherwise, the
PpapiHost code will implicitly generate a reply containing PP_OK on the host
side, which will be unnecessary.

So can you go through all the places where you do "Call" for messages without
replies and replace them with "Post"?

http://codereview.chromium.org/10944005/diff/59041/ppapi/proxy/websocket_reso...
ppapi/proxy/websocket_resource.cc:464: return;
Indenting is one space too much. This is also the case in the previous function
and maybe others (I assume this check was copy and pasted).

[Takashi Toyoshima, 2012-10-04 02:18:22]

Hi, Brett.
Default IPC reply has a type 0, and should be identified by sequence ID in
resource side.
But it can be returned before CallRenderer returns request ID to caller.
It means that we could not identify which is the corresponding request for the
auto generated reply.
I can avoid this issue by workaround like having 'bool calling_connect' flag
while processing CallRenderer(). But I think this is not proper solution. What
do you think about this?

[brettw, 2012-10-04 03:34:49]

Under what conditions can it be returned before the CallRenderer returns? The
current code for in-process does a PostTask so should never be reentrant, and
the out-of-process case always goes back to the message loop.

Victor is changing this in http://codereview.chromium.org/11053003/ and I
realized some advice that I gave him would cause what you're describing to
happen. But I don't think it should happen with the current code, and I don't
think this will happen when Victor does what I just suggested on his code
review.

[Takashi Toyoshima, 2012-10-04 03:40:22]

Hum..., I try it with the latest revision.
FYI, I faced the problematic cases in PPAPITest with baseline r159828 yesterday.

[Takashi Toyoshima, 2012-10-04 05:38:46]

This problem still exist in the latest revision.
I use new CallRenderer<> with CallbackType. But against auto-generated replies,
the callback is not invoked, then type 0 event is passed to
WebSocketResource::OnReplyReceived() instead.

[Takashi Toyoshima, 2012-10-04 06:08:16]

Ah... OK, I see.
I should call PluginResource::OnReplyReceived() explicitly in default case.

Actually, IPC replies comes before SendResourceCall() finishes in
PluginResource::CallHost().
But there, callback is registered with sequence ID before calling
SendResourceCall().
So, PluginResource::OnReplyReceived() can handle these auto-generated replies
correctly.

[Takashi Toyoshima, 2012-10-04 14:52:54]

Ugh, today I can not solve following problems.

1. As I said, Reply IPCs are received before CallRender returns except for out
of process mode.
2. TrackedCallback looks not invoked as PP_ERROR_ABORTED though they are still
active when the resource is going to destroyed (I confirmed that destructor of
the resource is called).

I'll continue tomorrow.

[brettw, 2012-10-04 18:01:12]

I talked to Victor today and it turns out I was confused about two
patches, and the patch that caused the reentrant calls you're seeing
for the in-process case was already checked in:
http://src.chromium.org/viewvc/chrome?view=rev&revision=159625
So this is why you're seeing that problem. The fix for this is simple
and we can hopefully get it in today. If you want to test your code,
you can just revert that change locally and it should fix the "reply
returning before the call does" problem. Sorry for the confusion!

FYI Raymes landed his improvement to the call reply handling:
http://src.chromium.org/viewvc/chrome?view=rev&revision=160015
It should fix all the nasty code you had to write to look up the
replies by request ID by folding it all into a nice base::Bind call
when you make the call. You can see examples of the existing resources
in that patch. You can feel free to keep your patch as-is and make
this change in a follow-up patch if you prefer. I know it's
inconvenient that not all of this stuff was working perfectly when you
started working on this.

--

I have not heard of any problems with TrackedCallback and nothing has
changed about that recently, so I'm surprised. What's supposed to
happen is that ppapi/shared_impl/callback_tracker.cc should get a call
for PostAbortForResource with the resource ID when the object is
released by the plugin. This is called by the ResourceTracker when the
plugin releases its last reference.

One confusing thing is that there are two reference counts. Resources
are RefCounted and our internal code can keep references to them. The
plugin does reference counting as well, and this reference count is
tracked by the ResourceTracker. The ResourceTracker keeps one "real"
internal reference on behalf of the plugin, no matter how many
references the plugin has taken. This means we can keep a resource in
a scoped_refptr internally, and a bad plugin can't call
ReleaseResource a bunch of extra times and start messing up our
internal references.

The PostAbortForResource is called when the last plugin reference is
released. We could still keep the object alive internally if we have a
scoped_refptr to the resource object. However, I don't think you will
encounter this case, I'm telling you primarily so some of the resource
tracking code makes more sense when you step through it.

Brett

[Takashi Toyoshima, 2012-10-05 01:23:39]

short update;
reverting router change resolves both troubles.
Thanks!

[brettw, 2012-10-05 03:08:49]

The fix should be in:
http://src.chromium.org/viewvc/chrome?view=rev&revision=160254

Please let me know if you're still seeing problems after this and
we'll need to investigate more.

Brett

On Thu, Oct 4, 2012 at 6:23 PM,  <toyoshim@chromium.org> wrote:
> short update;
> reverting router change resolves both troubles.
> Thanks!
>
> http://codereview.chromium.org/10944005/

[Takashi Toyoshima, 2012-10-05 07:35:01]

Thanks.
I finished resolving issues, and adding resource unit tests.
Could you take another look on this?

http://codereview.chromium.org/10944005/diff/52001/ppapi/proxy/websocket_reso...
File ppapi/proxy/websocket_resource.cc (right):

http://codereview.chromium.org/10944005/diff/52001/ppapi/proxy/websocket_reso...
ppapi/proxy/websocket_resource.cc:78: PP_Var url,
Thank you.
Now I understand the reason of PP_Var correctly.

http://codereview.chromium.org/10944005/diff/59041/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_websocket_host.cc (right):

http://codereview.chromium.org/10944005/diff/59041/content/renderer/pepper/pe...
content/renderer/pepper/pepper_websocket_host.cc:170:
host()->SendReply(connect_reply_,
On 2012/10/03 20:30:24, brettw wrote:
> As with the comment below, you shouldn't need to explicitly call SendReply
> unless you want to return non-default arguments or you need to return sometime
> in the future (in which case you would return PP_OK_COMPLETIONPENDING) from
the
> message handler. 
> 
> At first I thought you should just replace this with "return DoConnect(url,
> protocols, &spec);" but this misses the case where the completion happens in
the
> future, which I think is the normal way for DoConnect to succeed. In this
case,
> I think your current code is sending the reply twice.
> 
> What you should do is at the bottom of DoConnect where we know the call is
being
> sent to WebKit and will complete asynchronously, is to save the connect_reply_
> and return PP_OK_COMPLETIONPENDING. Otherwise, we should return the error
> immediately and not save connect_reply_, which will implicitly cause a reply
to
> be sent by the PpapiHost to this message.

Done.

http://codereview.chromium.org/10944005/diff/59041/content/renderer/pepper/pe...
content/renderer/pepper/pepper_websocket_host.cc:181: if (!websocket_.get()) {
On 2012/10/03 20:30:24, brettw wrote:
> In this error case, I believe you can just return PP_ERROR_FAILED, which will
> send the reply back, and then you'll call the message handler with the default
> args in the resource. So I don't think you need the explicit SendReply.
> 
> Also, doing this will fix the bug where we forgot to return early from this
> error case and will dereference the websocket_ below.
> 
> You shouldn't set close_reply_ in the error case. I'd move the first line in
> this function below the websocket_ null check.

Done.

http://codereview.chromium.org/10944005/diff/59041/ppapi/proxy/websocket_reso...
File ppapi/proxy/websocket_resource.cc (right):

http://codereview.chromium.org/10944005/diff/59041/ppapi/proxy/websocket_reso...
ppapi/proxy/websocket_resource.cc:57: empty_string_(new StringVar("", 0)),
On 2012/10/03 20:30:24, brettw wrote:
> Can you use "new StringVar(std::string())," here instead?

Done.

http://codereview.chromium.org/10944005/diff/59041/ppapi/proxy/websocket_reso...
ppapi/proxy/websocket_resource.cc:279:
CallRenderer(PpapiHostMsg_WebSocket_SendBinary(message_vector));
Thank you.
I misunderstood about params.has_callback() check in
PpapiHost::HandleResourceCall() because I didn't know PostToRenderer. This
misunderstanding is the reason why I implemented SendReply() in
pepper_websocket_host.cc implicitly.

Yes, I use PostToRenderer here.

http://codereview.chromium.org/10944005/diff/59041/ppapi/proxy/websocket_reso...
ppapi/proxy/websocket_resource.cc:464: return;
Oops.
Thank you for catching these nits.

[Takashi Toyoshima, 2012-10-07 05:13:19]

after Patch Set 19, I revise three points to resolve test failures.

1. reset connecting_ flag in pepper_websocket_host.cc in order to avoid invoke
ConnectReply rwice. (Patch Set 20)
 This was a bug simply because I forgot to write.

2. should check NOTREACHED() in plugin_dispatcher.cc iff IPC sequence is not
zero. (Patch Set 21)
 Unsolicited replies can be received after the receiving resource is destructed
because no one keep to have a reference to the resource for Unsolicited replies.

3. use TrackedCallback::IsPending in websocket_resource.cc to check if holding
callbacks requires invocation. (Patch Set 22)
 TrackedCallback can be invoked automatically in the case of abnormal
destruction like AbortCalls test. So we should use IsPending() to check if the
callback is already invoked. Also, we still need to reset the pointer when we
call PostRun/PostAbort because completed flag is set delayed and IsPending()
could not check the scheduled post task.

[brettw, 2012-10-08 21:45:45]

LGTM!

https://codereview.chromium.org/10944005/diff/57020/ppapi/proxy/websocket_res...
File ppapi/proxy/websocket_resource_unittest.cc (right):

https://codereview.chromium.org/10944005/diff/57020/ppapi/proxy/websocket_res...
ppapi/proxy/websocket_resource_unittest.cc:48: }
Nit: need "  // namespace"

[bbudge, 2012-10-08 22:15:56]

Drive by, a couple of nits.

http://codereview.chromium.org/10944005/diff/57020/content/public/renderer/re...
File content/public/renderer/renderer_ppapi_host.h (right):

http://codereview.chromium.org/10944005/diff/57020/content/public/renderer/re...
content/public/renderer/renderer_ppapi_host.h:41: // Returns teh
WebPluginContainer for the given plugin instance, or NULL if
s/teh/the

http://codereview.chromium.org/10944005/diff/57020/ppapi/thunk/ppb_websocket_...
File ppapi/thunk/ppb_websocket_thunk.cc (right):

http://codereview.chromium.org/10944005/diff/57020/ppapi/thunk/ppb_websocket_...
ppapi/thunk/ppb_websocket_thunk.cc:147: GetPPB_WebSocket_1_0_Thunk() {
Not sure why this needed to change.

[Takashi Toyoshima, 2012-10-09 01:38:42]

Thanks. I'll push it into CQ!!

http://codereview.chromium.org/10944005/diff/57020/content/public/renderer/re...
File content/public/renderer/renderer_ppapi_host.h (right):

http://codereview.chromium.org/10944005/diff/57020/content/public/renderer/re...
content/public/renderer/renderer_ppapi_host.h:41: // Returns teh
WebPluginContainer for the given plugin instance, or NULL if
On 2012/10/08 22:15:57, bbudge1 wrote:
> s/teh/the

Done.

http://codereview.chromium.org/10944005/diff/57020/ppapi/proxy/websocket_reso...
File ppapi/proxy/websocket_resource_unittest.cc (right):

http://codereview.chromium.org/10944005/diff/57020/ppapi/proxy/websocket_reso...
ppapi/proxy/websocket_resource_unittest.cc:48: }
On 2012/10/08 21:45:46, brettw wrote:
> Nit: need "  // namespace"

Done.

http://codereview.chromium.org/10944005/diff/57020/ppapi/thunk/ppb_websocket_...
File ppapi/thunk/ppb_websocket_thunk.cc (right):

http://codereview.chromium.org/10944005/diff/57020/ppapi/thunk/ppb_websocket_...
ppapi/thunk/ppb_websocket_thunk.cc:147: GetPPB_WebSocket_1_0_Thunk() {
Thanks.
Once I added EXPORT macro on this declaration.
When I reverted it, I mistakenly left the line break here.

[commit-bot: I haz the power, 2012-10-09 01:39:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/10944005/87002

[commit-bot: I haz the power, 2012-10-09 03:43:50]

Change committed as 160783