Skip to content
This repository has been archived by the owner on Apr 3, 2020. It is now read-only.

[Temp] Make unix socket Get{Peer|Local}Address return more rational errors. #223

Merged
merged 1 commit into from
Jan 9, 2015

Conversation

wuhengzhi
Copy link
Contributor

Rather than return ERR_NOT_IMPLEMENTED it seems sensible to
return ERR_SOCKET_NOT_CONNECTED in the case that the socket
is not connected, but unconditionally return ERR_ADDRESS_INVALID
if connected (because there's no IP addr or port associated
with either end of the socket).

BUG=431412

Review URL: https://codereview.chromium.org/841993002

Cr-Commit-Position: refs/heads/master@{#310564}

@crosswalk-trybot
Copy link

Testing patch series with wuhengzhi/chromium-crosswalk@b485241 as its head.

Bot Status
Content Shell Linux [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Content Shell Linux/builds/147)
Content Shell Android-x86 [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Content Shell Android-x86/builds/148)

@wang16
Copy link

wang16 commented Dec 24, 2014

I don't think it's the right way to do this. You need to find out how crosswalk uses UnixDomainSocket and provide crosswalk's implementation, even it's an empty impl.

@wuhengzhi
Copy link
Contributor Author

I have the same opinion, but in the call stack, UnixDomainSocket() was called in net/server/http_server.cc

#0  net::UnixDomainClientSocket::GetPeerAddress (this=0x691ee570, address=0x673f87a0) at ../../net/socket/unix_domain_client_socket_posix.cc:101
#1  0x61a5f0e0 in net::HttpServer::HandleReadResult (this=0x672dfcf0, connection=0x6744c030, rv=1732216736) at ../../net/server/http_server.cc:235
#2  0x61a5f747 in net::HttpServer::OnReadCompleted (this=0x672dfcf0, connection_id=1155, rv=22) at ../../net/server/http_server.cc:195
#3  0x61a5e633 in Run (a1=<optimized out>, a2=<optimized out>, object=<optimized out>, this=<synthetic pointer>) at ../../base/bind_internal.h:248
#4  MakeItSo (a3=@0x673f88ac: 22, a2=@0x680f255c: 1155, weak_ptr=base::WeakPtr((net::HttpServer *)0x672dfcf0), runnable=...) at ../../base/bind_internal.h:938
#5  base::internal::Invoker<2, base::internal::BindState<base::internal::RunnableAdapter<void (net::HttpServer::*)(int, int)>, void (net::HttpServer*, int, int), void (base::WeakPtr<net::HttpServer>, int)>, void (net::HttpServer*, int, int)>::Run(base::internal::BindStateBase*, int const&) (base=0x680f2540, x3=@0x673f88ac: 22) at ../../base/bind_internal.h:1349
#6  0x5fa489af in Run (a1=<optimized out>, this=<optimized out>) at ../../base/callback.h:441
#7  net::SocketLibevent::ReadCompleted (this=0x68190194) at ../../net/socket/socket_libevent.cc:433
#8  0x5fa4955e in net::SocketLibevent::OnFileCanReadWithoutBlocking (this=0x68190150, fd=118) at ../../net/socket/socket_libevent.cc:350
#9  0x5f7ebfaf in base::MessagePumpLibevent::FileDescriptorWatcher::OnFileCanReadWithoutBlocking (this=0x68190178, fd=118, pump=0x672e97e0) at ../../base/message_loop/message_pump_libevent.cc:99
#10 0x5f7ec1a0 in base::MessagePumpLibevent::OnLibeventNotification (fd=118, flags=2, context=0x673f8a68) at ../../base/message_loop/message_pump_libevent.cc:354
#11 0x5f87f723 in event_process_active (base=<optimized out>) at ../../third_party/libevent/event.c:373
#12 event_base_loop (base=0x672e9840, flags=1) at ../../third_party/libevent/event.c:513
#13 0x5f7eaed3 in base::MessagePumpLibevent::Run (this=0x672e97e0, delegate=0x672e9290) at ../../base/message_loop/message_pump_libevent.cc:259
#14 0x5f834db4 in base::MessageLoop::RunHandler (this=0x672e9290) at ../../base/message_loop/message_loop.cc:415
#15 0x5f84db48 in base::RunLoop::Run (this=0x673f8d18) at ../../base/run_loop.cc:55
#16 0x5f832aeb in base::MessageLoop::Run (this=0x672e9290) at ../../base/message_loop/message_loop.cc:308
#17 0x5f867eb9 in base::Thread::Run (this=0x672dfc70, message_loop=0x672e9290) at ../../base/threading/thread.cc:174
#18 0x5f868a34 in base::Thread::ThreadMain (this=0x672dfc70) at ../../base/threading/thread.cc:228
#19 0x5f863275 in base::(anonymous namespace)::ThreadFunc (params=0x65a4c510) at ../../base/threading/platform_thread_posix.cc:80

@wuhengzhi
Copy link
Contributor Author

how crosswalk uses UnixDomainSocket, you mean how upstream? We can extend StreamSocket and implement UnixDomainSocket(), then change the socket in HttpConnection, I am afraid this will affect the other processes.

@wang16
Copy link

wang16 commented Dec 24, 2014

net module always takes UnixDomainSocket as param for sure, but crosswalk could provide XWalkUnixDomainSocket. I remember android webview provides AndroidUnixDomainSocket, we could do similar.

@halton
Copy link

halton commented Dec 24, 2014

This error message is also available on upstream Android build. Why it is trouble for Crosswalk?

@wang16
Copy link

wang16 commented Dec 24, 2014

customer reported that it makes logcat useless.

@halton
Copy link

halton commented Dec 24, 2014

I'd agree this logcat is useless, same pain on me when I try to debug Crosswalk on Android. I think it is low priority and I'd suggest to discuss with upstream for fix. Removing this in our fork is last solution.

@wang16
Copy link

wang16 commented Dec 24, 2014

upstream has derived implementation for chrome and android webview. I don't think they care so much about content shell. We should adopt there approach for chrome/webview.

@halton
Copy link

halton commented Dec 24, 2014

upstream has derived implementation for chrome and android webview. I don't think they care so much about content shell. We should adopt there approach for chrome/webview.

Agreed, that is right solution.

@crosswalk-trybot
Copy link

Testing patch series with wuhengzhi/chromium-crosswalk@6dd62e7 as its head.

Bot Status
Content Shell Android-x86 [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Content Shell Android-x86/builds/153)
Content Shell Linux [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Content Shell Linux/builds/152)

@wuhengzhi wuhengzhi changed the title Remove the macro in the not implemented function. Make unix socket Get{Peer|Local}Address return more rational errors. Jan 9, 2015
@halton
Copy link

halton commented Jan 9, 2015

lgtm with nit, use [Temp] prefix in git log to indicate rebase owner to remove it in future.

@wang16
Copy link

wang16 commented Jan 9, 2015

LGTM after fix halton's comment, thanks.

@wuhengzhi
Copy link
Contributor Author

I will add it, thanks.

@crosswalk-trybot
Copy link

Testing patch series with wuhengzhi/chromium-crosswalk@44a8a9c as its head.

Bot Status
Content Shell Linux [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Content Shell Linux/builds/153)
Content Shell Android-x86 [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Content Shell Android-x86/builds/154)

@wuhengzhi wuhengzhi changed the title Make unix socket Get{Peer|Local}Address return more rational errors. [Temp] Make unix socket Get{Peer|Local}Address return more rational errors. Jan 9, 2015
…rrors.

Rather than return ERR_NOT_IMPLEMENTED it seems sensible to
return ERR_SOCKET_NOT_CONNECTED in the case that the socket
is not connected, but unconditionally return ERR_ADDRESS_INVALID
if connected (because there's no IP addr or port associated
with either end of the socket).

BUG=431412

Review URL: https://codereview.chromium.org/841993002

Cr-Commit-Position: refs/heads/master@{#310564}
rakuco added a commit that referenced this pull request Jan 9, 2015
[Temp] Make unix socket Get{Peer|Local}Address return more rational errors.
@rakuco rakuco merged commit 13ee6cc into crosswalk-project:master Jan 9, 2015
@rakuco
Copy link
Member

rakuco commented Jan 9, 2015

Make sure to roll DEPS.xwalk, and possibly backport this to crosswalk-11.

mrunalk pushed a commit that referenced this pull request Jan 27, 2015
Having a ContextWrapper passed in as the AwContents Context parameter
was causing us to use the wrong kind of window object which in turn
resulted in popup dialogs not being displayed.

BUG=434695

Review URL: https://codereview.chromium.org/756983002
[email protected]

Cr-Commit-Position: refs/heads/master@{#305796}
(cherry picked from commit 214620e)

Review URL: https://codereview.chromium.org/778183002

Cr-Commit-Position: refs/branch-heads/2214@{#223}
Cr-Branched-From: 03655fd-refs/heads/master@{#303346}
rakuco pushed a commit that referenced this pull request Feb 27, 2015
SingleThreadProxy was previously not considering
DidFailToInitializeOutputSurface responses as an outstanding
RequestNewOutputSurface call. This would cause the embedder to start
servicing an output surface request, then composite, receive another,
and then have double output surface requests.

The fix is to consider this failure state as a request.

[email protected],[email protected],[email protected]
BUG=444277

Review URL: https://codereview.chromium.org/871743002

Cr-Commit-Position: refs/heads/master@{#313357}
(cherry picked from commit 5232fbb)

Review URL: https://codereview.chromium.org/896423003

Cr-Commit-Position: refs/branch-heads/2272@{#223}
Cr-Branched-From: 827a380-refs/heads/master@{#310958}
rakuco pushed a commit that referenced this pull request Apr 30, 2015
…n't called.

With ephemeral profiles, in some cases, a Handle will be created and destroyed
without being initialized. Root cause of why the object is created is still not
known, but this change prevents the crash from occurring.

BUG=478215

Review URL: https://codereview.chromium.org/1072143003

Cr-Commit-Position: refs/heads/master@{#326371}
(cherry picked from commit 08f40f1)

Review URL: https://codereview.chromium.org/1087903003

Cr-Commit-Position: refs/branch-heads/2357@{#223}
Cr-Branched-From: 59d4494-refs/heads/master@{#323860}
rakuco pushed a commit that referenced this pull request Aug 13, 2015
… archives.

BUG=515216

Review URL: https://codereview.chromium.org/1262753002

Cr-Commit-Position: refs/heads/master@{#341475}
(cherry picked from commit 04a4eb9)

[email protected]

Review URL: https://codereview.chromium.org/1270823003 .

Cr-Commit-Position: refs/branch-heads/2454@{#223}
Cr-Branched-From: 12bfc33-refs/heads/master@{#338390}
mrunalk pushed a commit that referenced this pull request Sep 22, 2015
While we work on to resolve these known StrictMode
violations, it's a poor experience for dogfooders to see
these every time they are triggered. It is still
important for developers and dogfooders to see and report
newly introduced violations with the StrictMode red border
flash.

BUG=508615,525781,525785,527415,527429
NOTRY=true
NOPRESUBMIT=true

Review URL: https://codereview.chromium.org/1334533004

Cr-Commit-Position: refs/heads/master@{#348147}
(cherry picked from commit b6f6133)

[email protected]

Review URL: https://codereview.chromium.org/1336603002

Cr-Commit-Position: refs/branch-heads/2490@{#223}
Cr-Branched-From: 7790a35-refs/heads/master@{#344925}
mrunalk pushed a commit that referenced this pull request Dec 18, 2015
BUG=560752

Review URL: https://codereview.chromium.org/1489873004

Cr-Commit-Position: refs/heads/master@{#362857}
(cherry picked from commit 77c735a)

Review URL: https://codereview.chromium.org/1491373006 .

Cr-Commit-Position: refs/branch-heads/2564@{#223}
Cr-Branched-From: 1283eca-refs/heads/master@{#359700}
Bysmyyr pushed a commit to Bysmyyr/chromium-crosswalk that referenced this pull request Feb 10, 2016
BUG=580211

Review URL: https://codereview.chromium.org/1619273003

Cr-Commit-Position: refs/heads/master@{#371073}
(cherry picked from commit 38c7689)

Conflicts:
	components/startup_metric_utils.gypi
	components/startup_metric_utils/browser/BUILD.gn
	components/startup_metric_utils/browser/DEPS

[email protected]
[email protected], [email protected]

Review URL: https://codereview.chromium.org/1652083003 .

Cr-Commit-Position: refs/branch-heads/2623@{crosswalk-project#223}
Cr-Branched-From: 92d7753-refs/heads/master@{#369907}
(cherry picked from commit 1ab633f)

Conflicts:
	components/startup_metric_utils.gypi
	components/startup_metric_utils/browser/BUILD.gn
	components/startup_metric_utils/browser/DEPS
	components/startup_metric_utils/browser/startup_metric_utils.cc
	tools/metrics/histograms/histograms.xml

Review URL: https://codereview.chromium.org/1663273002 .

Cr-Commit-Position: refs/branch-heads/2564@{#666}
Cr-Branched-From: 1283eca-refs/heads/master@{#359700}
rakuco pushed a commit that referenced this pull request Mar 3, 2016
BUG=580211

Review URL: https://codereview.chromium.org/1619273003

Cr-Commit-Position: refs/heads/master@{#371073}
(cherry picked from commit 38c7689)

Conflicts:
	components/startup_metric_utils.gypi
	components/startup_metric_utils/browser/BUILD.gn
	components/startup_metric_utils/browser/DEPS

[email protected]

Review URL: https://codereview.chromium.org/1652083003 .

Cr-Commit-Position: refs/branch-heads/2623@{#223}
Cr-Branched-From: 92d7753-refs/heads/master@{#369907}
huningxin pushed a commit to huningxin/chromium-croswalk that referenced this pull request May 17, 2016
Cr-Commit-Position: refs/branch-heads/2661@{crosswalk-project#223}
Cr-Branched-From: ef6f6ae-refs/heads/master@{#378081}
huningxin pushed a commit to huningxin/chromium-croswalk that referenced this pull request Oct 9, 2016
Cr-Commit-Position: refs/branch-heads/2743@{crosswalk-project#223}
Cr-Branched-From: 2b3ae3b-refs/heads/master@{#394939}
imreotto pushed a commit to tenta-browser/chromium-crosswalk that referenced this pull request Nov 2, 2017
The long namespace paths of SingleLogFileLogSource::SupportedSource and
SingleDebugDaemonLogSource::SupportedSource are hard to read. Create
aliases to make the switch block cases easier to read.

BUG=762965
R=​[email protected]

Change-Id: I100003d5f286f8b2ee71bc4555e6700b2c0404c3
Reviewed-on: https://chromium-review.googlesource.com/656099
Reviewed-by: Ahmed Fakhry <[email protected]>
Commit-Queue: Simon Que <[email protected]>
Cr-Original-Commit-Position: refs/heads/master@{#500997}(cherry picked from commit 9639526)
Reviewed-on: https://chromium-review.googlesource.com/662379
Cr-Commit-Position: refs/branch-heads/3202@{crosswalk-project#223}
Cr-Branched-From: fa6a5d8-refs/heads/master@{#499098}
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants