Fix up RadioGroupContainer::SizeOfIncludingThis() and devirtualize RadioGroupContainer
Categories
(Core :: DOM: Forms, defect, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox133 | --- | fixed |
People
(Reporter: mccr8, Assigned: mccr8)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
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.
Assignee | ||
Comment 1•2 months ago
|
||
Assignee | ||
Comment 2•2 months ago
|
||
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
.
Assignee | ||
Comment 3•2 months ago
|
||
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
Assignee | ||
Updated•2 months ago
|
Comment 5•2 months ago
|
||
bugherder |
Description
•