Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(88)

Issue 10000004: Make the CancellableSyncSocket non-blocking on Send, and blocking on Receive (Closed)

Created:
8 years, 8 months ago by no longer working on chromium
Modified:
8 years, 8 months ago
CC:
chromium-reviews, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, jam, darin-cc_chromium.org
Visibility:
Public.

Description

If we are using blocking write, when the renderer stop getting the data without notifying the browser, it will hang the browser. This happens with some plugins which use the sync sockets provided by the Pepper. This patch change CancellableSyncSocket to be non-blocking on sending, so that we don't need to worry the whole browser hangs by one plugin application. Also, we remove the lock in audio_sync_reader.cc since it is not really needed if we don't set the socket_ to NULL when calling Close(). By doing this we allow the user to close the socket while another thread is writing to the socket. BUG=121152 TEST=ipc_tests Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=132842

Patch Set 1 #

Patch Set 2 : Changed the SyncSocket to non-blocking on sending. #

Patch Set 3 : add one more test on the blocking receiving && add windows code. #

Patch Set 4 : ready for review now. #

Total comments: 31

Patch Set 5 : addressed Tommi's comments #

Total comments: 15

Patch Set 6 : use WaitForMultipleObjects in windows #

Total comments: 12

Patch Set 7 : addressed Tommi's comments #

Total comments: 2

Patch Set 8 : addressed cpu's comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+614 lines, -514 lines) Patch
M base/sync_socket.h View 1 2 3 4 5 6 3 chunks +10 lines, -4 lines 0 comments Download
M base/sync_socket_posix.cc View 1 2 3 4 5 3 chunks +23 lines, -2 lines 0 comments Download
M base/sync_socket_win.cc View 1 2 3 4 5 6 7 1 chunk +271 lines, -255 lines 0 comments Download
M content/browser/renderer_host/media/audio_sync_reader.h View 1 chunk +0 lines, -6 lines 0 comments Download
M content/browser/renderer_host/media/audio_sync_reader.cc View 2 chunks +1 line, -3 lines 0 comments Download
M ipc/sync_socket_unittest.cc View 1 2 3 4 5 1 chunk +309 lines, -244 lines 0 comments Download

Messages

Total messages: 19 (0 generated)
no longer working on chromium
please review.
8 years, 8 months ago (2012-04-10 17:35:08 UTC) #1
tommi (sloooow) - chröme
please fix TEST= and BUG=
8 years, 8 months ago (2012-04-11 13:28:03 UTC) #2
no longer working on chromium
Tommi, please review now.
8 years, 8 months ago (2012-04-13 09:26:58 UTC) #3
tommi (sloooow) - chröme
https://chromiumcodereview.appspot.com/10000004/diff/5004/base/sync_socket.h File base/sync_socket.h (right): https://chromiumcodereview.appspot.com/10000004/diff/5004/base/sync_socket.h#newcode80 base/sync_socket.h:80: // another thread while a blocking Receive or a ...
8 years, 8 months ago (2012-04-13 10:28:10 UTC) #4
no longer working on chromium
Please take another round. https://chromiumcodereview.appspot.com/10000004/diff/5004/base/sync_socket.h File base/sync_socket.h (right): https://chromiumcodereview.appspot.com/10000004/diff/5004/base/sync_socket.h#newcode80 base/sync_socket.h:80: // another thread while a ...
8 years, 8 months ago (2012-04-13 11:50:49 UTC) #5
no longer working on chromium
Hi, Can I have your owner stamps brettw, for base. cpu, for ipc. Thanks, -SX
8 years, 8 months ago (2012-04-13 11:57:07 UTC) #6
tommi (sloooow) - chröme
https://chromiumcodereview.appspot.com/10000004/diff/14002/base/sync_socket_posix.cc File base/sync_socket_posix.cc (right): https://chromiumcodereview.appspot.com/10000004/diff/14002/base/sync_socket_posix.cc#newcode99 base/sync_socket_posix.cc:99: if (-1 == len) condense this to: return len ...
8 years, 8 months ago (2012-04-13 12:56:47 UTC) #7
no longer working on chromium
https://chromiumcodereview.appspot.com/10000004/diff/14002/base/sync_socket_posix.cc File base/sync_socket_posix.cc (right): https://chromiumcodereview.appspot.com/10000004/diff/14002/base/sync_socket_posix.cc#newcode99 base/sync_socket_posix.cc:99: if (-1 == len) On 2012/04/13 12:56:47, tommi wrote: ...
8 years, 8 months ago (2012-04-16 10:26:45 UTC) #8
tommi (sloooow) - chröme
Hey Shijing - feel free to ping me anytime today to go over the changed ...
8 years, 8 months ago (2012-04-16 11:24:56 UTC) #9
tommi (sloooow) - chröme
lgtm! great work Shijing. A few minor requests below. https://chromiumcodereview.appspot.com/10000004/diff/22001/base/sync_socket.h File base/sync_socket.h (right): https://chromiumcodereview.appspot.com/10000004/diff/22001/base/sync_socket.h#newcode108 base/sync_socket.h:108: ...
8 years, 8 months ago (2012-04-16 13:06:36 UTC) #10
no longer working on chromium
Done with the comments from Tommi. brettw and cpu, could you please take a quick ...
8 years, 8 months ago (2012-04-16 15:45:30 UTC) #11
brettw
Owners rubberstamp LGTM, I didn't look at the details
8 years, 8 months ago (2012-04-17 17:42:33 UTC) #12
cpu_(ooo_6.6-7.5)
give me 10 more minutes. http://codereview.chromium.org/10000004/diff/26003/base/sync_socket_win.cc File base/sync_socket_win.cc (right): http://codereview.chromium.org/10000004/diff/26003/base/sync_socket_win.cc#newcode137 base/sync_socket_win.cc:137: arraysize(events), events, false, timeout_in_ms); ...
8 years, 8 months ago (2012-04-17 20:04:53 UTC) #13
cpu_(ooo_6.6-7.5)
lgtm
8 years, 8 months ago (2012-04-17 20:24:08 UTC) #14
cpu_(ooo_6.6-7.5)
Jut for the record and given that this CL is only fixes on the already ...
8 years, 8 months ago (2012-04-17 20:37:02 UTC) #15
tommi (sloooow) - chröme
thanks cpu, that's a better approach. I'll keep it in mind the next time we ...
8 years, 8 months ago (2012-04-18 07:38:35 UTC) #16
no longer working on chromium
thanks cpu for the good tips. > On Tue, Apr 17, 2012 at 10:37 PM, ...
8 years, 8 months ago (2012-04-18 08:35:12 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/10000004/30002
8 years, 8 months ago (2012-04-18 17:14:17 UTC) #18
commit-bot: I haz the power
8 years, 8 months ago (2012-04-18 19:30:14 UTC) #19
Change committed as 132842

Powered by Google App Engine
This is Rietveld 408576698