Oilpan: Use WTF::allocPages() to allocate aligned blocks of memory

Oilpan: Use WTF::allocPages() to allocate aligned blocks of memory

PageMemoryRegion::allocate() duplicates the non-trivial functionality of
WTF::allocPages(), to allocate an aligned block of memory at a random
base address.

The two implementations were not identical, but the goals and APIs are
currently, so it doesn't seem meaningful at this point to have different implementations.

[Jens Widell, 2014-11-13 16:23:28]

jl@opera.com changed reviewers:
+ haraken@chromium.org

[Jens Widell, 2014-11-13 16:23:29]

PTAL

WTF::allocPages() is what PartitionAlloc uses.

Nothing important, just something that stood out a bit while reading the code
for the first time.

[haraken, 2014-11-14 00:47:36]

LGTM

[Jens Widell, 2014-11-14 06:58:17]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-11-14 06:59:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/724833002/1

[commit-bot: I haz the power, 2014-11-14 07:35:28]

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

[commit-bot: I haz the power, 2014-11-14 07:35:30]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/36492)

[Jens Widell, 2014-11-14 08:18:44]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-11-14 08:19:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/724833002/1

[commit-bot: I haz the power, 2014-11-14 08:52:01]

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

[commit-bot: I haz the power, 2014-11-14 08:52:05]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/36506)

[Jens Widell, 2014-11-14 11:48:30]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-11-14 11:48:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/724833002/1

[commit-bot: I haz the power, 2014-11-14 12:29:46]

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

[commit-bot: I haz the power, 2014-11-14 12:29:47]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/36529)

[sof, 2014-11-14 13:26:11]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2014-11-14 13:26:11]

Minor thing worth mentioning: the goals of these two allocation routines will
not necessarily remain the same for all time.

i.e., as stacks will have to be conservatively scanned, 
we may want to pick parts of the VM address space that reduces the likelihood of
false positives.

But not an issue at present.

[Jens Widell, 2014-11-14 13:35:07]

On 2014/11/14 13:26:11, sof wrote:
> Minor thing worth mentioning: the goals of these two allocation routines will
> not necessarily remain the same for all time.
> 
> i.e., as stacks will have to be conservatively scanned, 
> we may want to pick parts of the VM address space that reduces the likelihood
of
> false positives.
> 
> But not an issue at present.

I guess this would mean something like "we prefer an address > X" to avoid
overlap with "small to medium sized integers"? I should think this would be
something to extend the shared implementation with rather than a motivation to
have two separate implementations.

It's also possible that PA wants to avoid the same parts of the address space,
but for (paranoid) security reasons. And Oilpan too, for that matter.

[sof, 2014-11-14 13:40:07]

On 2014/11/14 13:35:07, Jens Widell wrote:
> On 2014/11/14 13:26:11, sof wrote:
> > Minor thing worth mentioning: the goals of these two allocation routines
will
> > not necessarily remain the same for all time.
> > 
> > i.e., as stacks will have to be conservatively scanned, 
> > we may want to pick parts of the VM address space that reduces the
likelihood
> of
> > false positives.
> > 
> > But not an issue at present.
> 
> I guess this would mean something like "we prefer an address > X" to avoid
> overlap with "small to medium sized integers"? I should think this would be
> something to extend the shared implementation with rather than a motivation to
> have two separate implementations.
> 

That would be but one trick; black lists of "almost valid" references are
another, you wanting to steer away from making them become valid with a later
allocation. Potentially.

I don't think you want to prevent that for other allocPages() usages.

[Jens Widell, 2014-11-14 13:50:09]

On 2014/11/14 13:40:07, sof wrote:
> On 2014/11/14 13:35:07, Jens Widell wrote:
> > On 2014/11/14 13:26:11, sof wrote:
> > > Minor thing worth mentioning: the goals of these two allocation routines
> will
> > > not necessarily remain the same for all time.
> > > 
> > > i.e., as stacks will have to be conservatively scanned, 
> > > we may want to pick parts of the VM address space that reduces the
> likelihood
> > of
> > > false positives.
> > > 
> > > But not an issue at present.
> > 
> > I guess this would mean something like "we prefer an address > X" to avoid
> > overlap with "small to medium sized integers"? I should think this would be
> > something to extend the shared implementation with rather than a motivation
to
> > have two separate implementations.
> > 
> 
> That would be but one trick; black lists of "almost valid" references are
> another, you wanting to steer away from making them become valid with a later
> allocation. Potentially.
> 
> I don't think you want to prevent that for other allocPages() usages.

No, preventing that for PartitionAlloc probably wouldn't make much sense. But I
think anything that's essentially a yea-or-nay on a proposed (or allocated)
address range could be added as an optional extension of the shared algorithm.
We could for instance make the (random) address suggestion generator a
parameter, and leave the rest shared.

[sof, 2014-11-14 13:58:51]

