Issue 229203003: Make mojo buffer allocators more robust.

Issue 229203003: Make mojo buffer allocators more robust. (Closed)

Created:
6 years, 8 months ago by Tom Sepez

Modified:
6 years, 8 months ago

Reviewers:
darin (slow to review)

CC:
chromium-reviews, Aaron Boodman, darin (slow to review), viettrungluu+watch_chromium.org, ben+mojo_chromium.org, abarth-chromium

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Adds a maximum segment size and enforces it. Adds comments about the necessity of zero'd memory in mojo allocators. R=darin@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262767

Patch Set 2 : Beef up mojo allocators. #

Patch Set 3 : Tidy a little. #

Total comments: 2

Created: 6 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+40 lines, -25 lines)			Patch
M	mojo/public/cpp/bindings/lib/fixed_buffer.cc	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	mojo/public/cpp/bindings/lib/scratch_buffer.h	View	1	2 chunks	+5 lines, -2 lines	0 comments	Download
M	mojo/public/cpp/bindings/lib/scratch_buffer.cc	View	1 2	2 chunks	+22 lines, -23 lines	2 comments	Download
M	mojo/public/cpp/bindings/tests/buffer_unittest.cc	View	1	2 chunks	+12 lines, -0 lines	0 comments	Download

Messages

Total messages: 11 (0 generated)

Expand Messages | Collapse Messages

Tom Sepez

While I'm at it, let me put some more checks into place.

6 years, 8 months ago (2014-04-08 23:16:21 UTC) #2

darin (slow to review)

LGTM https://codereview.chromium.org/229203003/diff/40001/mojo/public/cpp/bindings/lib/scratch_buffer.cc File mojo/public/cpp/bindings/lib/scratch_buffer.cc (right): https://codereview.chromium.org/229203003/diff/40001/mojo/public/cpp/bindings/lib/scratch_buffer.cc#newcode56 mojo/public/cpp/bindings/lib/scratch_buffer.cc:56: if (func && result) { I'm pretty sure ...

6 years, 8 months ago (2014-04-09 04:52:42 UTC) #3

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tsepez@chromium.org/229203003/40001

6 years, 8 months ago (2014-04-09 16:54:12 UTC) #5

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

6 years, 8 months ago (2014-04-09 18:07:01 UTC) #6

commit-bot: I haz the power

Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_clang_dbg

6 years, 8 months ago (2014-04-09 18:07:01 UTC) #7

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tsepez@chromium.org/229203003/40001

6 years, 8 months ago (2014-04-09 18:54:05 UTC) #9

Tom Sepez

6 years, 8 months ago (2014-04-09 19:55:27 UTC) #11

Message was sent while issue was closed.

On 2014/04/09 04:52:42, darin wrote:
> LGTM
> 
>
https://codereview.chromium.org/229203003/diff/40001/mojo/public/cpp/bindings...
> File mojo/public/cpp/bindings/lib/scratch_buffer.cc (right):
> 
>
https://codereview.chromium.org/229203003/diff/40001/mojo/public/cpp/bindings...
> mojo/public/cpp/bindings/lib/scratch_buffer.cc:56: if (func && result) {
> I'm pretty sure the bindings don't respond kindly to Buffer::Allocate
returning
> NULL. That might something to look at as well.
> 
>
https://codereview.chromium.org/229203003/diff/40001/mojo/public/cpp/bindings...
> mojo/public/cpp/bindings/lib/scratch_buffer.cc:85: if (segment) {
> I guess you are doing this because this code shouldn't count on malloc
aborting
> the program if it fails to allocate memory? Makes sense.
To answer your questions, this is to keep the unit tests happy; they seem to run
in an environment where malloc doesn't abort.

Expand Messages | Collapse Messages