Websocket should fire 'error' event if no server available

Websocket should fire 'error' event if no server available
Implement OnError virtual function, when network is down, OnError can be invoked.
Send error code to render process from browser process through IPC.

This patch depends on Blink side patch which can be found from https://codereview.chromium.org/14071008/ 


BUG=128057
TEST=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=196728

R=avi@chromium.org, jln@chromium.org, kinuko@chromium.org, tyoshino@chromium.org, yutak@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=197854

[Li Yin, 2012-06-25 06:41:39]

When the network is down, chromium will trigger OnError callback before OnClose
callback.
That is to say, if user doesn't send close frame or receive close frame before
network is down, WebSocket object will fire error event.

[Yuta Kitamura, 2012-07-13 05:44:27]

LGTM (you still need LGTMs from other OWNERs of each module)

Please keep in mind that you don't have to open a new code review issue to
upload your revised patch. I'm not sure how you opened extra code review issues,
but if you just revise your patch and upload it by git-cl or gcl, these tools
would do the right thing for you.

Please read the documentation for more details about these tools:
<http://dev.chromium.org/developers/contributing-code>

Please close the old code review issues to not confuse other reviewers.

[Li Yin, 2012-07-16 05:54:04]

On 2012/07/13 05:44:27, Yuta Kitamura wrote:
> LGTM (you still need LGTMs from other OWNERs of each module)
> 
> Please keep in mind that you don't have to open a new code review issue to
> upload your revised patch. I'm not sure how you opened extra code review
issues,
> but if you just revise your patch and upload it by git-cl or gcl, these tools
> would do the right thing for you.
> 
> Please read the documentation for more details about these tools:
> <http://dev.chromium.org/developers/contributing-code>
> 
> Please close the old code review issues to not confuse other reviewers.

Thanks for your reminding.
I used a new branch to upload patch. 
And I have closed other related issues.

[Li Yin, 2013-04-15 03:01:39]

On 2012/07/13 05:44:27, Yuta Kitamura wrote:
> LGTM (you still need LGTMs from other OWNERs of each module)

Hi Ben,
Could you give some comments? It seems you are the suggested owner for
socket_stream_messages.h.

[tyoshino (SeeGerritForStatus), 2013-04-15 03:44:58]

On 2013/04/15 03:01:39, Li Yin wrote:
> On 2012/07/13 05:44:27, Yuta Kitamura wrote:
> > LGTM (you still need LGTMs from other OWNERs of each module)
> 
> Hi Ben,
> Could you give some comments? It seems you are the suggested owner for
> socket_stream_messages.h.

Hi Li,

I'll take a look and then add appropriate OWNER reviewers for each directory.

Thanks

[tyoshino (SeeGerritForStatus), 2013-04-15 14:23:28]

https://codereview.chromium.org/10668018/diff/5001/content/browser/renderer_h...
File content/browser/renderer_host/socket_stream_dispatcher_host.cc (right):

https://codereview.chromium.org/10668018/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/socket_stream_dispatcher_host.cc:111: }
Not to surprise reader, please write some short comment explaining that
SocketStream::Delegate::OnError() events are handled as the _Fail the ... event
in the protocol spec and this RPC message means that.

https://codereview.chromium.org/10668018/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/socket_stream_dispatcher_host.cc:113: LOG(ERROR)
<< "SocketStreamMsg_Failed failed.";
Add
DeleteSocketStreamHost(socket_id);
?

https://codereview.chromium.org/10668018/diff/5001/content/common/socket_stre...
File content/common/socket_stream_dispatcher.cc (right):

https://codereview.chromium.org/10668018/diff/5001/content/common/socket_stre...
content/common/socket_stream_dispatcher.cc:168: DVLOG(1) <<
"IPCWebSocketStreamHandleBridge::OnFailed";
DVLOG(1) << "Bridge #" << socket_id_ << " OnFailed (error_code=" << error_code
<< ")";

