Skip thread-unsafe Windows heaps from MallocDumpProvider.

Skip thread-unsafe Windows heaps from MallocDumpProvider.

It's crashy to try to dump a heap created with HEAP_NO_SERIALIZE flag.
Unfortunately there's no WinAPI to tell with which flags heap has been
created. But low-fragmentation heaps are used by default on modern
versions of Windows and incompatible with HEAP_NO_SERIALIZE flag.
The type of heap is criteria to skip unsafe heaps from accounting.
Main heap is always accounted even if not LFH for some reason.

BUG=665516

[kraynov, 2016-11-23 21:44:03]

The CQ bit was checked by kraynov@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-23 21:44:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-23 23:46:32]

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

[commit-bot: I haz the power, 2016-11-23 23:46:33]

Dry run: Try jobs failed on following builders:
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[Primiano Tucci (use gerrit), 2016-11-24 01:23:06]

primiano@chromium.org changed reviewers:
+ brucedawson@chromium.org, primiano@chromium.org

[Primiano Tucci (use gerrit), 2016-11-24 01:23:07]

Bruce sorry to bug you but this is causing quite some headaches recently to us.
It looks like the existing code became uber crashy after control flow was
enabled as that introduces a HEAP_NO_SERIALIZATION heap which can't be locked
(see https://bugs.chromium.org/p/chromium/issues/detail?id=665516#c11)

Is this approach kraynov@ is trying out reasonable in your opinion? Honestly
don't know anymore what to expect.

[brucedawson, 2016-11-24 01:28:29]

On 2016/11/24 01:23:07, Primiano Tucci wrote:
> Bruce sorry to bug you but this is causing quite some headaches recently to
us.
> It looks like the existing code became uber crashy after control flow was
> enabled as that introduces a HEAP_NO_SERIALIZATION heap which can't be locked
> (see https://bugs.chromium.org/p/chromium/issues/detail?id=665516#c11)
> 
> Is this approach kraynov@ is trying out reasonable in your opinion? Honestly
> don't know anymore what to expect.

There is no safe way to iterate over all of the heaps in a process. Even if a
heap is created with serialization enabled, the code that created that heap
could decide to delete it at any point. Locking it doesn't help in that case
because the heap may be deleted before you try acquiring the lock.

We should probably avoid iterating heaps we don't own except in rare
circumstances.

[Primiano Tucci (use gerrit), 2016-11-24 18:09:09]

On 2016/11/24 01:28:29, brucedawson wrote:
> On 2016/11/24 01:23:07, Primiano Tucci wrote:
> > Bruce sorry to bug you but this is causing quite some headaches recently to
> us.
> > It looks like the existing code became uber crashy after control flow was
> > enabled as that introduces a HEAP_NO_SERIALIZATION heap which can't be
locked
> > (see https://bugs.chromium.org/p/chromium/issues/detail?id=665516#c11)
> > 
> > Is this approach kraynov@ is trying out reasonable in your opinion? Honestly
> > don't know anymore what to expect.
> 
> There is no safe way to iterate over all of the heaps in a process. Even if a
> heap is created with serialization enabled, the code that created that heap
> could decide to delete it at any point. Locking it doesn't help in that case
> because the heap may be deleted before you try acquiring the lock.
> 
> We should probably avoid iterating heaps we don't own except in rare
> circumstances.

Thanks bruce. kraynov and I did some further investigations.
Right, the problem here is that you can't safely do a HeapQueryInformation +
HeapLock, as the heap could be destroyed between the two.
The key thing is that HeapLock either succeeds or causes a crash if the heap has
been deleted. 
In an ideal world we'd have a HeapQueryInformationAndLockIfExists. but this
world is not ideal.
So we'll fall back just navigating the main heap and live happy with that.

[kraynov, 2016-11-24 18:30:45]

not lgtm
There is no safe way to do that :(

[kraynov, 2016-11-24 18:31:04]

Description was changed from

==========
Skip thread-unsafe Windows heaps from MallocDumpProvider.

It's crashy to try to dump a heap created with HEAP_NO_SERIALIZE flag.
Unfortunately there's no WinAPI to tell with which flags heap has been
created. But low-fragmentation heaps are used by default on modern
versions of Windows and incompatible with HEAP_NO_SERIALIZE flag.
The type of heap is criteria to skip unsafe heaps from accounting.
Main heap is always accounted even if not LFH for some reason.

BUG=665516
==========

to

==========
Skip thread-unsafe Windows heaps from MallocDumpProvider.

It's crashy to try to dump a heap created with HEAP_NO_SERIALIZE flag.
Unfortunately there's no WinAPI to tell with which flags heap has been
created. But low-fragmentation heaps are used by default on modern
versions of Windows and incompatible with HEAP_NO_SERIALIZE flag.
The type of heap is criteria to skip unsafe heaps from accounting.
Main heap is always accounted even if not LFH for some reason.

BUG=665516
==========