1921097 - Fix up RadioGroupContainer::SizeOfIncludingThis() and devirtualize RadioGroupContainer

Status: RESOLVED FIXED

(Core :: DOM: Forms, defect, P2)

Description

[Andrew McCreight [:mccr8]]

As seen in bug 1908426, ASan is producing this error when running RadioGroupContainer::SizeOfIncludingThis() during the test browser_data_documents_aboutmemory.js:

AddressSanitizer: attempting to call malloc_usable_size() for pointer which is not owned

That method is defined like this:

return mRadioGroups.SizeOfIncludingThis(aMallocSizeOf);

My guess is that the problem is that mRadioGroups is not actually a heap allocated pointer, but an interior pointer.

I thought this was strange, because RadioGroupContainer only has a single field, mRadioGroups and doesn't inherit anything else, so why isn't the interior pointer the same as the entire object? At first I assumed that maybe ASan builds were adding some weird padding, but in a local debug ASan build the assertion MOZ_ASSERT((void*)this == (void*)&mRadioGroups); was failing in RadioGroupContainer::SizeOfIncludingThis(), so that couldn't be it. Then I noticed that WalkRadioGroup is declared with NS_IMETHOD, which means it is virtual, despite the class being final and not having any parent classes. After I de-virtualized that method by changing NS_IMETHOD to nsresult, the assertion started passing.

In practice, I think this doesn't matter. I think jemalloc can deal with interior pointers because of the way it is implemented. I could run browser_data_documents_aboutmemory.js just fine without any problems. The vtable on this class probably doesn't matter, and a smart enough whole program optimizer could trivially remove it.

I have a patch, but I'll do a try push to make sure there's nothing else going on here.

This is precisely the sort of issue mstange's heuristic in bug 1845325 would have caught! Maybe I should actually look into doing that.

Comment 1

[Andrew McCreight [:mccr8]]

https://treeherder.mozilla.org/jobs?repo=try&revision=44c11216979a5d6faab7502ebb84b9759098e54c

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: Bug 1921097 - Fix up RadioGroupContainer::SizeOfIncludingThis().

mRadioGroups is an interior pointer, so we shouldn't pass it to
MallocSizeOf.

Also, devirtualize RadioGroupContainer::WalkRadioGroup. This method
being virtual meant that RadioGroupContainer needed a vtable, causing
this to not be equal to this.mRadioGroups.

Comment 3

[Andrew McCreight [:mccr8]]

I managed to fail to add a new file to the patch from bug 1908426, so here's a new try run:

https://treeherder.mozilla.org/jobs?repo=try&revision=c5f6df481e7ddeec51a8b7b1699027e706713f27

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8853d81e923b
Fix up RadioGroupContainer::SizeOfIncludingThis(). r=dom-core,jjaschke

Comment 5

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/8853d81e923b