New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 865958 link

Starred by 1 user
Status: Fixed
Owner:
█ xunji...@chromium.org
Last visit > 30 days ago
Closed: Jul 22
Cc:
jam@chromium.org
reillyg@chromium.org
morlovich@chromium.org
dxie@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug
Proj-Servicification



Sign in to add a comment

Migrating extensions sockets TCP API to mojo caused an extensions to break

Project Member Reported by xunji...@chromium.org, Jul 20 
For details, see internal bug: b/110354086.

Looks like the migration work done in "Convert Extensions TCP & TLS Socket APIs to mojo sockets" (https://chromium-review.googlesource.com/c/chromium/src/+/1070434) introduced a behavior change and caused a breakage in an internal Chrome app that uses the extensions TCP socket APIs.

M69 just branched yesterday (July 19). I think it's safer to revert, merge the revert to M69. Investigate and reland the CL in M70.

Comment 1 by xunji...@chromium.org, Jul 20

Cc: reillyg@chromium.org jam@chromium.org morlovich@chromium.org
ccing my CL reviewers as a fyi.
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 20 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/494108abe23d689c0a311025d464edc7453d368b

commit 494108abe23d689c0a311025d464edc7453d368b
Author: Helen Li <xunjieli@chromium.org>
Date: Fri Jul 20 21:00:31 2018

Revert "Convert Extensions TCP & TLS Socket APIs to mojo sockets"

This reverts commit 41066e83216ea19036dcf33cfbf35e18f77da9fe.

Reason for revert: This CL broke an internal app.  See  crbug.com/865958 

Original change's description:
> Convert Extensions TCP & TLS Socket APIs to mojo sockets
>
> This CL converts extensions TCP and TLS socket apis to network service's mojo
> socket.
>
> - Remove combined_socket_unittest.cc because the test cases no longer apply. I
> tried to write equivalent ones in tcp_socket_unittest.cc and
> tls_socket_unittest.cc
> - Add a ContentUtilityClient implementation for extensions_browsertests, so we
> can inject a NetworkServiceTestHelper to the utility process in which network
> service runs.
>
> Bug:  721401 
> Cq-Include-Trybots: luci.chromium.try:linux_chromium_dbg_ng;master.tryserver.chromium.linux:linux_mojo
> Change-Id: I333a3021a5c66eb94618be6830ba0ff68c442eba
> Reviewed-on: https://chromium-review.googlesource.com/1070434
> Commit-Queue: Helen Li <xunjieli@chromium.org>
> Reviewed-by: Maks Orlovich <morlovich@chromium.org>
> Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> Reviewed-by: Reilly Grant <reillyg@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#563390}

TBR=jam@chromium.org,reillyg@chromium.org,xunjieli@chromium.org,morlovich@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  721401 ,  865958 
Change-Id: I9e9408f31887ab45d0bd967e2f17f91c465836c6
Cq-Include-Trybots: luci.chromium.try:linux_chromium_dbg_ng;luci.chromium.try:linux_mojo
Reviewed-on: https://chromium-review.googlesource.com/1145340
Commit-Queue: Helen Li <xunjieli@chromium.org>
Reviewed-by: Helen Li <xunjieli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576982}
[modify] https://crrev.com/494108abe23d689c0a311025d464edc7453d368b/build/check_gn_headers_whitelist.txt
[add] https://crrev.com/494108abe23d689c0a311025d464edc7453d368b/chrome/browser/extensions/api/socket/combined_socket_unittest.cc
[add] https://crrev.com/494108abe23d689c0a311025d464edc7453d368b/chrome/browser/extensions/api/socket/mock_tcp_client_socket.h
[modify] https://crrev.com/494108abe23d689c0a311025d464edc7453d368b/chrome/browser/extensions/api/socket/tcp_socket_unittest.cc
[modify] https://crrev.com/494108abe23d689c0a311025d464edc7453d368b/chrome/browser/extensions/api/socket/tls_socket_unittest.cc
[modify] https://crrev.com/494108abe23d689c0a311025d464edc7453d368b/chrome/test/BUILD.gn
[modify] https://crrev.com/494108abe23d689c0a311025d464edc7453d368b/chrome/test/data/extensions/api_test/socket/api/background.js
[modify] https://crrev.com/494108abe23d689c0a311025d464edc7453d368b/extensions/browser/api/socket/BUILD.gn
[delete] https://crrev.com/756d07e6ffaab2d19b2a7fa478090727518ba3ab/extensions/browser/api/socket/mojo_data_pump.cc
[delete] https://crrev.com/756d07e6ffaab2d19b2a7fa478090727518ba3ab/extensions/browser/api/socket/mojo_data_pump.h
[modify] https://crrev.com/494108abe23d689c0a311025d464edc7453d368b/extensions/browser/api/socket/socket.cc
[modify] https://crrev.com/494108abe23d689c0a311025d464edc7453d368b/extensions/browser/api/socket/socket.h
[modify] https://crrev.com/494108abe23d689c0a311025d464edc7453d368b/extensions/browser/api/socket/socket_api.cc
[modify] https://crrev.com/494108abe23d689c0a311025d464edc7453d368b/extensions/browser/api/socket/socket_api.h
[modify] https://crrev.com/494108abe23d689c0a311025d464edc7453d368b/extensions/browser/api/socket/tcp_socket.cc
[modify] https://crrev.com/494108abe23d689c0a311025d464edc7453d368b/extensions/browser/api/socket/tcp_socket.h
[modify] https://crrev.com/494108abe23d689c0a311025d464edc7453d368b/extensions/browser/api/socket/tls_socket.cc
[modify] https://crrev.com/494108abe23d689c0a311025d464edc7453d368b/extensions/browser/api/socket/tls_socket.h
[modify] https://crrev.com/494108abe23d689c0a311025d464edc7453d368b/extensions/browser/api/socket/udp_socket.cc
[modify] https://crrev.com/494108abe23d689c0a311025d464edc7453d368b/extensions/browser/api/socket/udp_socket.h
[modify] https://crrev.com/494108abe23d689c0a311025d464edc7453d368b/extensions/browser/api/sockets_tcp/sockets_tcp_api.cc
[modify] https://crrev.com/494108abe23d689c0a311025d464edc7453d368b/extensions/browser/api/sockets_tcp/sockets_tcp_api.h
[modify] https://crrev.com/494108abe23d689c0a311025d464edc7453d368b/extensions/browser/api/sockets_tcp/sockets_tcp_apitest.cc
[modify] https://crrev.com/494108abe23d689c0a311025d464edc7453d368b/extensions/browser/api/sockets_tcp_server/sockets_tcp_server_api.cc
[modify] https://crrev.com/494108abe23d689c0a311025d464edc7453d368b/extensions/browser/api/sockets_tcp_server/sockets_tcp_server_api.h
[modify] https://crrev.com/494108abe23d689c0a311025d464edc7453d368b/extensions/browser/api/sockets_tcp_server/tcp_server_socket_event_dispatcher.cc
[modify] https://crrev.com/494108abe23d689c0a311025d464edc7453d368b/extensions/browser/api/sockets_tcp_server/tcp_server_socket_event_dispatcher.h