https://codereview.chromium.org/10668018/diff/5001/content/common/socket_stre...
content/common/socket_stream_dispatcher.cc:169: if (delegate_) {
remove braces for consistency with the other methods.

https://codereview.chromium.org/10668018/diff/5001/content/common/socket_stre...
content/common/socket_stream_dispatcher.cc:283: DLOG(ERROR) << "No
SocketStreamHandleBridge for socket_id=" << socket_id;
s/SocketStreamHandleBridge/bridge/ for consistency with the other methods.

https://codereview.chromium.org/10668018/diff/5001/content/common/socket_stre...
content/common/socket_stream_dispatcher.cc:284: }
put one blank line here

https://codereview.chromium.org/10668018/diff/5001/webkit/glue/websocketstrea...
File webkit/glue/websocketstreamhandle_impl.cc (right):

https://codereview.chromium.org/10668018/diff/5001/webkit/glue/websocketstrea...
webkit/glue/websocketstreamhandle_impl.cc:27: using WebKit::WebString;
string16?

https://codereview.chromium.org/10668018/diff/5001/webkit/tools/test_shell/si...
File webkit/tools/test_shell/simple_socket_stream_bridge.cc (right):

https://codereview.chromium.org/10668018/diff/5001/webkit/tools/test_shell/si...
webkit/tools/test_shell/simple_socket_stream_bridge.cc:162: int error_code) {
(optional) L161 and 162 can fit in one line

https://codereview.chromium.org/10668018/diff/5001/webkit/tools/test_shell/si...
webkit/tools/test_shell/simple_socket_stream_bridge.cc:235: const char*
error_msg) {
optional) L234 and 235 can fit in one line

[Li Yin, 2013-04-16 12:07:37]

https://codereview.chromium.org/10668018/diff/5001/content/browser/renderer_h...
File content/browser/renderer_host/socket_stream_dispatcher_host.cc (right):

https://codereview.chromium.org/10668018/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/socket_stream_dispatcher_host.cc:111: }
On 2013/04/15 14:23:28, tyoshino wrote:
> Not to surprise reader, please write some short comment explaining that
> SocketStream::Delegate::OnError() events are handled as the _Fail the ...
event
> in the protocol spec and this RPC message means that.

There is an issue here. OnError is almost called together with Onclose, it is so
senstive.
Maybe we should add some filter to avoid trigger onerror event every time.
What's your thought?

https://codereview.chromium.org/10668018/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/socket_stream_dispatcher_host.cc:113: LOG(ERROR)
<< "SocketStreamMsg_Failed failed.";
On 2013/04/15 14:23:28, tyoshino wrote:
> Add
> DeleteSocketStreamHost(socket_id);
> ?
Good catch. I'll fix it. Thanks.

[tyoshino (SeeGerritForStatus), 2013-04-17 08:15:49]

https://codereview.chromium.org/10668018/diff/5001/content/browser/renderer_h...
File content/browser/renderer_host/socket_stream_dispatcher_host.cc (right):

https://codereview.chromium.org/10668018/diff/5001/content/browser/renderer_h...
content/browser/renderer_host/socket_stream_dispatcher_host.cc:111: }
On 2013/04/16 12:07:37, Li Yin wrote:
> On 2013/04/15 14:23:28, tyoshino wrote:
> > Not to surprise reader, please write some short comment explaining that
> > SocketStream::Delegate::OnError() events are handled as the _Fail the ...
> event
> > in the protocol spec and this RPC message means that.
> 
> There is an issue here. OnError is almost called together with Onclose, it is
so
> senstive.
> Maybe we should add some filter to avoid trigger onerror event every time.
> What's your thought?

Oh, right. We need to fix SocketStream. Could you please change
delegate->OnError(this, result) in SocketStream::Finish() to be invoked only
when result != ERR_CONNECTION_CLOSED && result != ERR_PROTOCOL_SWITCHED

https://codereview.chromium.org/10668018/diff/16001/content/common/socket_str...
File content/common/socket_stream_messages.h (right):

https://codereview.chromium.org/10668018/diff/16001/content/common/socket_str...
content/common/socket_stream_messages.h:80: int /* error code */)
error_code

https://codereview.chromium.org/10668018/diff/16001/webkit/glue/websocketstre...
File webkit/glue/websocketstreamhandle_delegate.h (right):

https://codereview.chromium.org/10668018/diff/16001/webkit/glue/websocketstre...
webkit/glue/websocketstreamhandle_delegate.h:7: 
include base/string16.h

