1609638 - Add a check to clang-plugin identifying unperformant uses of temporary RefPtr's

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, task)

Description

[Simon Giesecke [:sg] [he/him]]

Using a temporary RefPtr just to dereference it once (using operator* or operator->) involves a short-lived increment of the refcount. This may be a performance problem and actually not desired. However, this cannot be easily seen from the source code causing this, as dereferencing a temporary raw pointer returned from a function cannot be distinguished from a RefPtr.

A check should be added to clang-plugin which shows a performance warning for such cases, maybe excluding typical uses such as those involving a MozPromise.

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

In principle I agree that we should avoid touching the refcount when it's unnecessary...

But I'm afraid the checker you suggest would not be able to detect cases where refcounting is actually required (e.g., because the RefPtr'd object could be destroyed between the time a function returns its raw pointer and the time the caller dereferences it), so it could be too noisy in practice.
And then how would we teach the checker to ignore these cases?

Also, I don't think the "temporariness" of a returned RefPtr is important: A non-temporary RefPtr would be just as wasteful if the refcounting was not needed either.

Anyway, if it's not difficult to implement, it could still be a good exercise to perform once or semi-regularly, to get an idea of how many "bad" cases exist.

Comment 2

[Simon Giesecke [:sg] [he/him]]

(In reply to Gerald Squelart [:gerald] (he/him) from comment #1)

In principle I agree that we should avoid touching the refcount when it's unnecessary...

But I'm afraid the checker you suggest would not be able to detect cases where refcounting is actually required (e.g., because the RefPtr'd object could be destroyed between the time a function returns its raw pointer and the time the caller dereferences it), so it could be too noisy in practice.
And then how would we teach the checker to ignore these cases?

Surely this is not something that could bring an auto-fix with, since it needs case-by-case judgment.

From a quick test (completely deleting the operators for temporaries), I found that there are around 500 locations in the code that do this. A significant number of those uses MozPromise::Then, which is a case which requires refcounting AFAIU. This could be hardcoded as an exception to reduce noise. I will check the exact number of remaining cases. Maybe there are one or two more which can be excepted to lower noise to an acceptable level.

Also, I don't think the "temporariness" of a returned RefPtr is important: A non-temporary RefPtr would be just as wasteful if the refcounting was not needed either.

It is important in the sense that with a member function RefPtr<X> GetX() in the following code:

this->GetX()->DoSomething();

it is not at all obvious that a RefPtr is involved, and the chance is high-ish that this is unintentional.

With

RefPtr<X> x = GetX();
x->DoSomething();

it is visible, and chances are lower that this is unintentional. It's still possible, of course, but I didn't want to claim that the suggested analysis is a silver bullet for the issue.

Using auto it might become non-obvious again:

auto x = GetX();
x->DoSomething();

This might be caught by an additional analysis.

Anyway, if it's not difficult to implement, it could still be a good exercise to perform once or semi-regularly, to get an idea of how many "bad" cases exist.

Comment 3

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1609638 - Add analysis identifying dereferences of temporary RefPtr objects. r=andi

Comment 4

[Simon Giesecke [:sg] [he/him]]

My initial implementation of the check, which ignores any cases of RefPtr<MozPromise> identifies 62 unique code locations across the codebase.

Comment 5

[Simon Giesecke [:sg] [he/him]]

One example is this one:

 7:52.86 /home/simon/work/refactor/gfx/ipc/VsyncBridgeChild.cpp:28:23: warning: Performance warning: do not dereference a temporary RefPtr of type 'RefPtr<nsIThread>'
 7:52.86   aThread->GetThread()->Dispatch(task.forget(), nsIThread::DISPATCH_NORMAL);
 7:52.86                       ^
 7:52.91 1 warning generated.

where https://searchfox.org/mozilla-central/source/gfx/ipc/VsyncIOThreadHolder.h#24 might be changed to dereference the RefPtr and return a reference instead (the calling code already assumes that mThread is not nullptr).

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Nice one, thank you for pursuing this, sorry for the negativity. 👏

Comment 7

[Simon Giesecke [:sg] [he/him]]