Comment 3 by xunji...@chromium.org, Jul 22

Labels: Merge-Request-69 OS-Chrome OS-Linux OS-Mac OS-Windows
Status: Fixed (was: Assigned)
Project Member

Comment 4 by sheriffbot@chromium.org, Jul 22

Labels: -Merge-Request-69 Merge-Review-69 Hotlist-Merge-Review
This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 5 by gov...@chromium.org, Jul 23

Cc: dxie@chromium.org
Before we approve merge to M69, could you pls comment how safe is the revert listed at #2 to merge to M69?

Comment 6 by xunji...@chromium.org, Jul 23 

This CL is safe to merge into M69. A refactoring caused a problem in client app, and we are still investigating. 

We are reverting to the stage before the refactoring. Before the refactoring, the code has been stable for a couple of releases.

Comment 7 by gov...@chromium.org, Jul 23

Labels: -Merge-Review-69 Merge-Approved-69
Approving merge to M69 branch 3497 based on comment #6. Please merge ASAP. Thank you.
Project Member

Comment 8 by bugdroid1@chromium.org, Jul 23

Labels: -merge-approved-69 merge-merged-3497
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/414ca59262e3866e8ad28cd4d00f37536ff5868b

commit 414ca59262e3866e8ad28cd4d00f37536ff5868b
Author: Helen Li <xunjieli@chromium.org>
Date: Mon Jul 23 19:49:41 2018

Revert "Convert Extensions TCP & TLS Socket APIs to mojo sockets"

This reverts commit 41066e83216ea19036dcf33cfbf35e18f77da9fe.

Reason for revert: This CL broke an internal app.  See  crbug.com/865958 

