1506441 - MOZ_CAN_RUN_SCRIPT may require unnecessary refcount if a method returns pointer to itself as different type

Status: NEW

(Developer Infrastructure :: Source Code Analysis, enhancement, P4)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

[Boris Zbarsky [: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.

Comment 3

[Markus Stange [:mstange]]

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

[Markus Stange [:mstange]]

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