nsXULPrototypeNode subclasses should have a private dtor
Categories
(Core :: XUL, task, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox69 | --- | fixed |
People
(Reporter: mccr8, Assigned: mccr8)
References
(Blocks 2 open bugs)
Details
Attachments
(4 files)
This class is refcounted, but it has a public dtor, which means you can, for instance, make an nsAutoPtr for it. Presumably this indicates a bug in the private dtor for refcounted things static analysis.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 1•5 years ago
•
|
||
Unsurprisingly, this is a problem for more than just the one subclass.
I'm working on a patch, and it indirectly revealed one security bug (so far). edit: It didn't actually. See below.
XULContentSinkImpl::CreateElement() creates a nsXULPrototypeElement, but does not addref it, which is gross, but sort of ok. This function has two call sites. The first, XULContentSinkImpl::OpenTag, calls delete on element if mContextStack.GetTopChildren() fails. The second, XULContentSinkImpl::OpenRoot(), calls Release() on element if a call to mContextStack.Push() fails, which is unsound, because element doesn't have a reference to it! Which is why I hid the bug. But it turns out that XULContentSinkImpl::ContextStack::Push() is actually infallible, so this is not actually a bug.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 2•5 years ago
|
||
By adding an optional nodeinfo arg to the nsXULPrototypeElement ctor,
the CreateElement method can be easily inlined.
Assignee | ||
Comment 3•5 years ago
|
||
This lets us remove some error handling code, including some that is
incorrect.
Assignee | ||
Comment 4•5 years ago
|
||
There are a few places that use XUL prototype nodes without ref
pointers, which is dangerous. A benefit of this change is that it
removes a direct call to delete on a refcounted class. It does result
in some additional refcount traffic, but hopefully that doesn't
matter.
Assignee | ||
Comment 5•5 years ago
|
||
Refcounted classes should not ever have delete called on them
directly, because something else might be holding a reference to
them. One way to make this harder is to declare the dtor private.
The immediate motivation for this change is that somebody was using
one of these classes as the value for an nsClassHashtable, which
should be rejected at compile time.
Pushed by amccreight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e5a0ca3d92cc part 1 - Inline XULContentSinkImpl::CreateElement. r=bzbarsky https://hg.mozilla.org/integration/autoland/rev/98e692885ecc part 2 - XULContentSinkImpl::ContextStack::Push is infallible. r=bzbarsky https://hg.mozilla.org/integration/autoland/rev/fd915382080d part 3 - Use refptrs in more places in XULContentSinkImpl. r=bzbarsky https://hg.mozilla.org/integration/autoland/rev/bfc0d9412df9 part 4 - Make dtors for nsXULPrototypeNode subclasses private. r=bzbarsky
Backout by nbeleuzu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/55e113f30a95 Backed out 4 changesets for build bustages on nsXULElement.h . CLOSED TREE
Pushed by amccreight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/aafcf88cbefc part 1 - Inline XULContentSinkImpl::CreateElement. r=bzbarsky https://hg.mozilla.org/integration/autoland/rev/700e7a6894f7 part 2 - XULContentSinkImpl::ContextStack::Push is infallible. r=bzbarsky https://hg.mozilla.org/integration/autoland/rev/66bc5ea2691b part 3 - Use refptrs in more places in XULContentSinkImpl. r=bzbarsky https://hg.mozilla.org/integration/autoland/rev/e50e14aed7e4 part 4 - Make dtors for nsXULPrototypeNode subclasses private. r=bzbarsky
Comment 9•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/aafcf88cbefc
https://hg.mozilla.org/mozilla-central/rev/700e7a6894f7
https://hg.mozilla.org/mozilla-central/rev/66bc5ea2691b
https://hg.mozilla.org/mozilla-central/rev/e50e14aed7e4
Description
•