WIP: ~HashTableAddResult crashes in oilpan builds

WIP: ~HashTableAddResult crashes in oilpan builds

BUG=402426

[haraken, 2014-08-13 07:54:32]

https://codereview.chromium.org/461413002/diff/20001/Source/core/dom/NodeList...
File Source/core/dom/NodeListsNodeData.h (right):

https://codereview.chromium.org/461413002/diff/20001/Source/core/dom/NodeList...
Source/core/dom/NodeListsNodeData.h:121: fprintf(stderr, "f\n");

This code prints the following crash log. The crash always happens in
jquery/manipulation.html.

a 1 1 0x1f6cb1c10b88 0x1f6cb1c10b88
b 1 707406378 0x1f6cb1c10b88 0x1f6cb1c10b88
c
1 1
d
e
1 707406378
ASSERTION FAILED: m_containerModifications == m_container->modifications()
../../third_party/WebKit/Source/wtf/HashTable.h(292) :
WTF::HashTableAddResult<WTF::HashTable<std::pair<unsigned char, WTF::StringImpl
*>, WTF::KeyValuePair<std::pair<unsigned char, WTF::StringImpl *>,
blink::WeakMember<blink::LiveNodeListBase> >, WTF::KeyValuePairKeyExtractor,
blink::NodeListsNodeData::NodeListAtomicCacheMapEntryHash,
WTF::HashMapValueTraits<WTF::HashTraits<std::pair<unsigned char, WTF::StringImpl
*> >, WTF::HashTraits<blink::WeakMember<blink::LiveNodeListBase> > >,
WTF::HashTraits<std::pair<unsigned char, WTF::StringImpl *> >,
blink::HeapAllocator>, WTF::KeyValuePair<std::pair<unsigned char,
WTF::StringImpl *>, blink::WeakMember<blink::LiveNodeListBase> >
>::~HashTableAddResult() [HashTableType = WTF::HashTable<std::pair<unsigned
char, WTF::StringImpl *>, WTF::KeyValuePair<std::pair<unsigned char,
WTF::StringImpl *>, blink::WeakMember<blink::LiveNodeListBase> >,
WTF::KeyValuePairKeyExtractor,
blink::NodeListsNodeData::NodeListAtomicCacheMapEntryHash,
WTF::HashMapValueTraits<WTF::HashTraits<std::pair<unsigned char, WTF::StringImpl
*> >, WTF::HashTraits<blink::WeakMember<blink::LiveNodeListBase> > >,
WTF::HashTraits<std::pair<unsigned char, WTF::StringImpl *> >,
blink::HeapAllocator>, ValueType = WTF::KeyValuePair<std::pair<unsigned char,
WTF::StringImpl *>, blink::WeakMember<blink::LiveNodeListBase> >]

This log means the following fact:

- The hash map (i.e., m_atomicNameCaches) is already collected in
Heap::collectAllGarbage(). In other words, the AddResult doesn't keep the hash
map alive.

Do you have any idea on what's going on? If no, I can investigate more.

(FYI, if I introduce 'Persistent<NodeListAtomicNameCacheMap>
protect(m_atomicNameCaches)', the above crash goes away. However, instead I hit
the checkHeader assertion in Heap::collectAllGarbage().)

[Mads Ager (chromium), 2014-08-13 08:33:23]

void Heap::collectAllGarbage()
{
    for (int i = 0; i < 5; i++)
        collectGarbage(ThreadState::NoHeapPointersOnStack);
}

This is doing garbage collections without taking pointers on the stack into
account. You need to do Heap::collectGarbage(ThreadState::HeapPointersOnStack)
here. Otherwise, nothing that is only reachable from the stack will be kept
alive. :)

[Mads Ager (chromium), 2014-08-13 08:37:13]

On 2014/08/13 08:33:23, Mads Ager (chromium) wrote:
> void Heap::collectAllGarbage()
> {
>     for (int i = 0; i < 5; i++)
>         collectGarbage(ThreadState::NoHeapPointersOnStack);
> }
> 
> This is doing garbage collections without taking pointers on the stack into
> account. You need to do Heap::collectGarbage(ThreadState::HeapPointersOnStack)
> here. Otherwise, nothing that is only reachable from the stack will be kept
> alive. :)

With that change, I can still consistently reproduce with the same layout test.
The printing becomes:

a 1 1 0x35c370590b88 0x35c370590b88
b 1 2 0x35c370590b88 0x35c370590b88

So there has indeed been a modification during the GC.

[haraken, 2014-08-13 08:41:17]

On 2014/08/13 08:33:23, Mads Ager (chromium) wrote:
> void Heap::collectAllGarbage()
> {
>     for (int i = 0; i < 5; i++)
>         collectGarbage(ThreadState::NoHeapPointersOnStack);
> }
> 
> This is doing garbage collections without taking pointers on the stack into
> account. You need to do Heap::collectGarbage(ThreadState::HeapPointersOnStack)
> here. Otherwise, nothing that is only reachable from the stack will be kept
> alive. :)

