1536736 - nsGkAtoms::* isn't considered as safe to use arguments of MOZ_CAN_RUN_SCRIPT methods

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, enhancement)

Description

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

nsGkAtoms::a of:

foo->CanRunScriptMethod(nsGkAtoms::a)

and also

foo->CanRunScriptMethod(*nsGkAtoms::a)

are considered as not safe. I think that MOZ_KnownLive() in these cases is redundant.

Comment 1

[Boris Zbarsky [:bzbarsky]]

We could probably hardcode nsGkAtoms, but I wonder whether it would make sense to generally allow a constexpr variable in the analysis. My understanding is that constexpr means the value is computed at compile time and then can't change, right? If that value is a refcounted thing, then presumably something fishy is going on with its refcounting (as it is in the case of static atoms), such that it's guaranteed-live, right?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1536736. Allow constexpr things in the MOZ_CAN_RUN_SCRIPT analysis. r=andi

Since these are compile-time constants, they can't exactly go away on us due to
running script, right?

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f9df48cfea43
Allow constexpr things in the MOZ_CAN_RUN_SCRIPT analysis.  r=andi

Comment 4

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d296a52a202d
Backed out 6 changesets (bug 1536736, bug 1536336, bug 1536719, bug 1536825, bug 1537537, bug 1536724) for build  bustages at TestCanRunScript. CLOSED TREE

Comment 5

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/664d49ea5275

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #1)

We could probably hardcode nsGkAtoms, but I wonder whether it would make sense to generally allow a constexpr variable in the analysis. My understanding is that constexpr means the value is computed at compile time and then can't change, right? If that value is a refcounted thing, then presumably something fishy is going on with its refcounting (as it is in the case of static atoms), such that it's guaranteed-live, right?

This analysis makes sense to me, since constexpr things are implicitly const. Note that C++20 (?) is going to allow non-const constexpr (constint) things, so you can compute the initial value at compile time, but then mutate the value at runtime. But presumably that will be exposed differently in the IR, so doesn't affect the analysis here?

Comment 7

[Boris Zbarsky [:bzbarsky]]

This analysis makes sense to me, since constexpr things are implicitly const

Right, but the cost thing is the value of the pointer, presumably, not the value the pointer points to? It's not entirely clear to me, actually in this case:

  static constexpr nsStaticAtom* name_;

But presumably that will be exposed differently in the IR, so doesn't affect the analysis here?

We're doing the analysis on the AST, not the IR. But hopefully it will look different in the AST, yes...