On 2014/11/14 13:50:09, Jens Widell wrote:
> On 2014/11/14 13:40:07, sof wrote:
> > On 2014/11/14 13:35:07, Jens Widell wrote:
> > > On 2014/11/14 13:26:11, sof wrote:
> > > > Minor thing worth mentioning: the goals of these two allocation routines
> > will
> > > > not necessarily remain the same for all time.
> > > > 
> > > > i.e., as stacks will have to be conservatively scanned, 
> > > > we may want to pick parts of the VM address space that reduces the
> > likelihood
> > > of
> > > > false positives.
> > > > 
> > > > But not an issue at present.
> > > 
> > > I guess this would mean something like "we prefer an address > X" to avoid
> > > overlap with "small to medium sized integers"? I should think this would
be
> > > something to extend the shared implementation with rather than a
motivation
> to
> > > have two separate implementations.
> > > 
> > 
> > That would be but one trick; black lists of "almost valid" references are
> > another, you wanting to steer away from making them become valid with a
later
> > allocation. Potentially.
> > 
> > I don't think you want to prevent that for other allocPages() usages.
> 
> No, preventing that for PartitionAlloc probably wouldn't make much sense. But
I
> think anything that's essentially a yea-or-nay on a proposed (or allocated)
> address range could be added as an optional extension of the shared algorithm.
> We could for instance make the (random) address suggestion generator a
> parameter, and leave the rest shared.

Certainly could. Just probing the claim of "identical goals" here; hope that's
ok.

[Jens Widell, 2014-11-14 14:24:55]

On 2014/11/14 13:58:51, sof wrote:
> On 2014/11/14 13:50:09, Jens Widell wrote:
> > On 2014/11/14 13:40:07, sof wrote:
> > > On 2014/11/14 13:35:07, Jens Widell wrote:
> > > > On 2014/11/14 13:26:11, sof wrote:
> > > > > Minor thing worth mentioning: the goals of these two allocation
routines
> > > will
> > > > > not necessarily remain the same for all time.
> > > > > 
> > > > > i.e., as stacks will have to be conservatively scanned, 
> > > > > we may want to pick parts of the VM address space that reduces the
> > > likelihood
> > > > of
> > > > > false positives.
> > > > > 
> > > > > But not an issue at present.
> > > > 
> > > > I guess this would mean something like "we prefer an address > X" to
avoid
> > > > overlap with "small to medium sized integers"? I should think this would
> be
> > > > something to extend the shared implementation with rather than a
> motivation
> > to
> > > > have two separate implementations.
> > > > 
> > > 
> > > That would be but one trick; black lists of "almost valid" references are
> > > another, you wanting to steer away from making them become valid with a
> later
> > > allocation. Potentially.
> > > 
> > > I don't think you want to prevent that for other allocPages() usages.
> > 
> > No, preventing that for PartitionAlloc probably wouldn't make much sense.
But
> I
> > think anything that's essentially a yea-or-nay on a proposed (or allocated)
> > address range could be added as an optional extension of the shared
algorithm.
> > We could for instance make the (random) address suggestion generator a
> > parameter, and leave the rest shared.
> 
> Certainly could. Just probing the claim of "identical goals" here; hope that's
> ok.

That couldn't be more ok, of course. :-)

I've adjusted the description to be a bit less definitive (in time) about the
goals being identical.

[Jens Widell, 2014-11-17 07:09:04]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-11-17 07:10:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/724833002/1

[commit-bot: I haz the power, 2014-11-17 07:33:30]

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

[commit-bot: I haz the power, 2014-11-17 07:33:31]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/36778)

[haraken, 2014-11-17 07:38:27]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-11-17 07:39:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/724833002/1

[commit-bot: I haz the power, 2014-11-17 08:00:41]

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

[commit-bot: I haz the power, 2014-11-17 08:00:42]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/36782)

[Jens Widell, 2014-11-17 08:29:46]

On 2014/11/17 08:00:42, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   win_blink_rel on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/36782)

Wait a minute... the last try run on win_blink_rel failed blink_heap_unittests,
crashing in WTF::allocPages().

[Jens Widell, 2014-11-17 08:31:16]

On 2014/11/17 08:29:46, Jens Widell wrote:
> On 2014/11/17 08:00:42, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   win_blink_rel on tryserver.blink
> >
>
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/36782)
> 
> Wait a minute... the last try run on win_blink_rel failed
blink_heap_unittests,
> crashing in WTF::allocPages().

ASSERTION FAILED: !(len & kPageAllocationGranularityOffsetMask)

Too used to win_blink_rel randomly failing.

[Jens Widell, 2014-11-17 10:20:08]

PS#2 takes care of the allocation granularity problem.

If I understand it correctly, you can actually only allocate multiples of 64KB
on Windows. Requesting less from the OS presumably actually gives you the
nearest 64KB multiple.

WTF::allocPages() forces the caller to care about this, which I guess is
essentially a good thing; we're forced to realize how much address space we're
actually allocating.

https://codereview.chromium.org/724833002/diff/20001/Source/platform/heap/Hea...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/724833002/diff/20001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:243: WTF::setSystemPagesInaccessible(base, size);
I realized the old code here actually allocated initially inaccessible memory,
whereas WTF::allocPages() allocates initially read-write memory.

Instead of adjusting the access bits after allocation, we could make
allocPages() take a flag indicating what the caller wants.

[haraken, 2015-03-25 00:23:59]

Jens: Sorry for triggering an old issue, but do you have any update on this CL?

We're currently getting some crash reports from PageMemoryRegion::allocatePages
in Windows (https://code.google.com/p/chromium/issues/detail?id=469283). It
seems that the current Windows-specific logic to allocate new OS pages is
causing the crash. So I want to fix the crash by replacing the implementation
with WTF::allocPages (which we already know is working correctly).