https://codereview.chromium.org/10668018/diff/16001/webkit/glue/websocketstre...
File webkit/glue/websocketstreamhandle_impl.cc (right):

https://codereview.chromium.org/10668018/diff/16001/webkit/glue/websocketstre...
webkit/glue/websocketstreamhandle_impl.cc:14: #include
"base/memory/scoped_ptr.h"
include base/string16.h

[Li Yin, 2013-04-23 05:22:59]

Hi tyoshino,
All the reviewed comments have been fixed already, do you thinks it is ready for
landing?
And it seems content/common/socket_stream_messages.h still needs OWNER's
agreement.

[tyoshino (SeeGerritForStatus), 2013-04-23 09:06:33]

LGTM

kinuko, could you please do OWNER review for webkit/?
jam, could you please do OWNER review for content/?

[kinuko, 2013-04-23 10:15:33]

webkit/ lgtm

[kinuko, 2013-04-23 10:15:47]

(leftover nit comment)

https://codereview.chromium.org/10668018/diff/29001/webkit/glue/websocketstre...
File webkit/glue/websocketstreamhandle_impl.cc (right):

https://codereview.chromium.org/10668018/diff/29001/webkit/glue/websocketstre...
webkit/glue/websocketstreamhandle_impl.cc:156: VLOG(1) << "DidFail";
nit: is this useful? maybe it might be better to output error_code/msg if we
have this?

[tyoshino (SeeGerritForStatus), 2013-04-23 16:04:07]

https://codereview.chromium.org/10668018/diff/29001/webkit/glue/websocketstre...
File webkit/glue/websocketstreamhandle_impl.cc (right):

https://codereview.chromium.org/10668018/diff/29001/webkit/glue/websocketstre...
webkit/glue/websocketstreamhandle_impl.cc:156: VLOG(1) << "DidFail";
On 2013/04/23 10:15:47, kinuko wrote:
> nit: is this useful? maybe it might be better to output error_code/msg if we
> have this?

The other VLOGs also should print more info to be useful for debugging. We can
do it in bulk later by a separate CL. I'd suggest that we commit this CL leaving
this VLOG as a placeholder.

[jam, 2013-04-23 17:14:41]

redirecting to Avi

[Avi (use Gerrit), 2013-04-23 17:47:01]

content lgtm

