Open Bug 1506441 Opened 5 years ago Updated 8 months ago

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)

enhancement

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.

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?

Flags: needinfo?(bzbarsky)

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.

Flags: needinfo?(bzbarsky)
See Also: → 1691515
Priority: -- → P4
Product: Firefox Build System → Developer Infrastructure
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.