Open Bug 1506441 Opened 6 years ago Updated 5 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)

Product:
Developer Infrastructure
Component:
Source Code Analysis
Type:
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)
Priority: -- → P4
Product: Firefox Build System → Developer Infrastructure
Severity: normal → S3

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.

Nevermind - bug 1843484 adds things like EventDispatcher::DispatchDOMEvent(MOZ_KnownLive(nsGlobalWindowInner::Cast(win)), ...) which will point to this bug after all.

You need to log in before you can comment on or make changes to this bug.