Why this CL triggers an OilPan crash.

Why this CL triggers an OilPan crash.

I've added a static/global
HashMap<int, PersistentHeapHashMap<WeakMember<Page>>>
and I've started seeing some OilPan-related crashes.  I've shown my CL
to dcheng@ who does't see anything *obviously* wrong, so we've thought
that maybe somebody more familiar with OilPan can take a look.

Repro steps:
1. Patch in this CL
2. Build browser_tests
3. DISPLAY=:20 out/gn/browser_tests --gtest_filter=*ExtensionApiTabTest.TabUpdate*
   And observe a DCHECK

The DCHECK happens when WebViewImpl's constructor assigns a (seemingly?)
unrelated instance
  dev_tools_emulator_ = DevToolsEmulator::Create(this);

The crash:
[1:1:0510/135608.648852:FATAL:PersistentNode.h(69)] Check failed: !node || node->IsUnused().
base::debug::StackTrace::StackTrace()
logging::LogMessage::~LogMessage()
blink::PersistentNode::FreeListNext()
blink::PersistentRegion::AllocatePersistentNode()
blink::PersistentBase<>::Initialize()
blink::WebViewImpl::WebViewImpl()
blink::WebViewImpl::Create()
content::RenderViewImpl::Initialize()
content::RenderViewImpl::Create()
content::RenderThreadImpl::CreateView()
content::mojom::RendererStubDispatch::Accept()

[Łukasz Anforowicz, 2017-05-10 21:25:11]

lukasza@chromium.org changed reviewers:
+ dcheng@chromium.org

[Łukasz Anforowicz, 2017-05-10 21:25:11]

dcheng@, this is a WIP / not-landable CL that exhibits a mysterious
OilPan-related crash.  The main place where I might have screwed something up
with lifetimes and stuff is in Page.cpp.  If you don't see anything obviously
wrong, then could you please forward to somebody from the OilPan Team (I see
that keishi@ has been looking at the recent memory corruption issues).  FWIW,
repro steps are in the CL description.

[dcheng, 2017-05-10 22:19:02]

dcheng@chromium.org changed reviewers:
+ haraken@chromium.org, keishi@chromium.org, sigbjorn@opera.com

[dcheng, 2017-05-10 22:19:03]

I don't see anything obviously wrong. We discussed earlier whether we need to do
PersistentHeapHashMap<int, PersistentHeapHashSet<WeakMember<T>> instead of just
HashMap<int, PersistentHeapHashSet<WeakMember<T>>, but it doesn't make a
difference (and in any case, Persistent* should be a GC root).

[haraken, 2017-05-11 02:21:19]

On 2017/05/10 22:19:03, dcheng wrote:
> I don't see anything obviously wrong. We discussed earlier whether we need to
do
> PersistentHeapHashMap<int, PersistentHeapHashSet<WeakMember<T>> instead of
just
> HashMap<int, PersistentHeapHashSet<WeakMember<T>>, but it doesn't make a
> difference (and in any case, Persistent* should be a GC root).

Is the crash gone if you use PersistentHeapHashMap<int,
HeapHashSet<WeakMember<T>>?

That sounds strange anyway.

[Łukasz Anforowicz, 2017-05-11 02:49:38]

On 2017/05/11 02:21:19, haraken wrote:
> On 2017/05/10 22:19:03, dcheng wrote:
> > I don't see anything obviously wrong. We discussed earlier whether we need
to
> do
> > PersistentHeapHashMap<int, PersistentHeapHashSet<WeakMember<T>> instead of
> just
> > HashMap<int, PersistentHeapHashSet<WeakMember<T>>, but it doesn't make a
> > difference (and in any case, Persistent* should be a GC root).
> 
> Is the crash gone if you use PersistentHeapHashMap<int,
> HeapHashSet<WeakMember<T>>?
> 
> That sounds strange anyway.

Yes, indeed - the crash is gone when I use:
using BrowsingInstanceHashMap = PersistentHeapHashMap<int,
HeapHashSet<WeakMember<Page>>>;

[Łukasz Anforowicz, 2017-05-11 02:49:38]

On 2017/05/11 02:21:19, haraken wrote:
> On 2017/05/10 22:19:03, dcheng wrote:
> > I don't see anything obviously wrong. We discussed earlier whether we need
to
> do
> > PersistentHeapHashMap<int, PersistentHeapHashSet<WeakMember<T>> instead of
> just
> > HashMap<int, PersistentHeapHashSet<WeakMember<T>>, but it doesn't make a
> > difference (and in any case, Persistent* should be a GC root).
> 
> Is the crash gone if you use PersistentHeapHashMap<int,
> HeapHashSet<WeakMember<T>>?
> 
> That sounds strange anyway.

Yes, indeed - the crash is gone when I use:
using BrowsingInstanceHashMap = PersistentHeapHashMap<int,
HeapHashSet<WeakMember<Page>>>;

[Łukasz Anforowicz, 2017-05-11 03:01:58]

On 2017/05/11 02:49:38, Łukasz A. wrote:
> On 2017/05/11 02:21:19, haraken wrote:
> > On 2017/05/10 22:19:03, dcheng wrote:
> > > I don't see anything obviously wrong. We discussed earlier whether we need
> to
> > do
> > > PersistentHeapHashMap<int, PersistentHeapHashSet<WeakMember<T>> instead of
> > just
> > > HashMap<int, PersistentHeapHashSet<WeakMember<T>>, but it doesn't make a
> > > difference (and in any case, Persistent* should be a GC root).
> > 
> > Is the crash gone if you use PersistentHeapHashMap<int,
> > HeapHashSet<WeakMember<T>>?
> > 
> > That sounds strange anyway.
> 
> Yes, indeed - the crash is gone when I use:
> using BrowsingInstanceHashMap = PersistentHeapHashMap<int,
> HeapHashSet<WeakMember<Page>>>;