[commit-bot: I haz the power, 2013-04-24 00:40:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/li.yin@intel.com/10668018/29001

[commit-bot: I haz the power, 2013-04-24 00:40:54]

Failed to apply patch for AUTHORS:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file AUTHORS
  Hunk #1 FAILED at 241.
  1 out of 1 hunk FAILED -- saving rejects to file AUTHORS.rej

Patch:       AUTHORS
Index: AUTHORS
diff --git a/AUTHORS b/AUTHORS
index
27ec45782abd88b0f603ac2510afa19a3f8faf89..496b8405a7e71065a522fb36f4b11aea10b95160
100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -241,3 +241,4 @@ Mrunal Kapade <mrunal.kapade@intel.com>
 Yael Aharon <yael.aharon@intel.com>
 Haojian Wu <hokein.wu@gmail.com>
 Sathish Kuppuswamy <sathish.kuppuswamy@intel.com>
+Li Yin <li.yin@intel.com>

[commit-bot: I haz the power, 2013-04-24 01:04:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/li.yin@intel.com/10668018/42001

[commit-bot: I haz the power, 2013-04-24 01:04:34]

Presubmit check for 10668018-42001 failed and returned exit status 1.

INFO:root:Found 12 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/net/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    content/common/socket_stream_messages.h

Presubmit checks took 1.7s to calculate.

[tyoshino (SeeGerritForStatus), 2013-04-24 02:05:33]

+tsepez

Hi tsepez,

Could you please take a look at the change on
content/common/socket_stream_messages.h ?

The new IPC message SocketStreamMsg_Failed tells the render that the TCP
connection for the corresponding SocketStream is closed unexpectedly.

[tyoshino (SeeGerritForStatus), 2013-04-24 02:09:06]

Oops. I added cevans@ but was talking to tsepez@ in the mail.

Hi cevans,

Could you please take a look at the change on
content/common/socket_stream_messages.h ?

The new IPC message SocketStreamMsg_Failed tells the render that the TCP
connection for the corresponding SocketStream is closed unexpectedly.

[Li Yin, 2013-04-26 00:43:56]

+jln, Hi Julien, could you please review
content/common/socket_stream_messages.h?
Thanks in advance.

[jln (very slow on Chromium), 2013-04-26 01:03:22]

socket_stream_messages.h lgtm

[commit-bot: I haz the power, 2013-04-26 01:24:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/li.yin@intel.com/10668018/42001

[commit-bot: I haz the power, 2013-04-26 01:24:40]

Failed to apply patch for AUTHORS:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file AUTHORS
  Hunk #1 FAILED at 242.
  1 out of 1 hunk FAILED -- saving rejects to file AUTHORS.rej

Patch:       AUTHORS
Index: AUTHORS
diff --git a/AUTHORS b/AUTHORS
index
ceca7d6eaefd04c0068ff33598c495db29ac6f85..659a9f116268cf2307bde94342fd4726d91cc22a
100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -242,3 +242,4 @@ Yael Aharon <yael.aharon@intel.com>
 Haojian Wu <hokein.wu@gmail.com>
 Sathish Kuppuswamy <sathish.kuppuswamy@intel.com>
 Joone Hur <joone.hur@intel.com>
+Li Yin <li.yin@intel.com>

[commit-bot: I haz the power, 2013-04-26 01:29:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/li.yin@intel.com/10668018/52001

[commit-bot: I haz the power, 2013-04-26 02:07:40]

Retried try job too often on linux_rel for step(s) net_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[tyoshino (SeeGerritForStatus), 2013-04-26 05:44:16]

Checked results. Looks ok to commit. I'll commit it manually.

[Li Yin, 2013-04-26 05:49:52]

On 2013/04/26 05:44:16, tyoshino wrote:
> Checked results. Looks ok to commit. I'll commit it manually.

Please wait.
Some net tests need to fix because SocketStream's behavior changed.

[tyoshino (SeeGerritForStatus), 2013-04-26 06:05:48]

On 2013/04/26 05:49:52, Li Yin wrote:
> On 2013/04/26 05:44:16, tyoshino wrote:
> > Checked results. Looks ok to commit. I'll commit it manually.
> 
> Please wait.
> Some net tests need to fix because SocketStream's behavior changed.

oh, yes. i'll wait for fix.

[Li Yin, 2013-04-26 06:57:18]

https://codereview.chromium.org/10668018/diff/66002/net/socket_stream/socket_...
File net/socket_stream/socket_stream.cc (right):

https://codereview.chromium.org/10668018/diff/66002/net/socket_stream/socket_...
net/socket_stream/socket_stream.cc:362: if (result != ERR_CONNECTION_CLOSED &&
result != ERR_PROTOCOL_SWITCHED)
I am unfamiliar with SwitchToSpdy related code, and don't know if this fix is
reasonable. Could you please have a look how to change TEST_F(SocketStreamTest,
SwitchToSpdy) and TEST_F(SocketStreamTest, SwitchAfterPending)?
Thanks.

[tyoshino (SeeGerritForStatus), 2013-04-26 07:19:43]

https://codereview.chromium.org/10668018/diff/66002/net/socket_stream/socket_...
File net/socket_stream/socket_stream.cc (right):

https://codereview.chromium.org/10668018/diff/66002/net/socket_stream/socket_...
net/socket_stream/socket_stream.cc:362: if (result != ERR_CONNECTION_CLOSED &&
result != ERR_PROTOCOL_SWITCHED)
On 2013/04/26 06:57:18, Li Yin wrote:
> I am unfamiliar with SwitchToSpdy related code, and don't know if this fix is
> reasonable. Could you please have a look how to change
TEST_F(SocketStreamTest,
> SwitchToSpdy) and TEST_F(SocketStreamTest, SwitchAfterPending)?
> Thanks.

Yes. I'm looking here too. Raising ERR_PROTOCOL_SWITCHED is fine for
WebSocketJob. Sorry. You can remove result != ERR_PROTOCOL_SWITCHED from here.

But even with that, we still need to fix some part in the unittest.

As we've changed Blink side not to deref on OnError, it makes sense to change
the recorder to call completion callback on OnClose call instead. Then, we need
to change L364 too. I.e. removing if-clause.

But then some of Spdy tests fail. I'm looking

[Li Yin, 2013-04-26 07:57:52]

On 2013/04/26 07:19:43, tyoshino wrote:
> Yes. I'm looking here too. Raising ERR_PROTOCOL_SWITCHED is fine for
> WebSocketJob. Sorry. You can remove result != ERR_PROTOCOL_SWITCHED from here.
> 
> But even with that, we still need to fix some part in the unittest.
> 
> As we've changed Blink side not to deref on OnError, it makes sense to change
> the recorder to call completion callback on OnClose call instead. Then, we
need
> to change L364 too. I.e. removing if-clause.
> 
> But then some of Spdy tests fail. I'm looking

I keep Spdy related code unchanged. And only change WebSocket related code and
unit tests.
If there is indeed a bug about Spdy, let's trigger it on the followed patch.
Do you think it is okay?

[Li Yin, 2013-04-26 08:00:12]

On 2013/04/26 07:57:52, Li Yin wrote:
> If there is indeed a bug about Spdy, let's trigger it on the followed patch.

Sorry, let's fix it on the followed patch.

[tyoshino (SeeGerritForStatus), 2013-04-26 08:34:05]

On 2013/04/26 08:00:12, Li Yin wrote:
> On 2013/04/26 07:57:52, Li Yin wrote:
> > If there is indeed a bug about Spdy, let's trigger it on the followed patch.
> 
> Sorry, let's fix it on the followed patch.

I made fix but needs quick review by WebSocket/SPDY expert. I'm going to ask
Takashi for this.

[tyoshino (SeeGerritForStatus), 2013-04-26 08:58:43]

Li-san,

Sorry but please upload patch set 10 again? We can go with that.

[commit-bot: I haz the power, 2013-04-26 09:51:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/li.yin@intel.com/10668018/66017

[commit-bot: I haz the power, 2013-04-26 14:31:19]

Change committed as 196728

[ojan, 2013-04-26 22:21:21]

This patch looks like it made a layout test flaky. Reverting.

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showLarge...

[Li Yin, 2013-04-27 01:30:10]

On 2013/04/26 22:21:21, ojan wrote:
> This patch looks like it made a layout test flaky. Reverting.
> 
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showLarge...

Sorry for that.
It seems it is timing and platform related issue.
It is always passed on my developing machine(Ubuntu 64-bit Debug mode).
Anyway, I will have a look.

[commit-bot: I haz the power, 2013-05-01 03:23:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/li.yin@intel.com/10668018/81001

[Li Yin, 2013-05-01 03:27:17]

On 2013/04/27 01:30:10, Li Yin wrote:
> On 2013/04/26 22:21:21, ojan wrote:
> > This patch looks like it made a layout test flaky. Reverting.
> > 
> >
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showLarge...
> 
> Sorry for that.
> It seems it is timing and platform related issue.
> It is always passed on my developing machine(Ubuntu 64-bit Debug mode).
> Anyway, I will have a look.

The patch from blink should fix the flaky tests issue.
Let me tey commit-bot again.

[commit-bot: I haz the power, 2013-05-01 03:52:22]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
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, 2013-05-01 05:54:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/li.yin@intel.com/10668018/81001

[commit-bot: I haz the power, 2013-05-01 08:12:17]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[Li Yin, 2013-05-01 08:44:08]

On 2013/05/01 08:12:17, I haz the power (commit-bot) wrote:
> Retried try job too often on win7_aura for step(s) browser_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

According to the test result, the patch should be OK to commit.

Hi tyoshino,
Could you please help to commit it manually? Thanks in advance.

[commit-bot: I haz the power, 2013-05-02 02:11:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/li.yin@intel.com/10668018/81001

[commit-bot: I haz the power, 2013-05-02 04:09:20]

Retried try job too often on win7_aura for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[tyoshino (SeeGerritForStatus), 2013-05-02 07:38:41]

Committed patchset #16 manually as r197854 (presubmit successful).

[tyoshino (SeeGerritForStatus), 2013-05-02 07:40:21]

Ran webkit tests, net_unittests and relanded.