oh, good point!

[haraken, 2014-08-13 08:42:08]

On 2014/08/13 08:37:13, Mads Ager (chromium) wrote:
> On 2014/08/13 08:33:23, Mads Ager (chromium) wrote:
> > void Heap::collectAllGarbage()
> > {
> >     for (int i = 0; i < 5; i++)
> >         collectGarbage(ThreadState::NoHeapPointersOnStack);
> > }
> > 
> > This is doing garbage collections without taking pointers on the stack into
> > account. You need to do
Heap::collectGarbage(ThreadState::HeapPointersOnStack)
> > here. Otherwise, nothing that is only reachable from the stack will be kept
> > alive. :)
> 
> With that change, I can still consistently reproduce with the same layout
test.
> The printing becomes:
> 
> a 1 1 0x35c370590b88 0x35c370590b88
> b 1 2 0x35c370590b88 0x35c370590b88
> 
> So there has indeed been a modification during the GC.

Thanks, I'll investigate who did the modification.

[Mads Ager (chromium), 2014-08-13 08:57:17]

On 2014/08/13 08:42:08, haraken wrote:
> On 2014/08/13 08:37:13, Mads Ager (chromium) wrote:
> > On 2014/08/13 08:33:23, Mads Ager (chromium) wrote:
> > > void Heap::collectAllGarbage()
> > > {
> > >     for (int i = 0; i < 5; i++)
> > >         collectGarbage(ThreadState::NoHeapPointersOnStack);
> > > }
> > > 
> > > This is doing garbage collections without taking pointers on the stack
into
> > > account. You need to do
> Heap::collectGarbage(ThreadState::HeapPointersOnStack)
> > > here. Otherwise, nothing that is only reachable from the stack will be
kept
> > > alive. :)
> > 
> > With that change, I can still consistently reproduce with the same layout
> test.
> > The printing becomes:
> > 
> > a 1 1 0x35c370590b88 0x35c370590b88
> > b 1 2 0x35c370590b88 0x35c370590b88
> > 
> > So there has indeed been a modification during the GC.
> 
> Thanks, I'll investigate who did the modification.

We are doing the modification in weak processing. We have introduced a bug here.
We should not be modifying the weak map in this GC. We should have realized that
there is a pointer from the stack. That should have strongified the collection.
That apparently doesn't happen.

[haraken, 2014-08-13 09:05:49]

On 2014/08/13 08:57:17, Mads Ager (chromium) wrote:
> On 2014/08/13 08:42:08, haraken wrote:
> > On 2014/08/13 08:37:13, Mads Ager (chromium) wrote:
> > > On 2014/08/13 08:33:23, Mads Ager (chromium) wrote:
> > > > void Heap::collectAllGarbage()
> > > > {
> > > >     for (int i = 0; i < 5; i++)
> > > >         collectGarbage(ThreadState::NoHeapPointersOnStack);
> > > > }
> > > > 
> > > > This is doing garbage collections without taking pointers on the stack
> into
> > > > account. You need to do
> > Heap::collectGarbage(ThreadState::HeapPointersOnStack)
> > > > here. Otherwise, nothing that is only reachable from the stack will be
> kept
> > > > alive. :)
> > > 
> > > With that change, I can still consistently reproduce with the same layout
> > test.
> > > The printing becomes:
> > > 
> > > a 1 1 0x35c370590b88 0x35c370590b88
> > > b 1 2 0x35c370590b88 0x35c370590b88
> > > 
> > > So there has indeed been a modification during the GC.
> > 
> > Thanks, I'll investigate who did the modification.
> 
> We are doing the modification in weak processing. We have introduced a bug
here.
> We should not be modifying the weak map in this GC. We should have realized
that
> there is a pointer from the stack. That should have strongified the
collection.
> That apparently doesn't happen.

Probably this one?

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

ephemeron processing is always called with WeakPointersActWeak.

[Mads Ager (chromium), 2014-08-13 09:06:38]

On 2014/08/13 08:57:17, Mads Ager (chromium) wrote:
> On 2014/08/13 08:42:08, haraken wrote:
> > On 2014/08/13 08:37:13, Mads Ager (chromium) wrote:
> > > On 2014/08/13 08:33:23, Mads Ager (chromium) wrote:
> > > > void Heap::collectAllGarbage()
> > > > {
> > > >     for (int i = 0; i < 5; i++)
> > > >         collectGarbage(ThreadState::NoHeapPointersOnStack);
> > > > }
> > > > 
> > > > This is doing garbage collections without taking pointers on the stack
> into
> > > > account. You need to do
> > Heap::collectGarbage(ThreadState::HeapPointersOnStack)
> > > > here. Otherwise, nothing that is only reachable from the stack will be
> kept
> > > > alive. :)
> > > 
> > > With that change, I can still consistently reproduce with the same layout
> > test.
> > > The printing becomes:
> > > 
> > > a 1 1 0x35c370590b88 0x35c370590b88
> > > b 1 2 0x35c370590b88 0x35c370590b88
> > > 
> > > So there has indeed been a modification during the GC.
> > 
> > Thanks, I'll investigate who did the modification.
> 
> We are doing the modification in weak processing. We have introduced a bug
here.
> We should not be modifying the weak map in this GC. We should have realized
that
> there is a pointer from the stack. That should have strongified the
collection.
> That apparently doesn't happen.