At any rate, even with the crash gone for me, this seems like a bug in
Persistent, right?  Whatever copying/moving/etc HashMap is doing, it shouldn't
break internal state of PersistentNodes, right?  OTOH, I see that HashMap
sometimes tries to take shortcuts via memset and similar things (but hopefully
this is forbidden for Persistent).

[haraken, 2017-05-11 03:36:24]

On 2017/05/11 03:01:58, Łukasz A. wrote:
> On 2017/05/11 02:49:38, Łukasz A. wrote:
> > On 2017/05/11 02:21:19, haraken wrote:
> > > On 2017/05/10 22:19:03, dcheng wrote:
> > > > I don't see anything obviously wrong. We discussed earlier whether we
need
> > to
> > > do
> > > > PersistentHeapHashMap<int, PersistentHeapHashSet<WeakMember<T>> instead
of
> > > just
> > > > HashMap<int, PersistentHeapHashSet<WeakMember<T>>, but it doesn't make a
> > > > difference (and in any case, Persistent* should be a GC root).
> > > 
> > > Is the crash gone if you use PersistentHeapHashMap<int,
> > > HeapHashSet<WeakMember<T>>?
> > > 
> > > That sounds strange anyway.
> > 
> > Yes, indeed - the crash is gone when I use:
> > using BrowsingInstanceHashMap = PersistentHeapHashMap<int,
> > HeapHashSet<WeakMember<Page>>>;
> 
> At any rate, even with the crash gone for me, this seems like a bug in
> Persistent, right?  Whatever copying/moving/etc HashMap is doing, it shouldn't
> break internal state of PersistentNodes, right?  OTOH, I see that HashMap
> sometimes tries to take shortcuts via memset and similar things (but hopefully
> this is forbidden for Persistent).

Agreed, I think there is an underlying bug in Persistent, which needs to be
cared for.

[dcheng, 2017-05-11 20:43:37]

On 2017/05/11 03:36:24, haraken wrote:
> On 2017/05/11 03:01:58, Łukasz A. wrote:
> > On 2017/05/11 02:49:38, Łukasz A. wrote:
> > > On 2017/05/11 02:21:19, haraken wrote:
> > > > On 2017/05/10 22:19:03, dcheng wrote:
> > > > > I don't see anything obviously wrong. We discussed earlier whether we
> need
> > > to
> > > > do
> > > > > PersistentHeapHashMap<int, PersistentHeapHashSet<WeakMember<T>>
instead
> of
> > > > just
> > > > > HashMap<int, PersistentHeapHashSet<WeakMember<T>>, but it doesn't make
a
> > > > > difference (and in any case, Persistent* should be a GC root).
> > > > 
> > > > Is the crash gone if you use PersistentHeapHashMap<int,
> > > > HeapHashSet<WeakMember<T>>?
> > > > 
> > > > That sounds strange anyway.
> > > 
> > > Yes, indeed - the crash is gone when I use:
> > > using BrowsingInstanceHashMap = PersistentHeapHashMap<int,
> > > HeapHashSet<WeakMember<Page>>>;
> > 
> > At any rate, even with the crash gone for me, this seems like a bug in
> > Persistent, right?  Whatever copying/moving/etc HashMap is doing, it
shouldn't
> > break internal state of PersistentNodes, right?  OTOH, I see that HashMap
> > sometimes tries to take shortcuts via memset and similar things (but
hopefully
> > this is forbidden for Persistent).
> 
> Agreed, I think there is an underlying bug in Persistent, which needs to be
> cared for.

Shall we file a bug to track this?

[Łukasz Anforowicz, 2017-05-11 20:56:30]

On 2017/05/11 20:43:37, dcheng wrote:
> On 2017/05/11 03:36:24, haraken wrote:
> > On 2017/05/11 03:01:58, Łukasz A. wrote:
> > > On 2017/05/11 02:49:38, Łukasz A. wrote:
> > > > On 2017/05/11 02:21:19, haraken wrote:
> > > > > On 2017/05/10 22:19:03, dcheng wrote:
> > > > > > I don't see anything obviously wrong. We discussed earlier whether
we
> > need
> > > > to
> > > > > do
> > > > > > PersistentHeapHashMap<int, PersistentHeapHashSet<WeakMember<T>>
> instead
> > of
> > > > > just
> > > > > > HashMap<int, PersistentHeapHashSet<WeakMember<T>>, but it doesn't
make
> a
> > > > > > difference (and in any case, Persistent* should be a GC root).
> > > > > 
> > > > > Is the crash gone if you use PersistentHeapHashMap<int,
> > > > > HeapHashSet<WeakMember<T>>?
> > > > > 
> > > > > That sounds strange anyway.
> > > > 
> > > > Yes, indeed - the crash is gone when I use:
> > > > using BrowsingInstanceHashMap = PersistentHeapHashMap<int,
> > > > HeapHashSet<WeakMember<Page>>>;
> > > 
> > > At any rate, even with the crash gone for me, this seems like a bug in
> > > Persistent, right?  Whatever copying/moving/etc HashMap is doing, it
> shouldn't
> > > break internal state of PersistentNodes, right?  OTOH, I see that HashMap
> > > sometimes tries to take shortcuts via memset and similar things (but
> hopefully
> > > this is forbidden for Persistent).
> > 
> > Agreed, I think there is an underlying bug in Persistent, which needs to be
> > cared for.
> 
> Shall we file a bug to track this?

https://crbug.com/721531