1479232 - error: variable of type 'already_AddRefed<*>' is only valid as a temporary

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Mike Hommey [:glandium]]

Background:
- bug 1434689 added a static analysis for classes intended to be used for temporaries only.
- bug 1443265 marked already_AddRefed as such.

I was going a try build with the same revision of clang on osx as we use on windows to see if it would fix its LTO issues. Mac builds have the clang plugin enabled by default, and my build failed with:

[task 2018-07-28T23:48:58.705Z] 23:48:58     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/ChromeUtilsBinding.h:11:
[task 2018-07-28T23:48:58.705Z] 23:48:58     INFO -  /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/FakeString.h:130:31: error: variable of type 'already_AddRefed<nsStringBuffer>' is only valid as a temporary
[task 2018-07-28T23:48:58.705Z] 23:48:58     INFO -    void AssignFromStringBuffer(already_AddRefed<nsStringBuffer> aBuffer) {
[task 2018-07-28T23:48:58.707Z] 23:48:58     INFO -                                ^
[task 2018-07-28T23:48:58.708Z] 23:48:58     INFO -  /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/FakeString.h:130:31: note: value incorrectly allocated in an automatic variable
[task 2018-07-28T23:48:58.709Z] 23:48:58     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/security/certverifier/Unified_cpp_certverifier0.cpp:110:
[task 2018-07-28T23:48:58.710Z] 23:48:58     INFO -  In file included from /builds/worker/workspace/build/src/security/certverifier/NSSCertDBTrustDomain.cpp:28:
[task 2018-07-28T23:48:58.711Z] 23:48:58     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:20:
[task 2018-07-28T23:48:58.711Z] 23:48:58     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIThread.h:10:
[task 2018-07-28T23:48:58.712Z] 23:48:58     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/nsISerialEventTarget.h:10:
[task 2018-07-28T23:48:58.713Z] 23:48:58     INFO -  /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIEventTarget.h:60:23: error: variable of type 'already_AddRefed<nsIRunnable>' is only valid as a temporary
[task 2018-07-28T23:48:58.713Z] 23:48:58     INFO -    NS_IMETHOD Dispatch(already_AddRefed<nsIRunnable> event, uint32_t flags = DISPATCH_NORMAL) = 0;
[task 2018-07-28T23:48:58.713Z] 23:48:58     INFO -                        ^
[task 2018-07-28T23:48:58.714Z] 23:48:58     INFO -  /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIEventTarget.h:60:23: note: value incorrectly allocated in an automatic variable
[task 2018-07-28T23:48:58.714Z] 23:48:58     INFO -  /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIEventTarget.h:66:30: error: variable of type 'already_AddRefed<nsIRunnable>' is only valid as a temporary
[task 2018-07-28T23:48:58.714Z] 23:48:58     INFO -    NS_IMETHOD DelayedDispatch(already_AddRefed<nsIRunnable> event, uint32_t delay) = 0;
[task 2018-07-28T23:48:58.714Z] 23:48:58     INFO -                               ^
[task 2018-07-28T23:48:58.714Z] 23:48:58     INFO -  /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIEventTarget.h:66:30: note: value incorrectly allocated in an automatic variable
[task 2018-07-28T23:48:58.714Z] 23:48:58     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/security/certverifier/Unified_cpp_certverifier0.cpp:110:
[task 2018-07-28T23:48:58.714Z] 23:48:58     INFO -  In file included from /builds/worker/workspace/build/src/security/certverifier/NSSCertDBTrustDomain.cpp:28:
[task 2018-07-28T23:48:58.714Z] 23:48:58     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:20:
[task 2018-07-28T23:48:58.714Z] 23:48:58     INFO -  /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIThread.h:51:27: error: variable of type 'already_AddRefed<nsIRunnable>' is only valid as a temporary
[task 2018-07-28T23:48:58.714Z] 23:48:58     INFO -    NS_IMETHOD IdleDispatch(already_AddRefed<nsIRunnable> event) = 0;
[task 2018-07-28T23:48:58.714Z] 23:48:58     INFO -                            ^
[task 2018-07-28T23:48:58.714Z] 23:48:58     INFO -  /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIThread.h:51:27: note: value incorrectly allocated in an automatic variable
[task 2018-07-28T23:48:58.714Z] 23:48:58     INFO -  4 errors generated.
[task 2018-07-28T23:48:58.714Z] 23:48:58     INFO -  /builds/worker/workspace/build/src/config/rules.mk:1052: recipe for target 'Unified_cpp_certverifier0.o' failed
[task 2018-07-28T23:48:58.714Z] 23:48:58     INFO -  make[4]: *** [Unified_cpp_certverifier0.o] Error 1

Here's the weird thing though: those files have been using alreadyAddRefed this way since well before those bugs landed.

Considering the error is happening on method declarations rather than calls, it would seem the static analysis is broken somehow with newer versions of clang.

Comment 1

[Nathan Froyd [:froydnj]]

I saw errors like that in bug 1428158 comment 10 (static analysis for Android), but I think they went away after fixing some mismatches between what configure thought clang supported, AST-wise, and what the static analysis actually needed to look for (cf bug 1454667).  Maybe there's a mismatch between host and target clangs, or something?

Comment 2

[Mike Hommey [:glandium]]

This also (unsurprisingly) happens if I upgrade clang to version 7rc3 on Linux builds.

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Andi, when you are back from pto, could you please help Mike with that?

Comment 4

[Mike Hommey [:glandium]]

Attachment: Bug 1479232 - Only use one matcher for varDecl and parmVarDecl

ParmVarDecl being a subclass of VarDecl, using two matchers then caused
ScopeChecker::check to be called twice for ParmVarDecl nodes, once for
each match. But the code in ScopeCheck::check is written with the
assumption that it's called only once for such nodes.

Somehow, this didn't cause problems with clang up to version 6, but
makes the plugin spuriously warn about already_AddRefed not being used
as temporaries when used as argument in function declarations, with
clang 7.

Comment 5

[Andi [:andi]]

Comment on attachment 9010540 [details]
Bug 1479232 - Only use one matcher for varDecl and parmVarDecl

Andi-Bogdan Postelnicu [:andi] has approved the revision.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/65cd5f435d68
Only use one matcher for varDecl and parmVarDecl r=andi

Comment 7

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/65cd5f435d68