This is due to changes done to marking. I'll revert the change causing this: 

https://codereview.chromium.org/413133006

I'll attempt to create a regression unittest case so we will not regress this
again. :)

[haraken, 2014-08-13 09:07:10]

On 2014/08/13 09:06:38, Mads Ager (chromium) wrote:
> On 2014/08/13 08:57:17, Mads Ager (chromium) wrote:
> > On 2014/08/13 08:42:08, haraken wrote:
> > > On 2014/08/13 08:37:13, Mads Ager (chromium) wrote:
> > > > On 2014/08/13 08:33:23, Mads Ager (chromium) wrote:
> > > > > void Heap::collectAllGarbage()
> > > > > {
> > > > >     for (int i = 0; i < 5; i++)
> > > > >         collectGarbage(ThreadState::NoHeapPointersOnStack);
> > > > > }
> > > > > 
> > > > > This is doing garbage collections without taking pointers on the stack
> > into
> > > > > account. You need to do
> > > Heap::collectGarbage(ThreadState::HeapPointersOnStack)
> > > > > here. Otherwise, nothing that is only reachable from the stack will be
> > kept
> > > > > alive. :)
> > > > 
> > > > With that change, I can still consistently reproduce with the same
layout
> > > test.
> > > > The printing becomes:
> > > > 
> > > > a 1 1 0x35c370590b88 0x35c370590b88
> > > > b 1 2 0x35c370590b88 0x35c370590b88
> > > > 
> > > > So there has indeed been a modification during the GC.
> > > 
> > > Thanks, I'll investigate who did the modification.
> > 
> > We are doing the modification in weak processing. We have introduced a bug
> here.
> > We should not be modifying the weak map in this GC. We should have realized
> that
> > there is a pointer from the stack. That should have strongified the
> collection.
> > That apparently doesn't happen.
> 
> This is due to changes done to marking. I'll revert the change causing this: 
> 
> https://codereview.chromium.org/413133006
> 
> I'll attempt to create a regression unittest case so we will not regress this
> again. :)

I'll delegate the work to you :)

[Mads Ager (chromium), 2014-08-13 10:57:41]

On 2014/08/13 09:07:10, haraken wrote:
> On 2014/08/13 09:06:38, Mads Ager (chromium) wrote:
> > On 2014/08/13 08:57:17, Mads Ager (chromium) wrote:
> > > On 2014/08/13 08:42:08, haraken wrote:
> > > > On 2014/08/13 08:37:13, Mads Ager (chromium) wrote:
> > > > > On 2014/08/13 08:33:23, Mads Ager (chromium) wrote:
> > > > > > void Heap::collectAllGarbage()
> > > > > > {
> > > > > >     for (int i = 0; i < 5; i++)
> > > > > >         collectGarbage(ThreadState::NoHeapPointersOnStack);
> > > > > > }
> > > > > > 
> > > > > > This is doing garbage collections without taking pointers on the
stack
> > > into
> > > > > > account. You need to do
> > > > Heap::collectGarbage(ThreadState::HeapPointersOnStack)
> > > > > > here. Otherwise, nothing that is only reachable from the stack will
be
> > > kept
> > > > > > alive. :)
> > > > > 
> > > > > With that change, I can still consistently reproduce with the same
> layout
> > > > test.
> > > > > The printing becomes:
> > > > > 
> > > > > a 1 1 0x35c370590b88 0x35c370590b88
> > > > > b 1 2 0x35c370590b88 0x35c370590b88
> > > > > 
> > > > > So there has indeed been a modification during the GC.
> > > > 
> > > > Thanks, I'll investigate who did the modification.
> > > 
> > > We are doing the modification in weak processing. We have introduced a bug
> > here.
> > > We should not be modifying the weak map in this GC. We should have
realized
> > that
> > > there is a pointer from the stack. That should have strongified the
> > collection.
> > > That apparently doesn't happen.
> > 
> > This is due to changes done to marking. I'll revert the change causing this:

> > 
> > https://codereview.chromium.org/413133006
> > 
> > I'll attempt to create a regression unittest case so we will not regress
this
> > again. :)
> 
> I'll delegate the work to you :)

Update: This is *not* caused by the change above. It is only made much more
likely to happen with the change that I reverted. The underlying issue is that
the builtin weak processing can end up removing the newly added nullptr weak
member in weak processing. It should never do that. Erik is creating the real
fix for this and then I will reland the marking order change. :)