Closed Bug 1536736 Opened 8 months ago Closed 8 months ago

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

Categories

(Firefox Build System :: Source Code Analysis, enhancement)

enhancement
Not set

Tracking

(firefox68 fixed)

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: masayuki, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.

Component: DOM: Core & HTML → XPCOM
Component: XPCOM → Source Code Analysis
Product: Core → Firefox Build System

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?

Flags: needinfo?(nfroyd)

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

Assignee: nobody → bzbarsky
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f9df48cfea43
Allow constexpr things in the MOZ_CAN_RUN_SCRIPT analysis.  r=andi
Backout by cbrindusan@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

(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?

Flags: needinfo?(nfroyd)

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...

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