MOZ_CAN_RUN_SCRIPT may require unnecessary refcount if a method returns pointer to itself as different type
Categories
(Developer Infrastructure :: Source Code Analysis, enhancement, P4)
Tracking
(Not tracked)
People
(Reporter: masayuki, Unassigned)
References
(Blocks 1 open bug)
Details
In some classes, we have methods as following:
> Bar* Foo::AsBar() const { return static_cast<Bar*>(this); }
If Foo (and Bar) is refcountable class and you do like as following, you'd see compile error, "arguments must all be strong refs or parent parameters when calling a function marked as MOZ_CAN_RUN_SCRIPT (including the implicit object argument)".
class Foo {
Bar* AsBar() const { return IsBar() ? static_cast<Bar*>(this) : nullptr; }
};
class Baz {
RefPtr<Foo> mFoo;
MOZ_CAN_RUN_SCRIPT void DoSomethingDangerous()
{
mFoo->AsBar()->DoSomethingDangerous();
}
This method needs to be written as:
{
RefPtr<Bar> bar = mFoo->AsBar();
bar->DoSomethingDangerous();
}
This redundant adding-reference may make damage to the performance if it's in a hot path.
So, perhaps, there is another attribute for mFoo::AsBar() which indicates that when it returns non-nullptr, it's point to same instance and MOZ_CAN_RUN_SCRIPT check should check whether the mFoo is grabbed with RefPtr or nsCOMPtr, or not.
Reporter | ||
Comment 1•5 years ago
|
||
If we won't add a template class like already_AddRefed
, using MOZ_KnownLive()
in each caller is the solution.
What do you think, bz?
![]() |
||
Comment 2•5 years ago
|
||
We can use MOZ_KnownLive for now, yes, but it would be better if this were handled automatically somehow. The problem is that automatic handling would need to catch cases like this:
auto* bar = foo->AsBar();
foo = baz;
bar->DoSomethingDangerous();
with the "foo = baz" possibly hidden inside other function calls, etc.
I don't have any bright ideas for automating this so far.
Updated•3 years ago
|
Updated•1 year ago
|
Updated•1 year ago
|
Comment 3•1 month ago
|
||
After bug 1843484, there will not be any comments left pointing to this bug. The issue described in comment 0 will still remain, though; but we may not have any code any more which runs into it.
Comment 4•1 month ago
|
||
Nevermind - bug 1843484 adds things like EventDispatcher::DispatchDOMEvent(MOZ_KnownLive(nsGlobalWindowInner::Cast(win)), ...)
which will point to this bug after all.
Description
•