Original change's description:
> Convert Extensions TCP & TLS Socket APIs to mojo sockets
>
> This CL converts extensions TCP and TLS socket apis to network service's mojo
> socket.
>
> - Remove combined_socket_unittest.cc because the test cases no longer apply. I
> tried to write equivalent ones in tcp_socket_unittest.cc and
> tls_socket_unittest.cc
> - Add a ContentUtilityClient implementation for extensions_browsertests, so we
> can inject a NetworkServiceTestHelper to the utility process in which network
> service runs.
>
> Bug:  721401 
> Cq-Include-Trybots: luci.chromium.try:linux_chromium_dbg_ng;master.tryserver.chromium.linux:linux_mojo
> Change-Id: I333a3021a5c66eb94618be6830ba0ff68c442eba
> Reviewed-on: https://chromium-review.googlesource.com/1070434
> Commit-Queue: Helen Li <xunjieli@chromium.org>
> Reviewed-by: Maks Orlovich <morlovich@chromium.org>
> Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> Reviewed-by: Reilly Grant <reillyg@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#563390}

TBR=jam@chromium.org,reillyg@chromium.org,xunjieli@chromium.org,morlovich@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  721401 ,  865958 
Change-Id: I9e9408f31887ab45d0bd967e2f17f91c465836c6
Cq-Include-Trybots: luci.chromium.try:linux_chromium_dbg_ng;luci.chromium.try:linux_mojo
Reviewed-on: https://chromium-review.googlesource.com/1145340
Commit-Queue: Helen Li <xunjieli@chromium.org>
Reviewed-by: Helen Li <xunjieli@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#576982}(cherry picked from commit 494108abe23d689c0a311025d464edc7453d368b)
Reviewed-on: https://chromium-review.googlesource.com/1147182
Cr-Commit-Position: refs/branch-heads/3497@{#23}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/414ca59262e3866e8ad28cd4d00f37536ff5868b/build/check_gn_headers_whitelist.txt
[add] https://crrev.com/414ca59262e3866e8ad28cd4d00f37536ff5868b/chrome/browser/extensions/api/socket/combined_socket_unittest.cc
[add] https://crrev.com/414ca59262e3866e8ad28cd4d00f37536ff5868b/chrome/browser/extensions/api/socket/mock_tcp_client_socket.h
[modify] https://crrev.com/414ca59262e3866e8ad28cd4d00f37536ff5868b/chrome/browser/extensions/api/socket/tcp_socket_unittest.cc
[modify] https://crrev.com/414ca59262e3866e8ad28cd4d00f37536ff5868b/chrome/browser/extensions/api/socket/tls_socket_unittest.cc
[modify] https://crrev.com/414ca59262e3866e8ad28cd4d00f37536ff5868b/chrome/test/BUILD.gn
[modify] https://crrev.com/414ca59262e3866e8ad28cd4d00f37536ff5868b/chrome/test/data/extensions/api_test/socket/api/background.js
[modify] https://crrev.com/414ca59262e3866e8ad28cd4d00f37536ff5868b/extensions/browser/api/socket/BUILD.gn
[delete] https://crrev.com/1b5dd7ce82f97dfd81f07649fff0c7dc76c8e6d6/extensions/browser/api/socket/mojo_data_pump.cc
[delete] https://crrev.com/1b5dd7ce82f97dfd81f07649fff0c7dc76c8e6d6/extensions/browser/api/socket/mojo_data_pump.h
[modify] https://crrev.com/414ca59262e3866e8ad28cd4d00f37536ff5868b/extensions/browser/api/socket/socket.cc
[modify] https://crrev.com/414ca59262e3866e8ad28cd4d00f37536ff5868b/extensions/browser/api/socket/socket.h
[modify] https://crrev.com/414ca59262e3866e8ad28cd4d00f37536ff5868b/extensions/browser/api/socket/socket_api.cc
[modify] https://crrev.com/414ca59262e3866e8ad28cd4d00f37536ff5868b/extensions/browser/api/socket/socket_api.h
[modify] https://crrev.com/414ca59262e3866e8ad28cd4d00f37536ff5868b/extensions/browser/api/socket/tcp_socket.cc
[modify] https://crrev.com/414ca59262e3866e8ad28cd4d00f37536ff5868b/extensions/browser/api/socket/tcp_socket.h
[modify] https://crrev.com/414ca59262e3866e8ad28cd4d00f37536ff5868b/extensions/browser/api/socket/tls_socket.cc
[modify] https://crrev.com/414ca59262e3866e8ad28cd4d00f37536ff5868b/extensions/browser/api/socket/tls_socket.h
[modify] https://crrev.com/414ca59262e3866e8ad28cd4d00f37536ff5868b/extensions/browser/api/socket/udp_socket.cc
[modify] https://crrev.com/414ca59262e3866e8ad28cd4d00f37536ff5868b/extensions/browser/api/socket/udp_socket.h
[modify] https://crrev.com/414ca59262e3866e8ad28cd4d00f37536ff5868b/extensions/browser/api/sockets_tcp/sockets_tcp_api.cc
[modify] https://crrev.com/414ca59262e3866e8ad28cd4d00f37536ff5868b/extensions/browser/api/sockets_tcp/sockets_tcp_api.h
[modify] https://crrev.com/414ca59262e3866e8ad28cd4d00f37536ff5868b/extensions/browser/api/sockets_tcp/sockets_tcp_apitest.cc
[modify] https://crrev.com/414ca59262e3866e8ad28cd4d00f37536ff5868b/extensions/browser/api/sockets_tcp_server/sockets_tcp_server_api.cc
[modify] https://crrev.com/414ca59262e3866e8ad28cd4d00f37536ff5868b/extensions/browser/api/sockets_tcp_server/sockets_tcp_server_api.h
[modify] https://crrev.com/414ca59262e3866e8ad28cd4d00f37536ff5868b/extensions/browser/api/sockets_tcp_server/tcp_server_socket_event_dispatcher.cc
[modify] https://crrev.com/414ca59262e3866e8ad28cd4d00f37536ff5868b/extensions/browser/api/sockets_tcp_server/tcp_server_socket_event_dispatcher.h
Project Member

Comment 9 by bugdroid1@chromium.org, Jul 24 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bff8e3ea6d0d7c1f1fe045f595ffaeaf64ff41f9

commit bff8e3ea6d0d7c1f1fe045f595ffaeaf64ff41f9
Author: Helen Li <xunjieli@chromium.org>
Date: Tue Jul 24 16:06:56 2018

Reland "Convert Extensions TCP & TLS Socket APIs to mojo sockets"

This reverts commit 494108abe23d689c0a311025d464edc7453d368b.

Reason for revert: Reland it in M70.

Original change's description:
> Revert "Convert Extensions TCP & TLS Socket APIs to mojo sockets"
> 
> This reverts commit 41066e83216ea19036dcf33cfbf35e18f77da9fe.
> 
> Reason for revert: This CL broke an internal app.  See  crbug.com/865958 
> 
> Original change's description:
> > Convert Extensions TCP & TLS Socket APIs to mojo sockets
> >
> > This CL converts extensions TCP and TLS socket apis to network service's mojo
> > socket.
> >
> > - Remove combined_socket_unittest.cc because the test cases no longer apply. I
> > tried to write equivalent ones in tcp_socket_unittest.cc and
> > tls_socket_unittest.cc
> > - Add a ContentUtilityClient implementation for extensions_browsertests, so we
> > can inject a NetworkServiceTestHelper to the utility process in which network
> > service runs.
> >
> > Bug:  721401 
> > Cq-Include-Trybots: luci.chromium.try:linux_chromium_dbg_ng;master.tryserver.chromium.linux:linux_mojo
> > Change-Id: I333a3021a5c66eb94618be6830ba0ff68c442eba
> > Reviewed-on: https://chromium-review.googlesource.com/1070434
> > Commit-Queue: Helen Li <xunjieli@chromium.org>
> > Reviewed-by: Maks Orlovich <morlovich@chromium.org>
> > Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> > Reviewed-by: Reilly Grant <reillyg@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#563390}
> 
> TBR=jam@chromium.org,reillyg@chromium.org,xunjieli@chromium.org,morlovich@chromium.org
> 
> # Not skipping CQ checks because original CL landed > 1 day ago.
> 
> Bug:  721401 ,  865958 
> Change-Id: I9e9408f31887ab45d0bd967e2f17f91c465836c6
> Cq-Include-Trybots: luci.chromium.try:linux_chromium_dbg_ng;luci.chromium.try:linux_mojo
> Reviewed-on: https://chromium-review.googlesource.com/1145340
> Commit-Queue: Helen Li <xunjieli@chromium.org>
> Reviewed-by: Helen Li <xunjieli@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#576982}

TBR=jam@chromium.org,reillyg@chromium.org,xunjieli@chromium.org,morlovich@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  721401 ,  865958 
Change-Id: I9748ea1d6c04f26429508da9358ca16ff01ff90d
Cq-Include-Trybots: luci.chromium.try:linux_chromium_dbg_ng;luci.chromium.try:linux_mojo
Reviewed-on: https://chromium-review.googlesource.com/1148480
Reviewed-by: Helen Li <xunjieli@chromium.org>
Commit-Queue: Helen Li <xunjieli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577565}
[modify] https://crrev.com/bff8e3ea6d0d7c1f1fe045f595ffaeaf64ff41f9/build/check_gn_headers_whitelist.txt
[delete] https://crrev.com/cdd345c5e180e46e21ef64e4ac87d22b4eb8332f/chrome/browser/extensions/api/socket/combined_socket_unittest.cc
[delete] https://crrev.com/cdd345c5e180e46e21ef64e4ac87d22b4eb8332f/chrome/browser/extensions/api/socket/mock_tcp_client_socket.h
[modify] https://crrev.com/bff8e3ea6d0d7c1f1fe045f595ffaeaf64ff41f9/chrome/browser/extensions/api/socket/tcp_socket_unittest.cc
[modify] https://crrev.com/bff8e3ea6d0d7c1f1fe045f595ffaeaf64ff41f9/chrome/browser/extensions/api/socket/tls_socket_unittest.cc
[modify] https://crrev.com/bff8e3ea6d0d7c1f1fe045f595ffaeaf64ff41f9/chrome/test/BUILD.gn
[modify] https://crrev.com/bff8e3ea6d0d7c1f1fe045f595ffaeaf64ff41f9/chrome/test/data/extensions/api_test/socket/api/background.js
[modify] https://crrev.com/bff8e3ea6d0d7c1f1fe045f595ffaeaf64ff41f9/extensions/browser/api/socket/BUILD.gn
[add] https://crrev.com/bff8e3ea6d0d7c1f1fe045f595ffaeaf64ff41f9/extensions/browser/api/socket/mojo_data_pump.cc
[add] https://crrev.com/bff8e3ea6d0d7c1f1fe045f595ffaeaf64ff41f9/extensions/browser/api/socket/mojo_data_pump.h
[modify] https://crrev.com/bff8e3ea6d0d7c1f1fe045f595ffaeaf64ff41f9/extensions/browser/api/socket/socket.cc
[modify] https://crrev.com/bff8e3ea6d0d7c1f1fe045f595ffaeaf64ff41f9/extensions/browser/api/socket/socket.h
[modify] https://crrev.com/bff8e3ea6d0d7c1f1fe045f595ffaeaf64ff41f9/extensions/browser/api/socket/socket_api.cc
[modify] https://crrev.com/bff8e3ea6d0d7c1f1fe045f595ffaeaf64ff41f9/extensions/browser/api/socket/socket_api.h
[modify] https://crrev.com/bff8e3ea6d0d7c1f1fe045f595ffaeaf64ff41f9/extensions/browser/api/socket/tcp_socket.cc
[modify] https://crrev.com/bff8e3ea6d0d7c1f1fe045f595ffaeaf64ff41f9/extensions/browser/api/socket/tcp_socket.h
[modify] https://crrev.com/bff8e3ea6d0d7c1f1fe045f595ffaeaf64ff41f9/extensions/browser/api/socket/tls_socket.cc
[modify] https://crrev.com/bff8e3ea6d0d7c1f1fe045f595ffaeaf64ff41f9/extensions/browser/api/socket/tls_socket.h
[modify] https://crrev.com/bff8e3ea6d0d7c1f1fe045f595ffaeaf64ff41f9/extensions/browser/api/socket/udp_socket.cc
[modify] https://crrev.com/bff8e3ea6d0d7c1f1fe045f595ffaeaf64ff41f9/extensions/browser/api/socket/udp_socket.h
[modify] https://crrev.com/bff8e3ea6d0d7c1f1fe045f595ffaeaf64ff41f9/extensions/browser/api/sockets_tcp/sockets_tcp_api.cc
[modify] https://crrev.com/bff8e3ea6d0d7c1f1fe045f595ffaeaf64ff41f9/extensions/browser/api/sockets_tcp/sockets_tcp_api.h
[modify] https://crrev.com/bff8e3ea6d0d7c1f1fe045f595ffaeaf64ff41f9/extensions/browser/api/sockets_tcp/sockets_tcp_apitest.cc
[modify] https://crrev.com/bff8e3ea6d0d7c1f1fe045f595ffaeaf64ff41f9/extensions/browser/api/sockets_tcp_server/sockets_tcp_server_api.cc
[modify] https://crrev.com/bff8e3ea6d0d7c1f1fe045f595ffaeaf64ff41f9/extensions/browser/api/sockets_tcp_server/sockets_tcp_server_api.h
[modify] https://crrev.com/bff8e3ea6d0d7c1f1fe045f595ffaeaf64ff41f9/extensions/browser/api/sockets_tcp_server/tcp_server_socket_event_dispatcher.cc
[modify] https://crrev.com/bff8e3ea6d0d7c1f1fe045f595ffaeaf64ff41f9/extensions/browser/api/sockets_tcp_server/tcp_server_socket_event_dispatcher.h
►

Sign in to add a comment