(In reply to Gerald Squelart [:gerald] (he/him) from comment #6)

Nice one, thank you for pursuing this, sorry for the negativity. 👏

Never mind, didn't feel that way :)

Comment 8

[Andi [:andi]]

Simon when you think the checker is ready for review feel free to r? me.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3b2013abdcbc
Add analysis identifying dereferences of temporary RefPtr objects. r=andi

Comment 10

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/3b2013abdcbc

Comment 11

[(Away)]

Do you intend to fix existing issues and/or make this warning fatal in the future? Otherwise it's adding a lot of noise to build logs, and I suspect it's not going to stop more instances of this antipattern from being landed.

Comment 12

[Simon Giesecke [:sg] [he/him]]

(In reply to :dmajor from comment #11)

Do you intend to fix existing issues and/or make this warning fatal in the future? Otherwise it's adding a lot of noise to build logs, and I suspect it's not going to stop more instances of this antipattern from being landed.

Thanks for bringing this up! It was not my intention to introduce noise. Do all (CI) builds enable the clang-plugin? One immediate option might be to enable the check only in clang-tidy for now.

If there is agreement that this should never be done, we can surely make this an error once all existing occurrences have been solved. Technically, it can be done in all cases. I was not sure, if I should do this myself, since it is not always obvious if changing the function type to a raw reference is safe, or if a strong reference/RefPtr must actually be held in a kungFuDeathGrip variable. I can work on this though, and maybe create individual patches for each affected module, so that they can be separately reviewed. Then we can come to a situation soon where this check can be made fatal, and no new instances can be introduced.

Comment 13

[(Away)]

(In reply to Simon Giesecke [:sg] [he/him] from comment #12)

Great, thanks!

Do all (CI) builds enable the clang-plugin?

There are some one-off exceptions (like mac asan) but almost all CI builds do enable the plugin, yes.

Comment 14

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Simon Giesecke [:sg] [he/him] from comment #5)

One example is this one:

 7:52.86 /home/simon/work/refactor/gfx/ipc/VsyncBridgeChild.cpp:28:23: warning: Performance warning: do not dereference a temporary RefPtr of type 'RefPtr<nsIThread>'
 7:52.86   aThread->GetThread()->Dispatch(task.forget(), nsIThread::DISPATCH_NORMAL);
 7:52.86                       ^
 7:52.91 1 warning generated.

where https://searchfox.org/mozilla-central/source/gfx/ipc/VsyncIOThreadHolder.h#24 might be changed to dereference the RefPtr and return a reference instead (the calling code already assumes that mThread is not nullptr).

Simon, I'm glad the new warning found this bug. But notably, the bug found is not actually at the reported callsite.

In fact, once fixed as you suggest (changing GetThread() to return a RefPtr<nsIThread>&) there's no need to change the callsite.

So I claim this warning is wrong. See bug 1612721 comment 3 for more.

Comment 15

[Simon Giesecke [:sg] [he/him]]

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #14)

(In reply to Simon Giesecke [:sg] [he/him] from comment #5)

One example is this one:

 7:52.86 /home/simon/work/refactor/gfx/ipc/VsyncBridgeChild.cpp:28:23: warning: Performance warning: do not dereference a temporary RefPtr of type 'RefPtr<nsIThread>'
 7:52.86   aThread->GetThread()->Dispatch(task.forget(), nsIThread::DISPATCH_NORMAL);
 7:52.86                       ^
 7:52.91 1 warning generated.

where https://searchfox.org/mozilla-central/source/gfx/ipc/VsyncIOThreadHolder.h#24 might be changed to dereference the RefPtr and return a reference instead (the calling code already assumes that mThread is not nullptr).

Simon, I'm glad the new warning found this bug. But notably, the bug found is not actually at the reported callsite.

In fact, once fixed as you suggest (changing GetThread() to return a RefPtr<nsIThread>&) there's no need to change the callsite.

So I claim this warning is wrong. See bug 1612721 comment 3 for more.

It appears the message is ambiguous. It did't mean suggest to return a RefPtr<T>& but rather a T& (and then the callsite needs to be changed as well). It's just one option. Returning a RefPtr<T>& is not a good option, IMO. Maybe returning a const RefPtr<T>& is one under some circumstances (when some callers might need a copy of the RefPtr, but others do not, and for some reason one doesn't want multiple functions), but I am not sure if I would recommend that. Maybe the note is not the right place to provide exemplary suggestions, but it should link to a more verbose documentation.

Comment 16

[Eric Rahm [:erahm]]

Simon, what do you think about disabling this by default until we can decide if fixing all the instances is: a) feasible and b) completed? It's pretty noisy right now for folks who have the clang-plugin enabled.

It might be worth more discussion if this is overall desirable. I'd very much like to avoid encouraging a dangerous by default behavior where we encourage people through warnings to expose us to an exploitable UAF because there might be a perf win. Perhaps we can narrow this down to potentially safer situations (for example if the class implements thread-safe ref counting we almost certainly don't want to encourage handing out raw pointers / refs).

Comment 17

[Andi [:andi]]

:erahm i must reiterate that I also feel that we should back this out for the moment.

Comment 18

[Simon Giesecke [:sg] [he/him]]

Ok, I will provide a patch to disable it for now. Sorry for any annoyance. Eric also thanks for the hints on possible improvements to the check, I will keep that in mind for the future.

Comment 19

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1609638 - Disabled mozilla-performance-temp-refptr in default builds. r=andi