1733727 - Use StaticAutoPtr to manage lifetime of dynamically allocated global variables in layout

Status: RESOLVED FIXED

(Core :: Layout, task)

Description

[Daniel Holbert [:dholbert]]

Filing this bug to use StaticAutoPtr (a smart-pointer that's suitable for use as a static/global variable) to manage lifetime of some global variables in layout.

Background:
https://searchfox.org/mozilla-central/rev/c3d7964c593e0bedabea2fea0b35ba243cf9e696/xpcom/base/StaticPtr.h#17-36

This lets us remove a bunch of explicit delete statements, which reduces our audit surface for potential double-free bugs.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: Bug 1733727 part 1: Use smart pointers to manage lifetime of global variables in nsRefreshDriver.cpp. r?#layout-reviewers

This patch doesn't change behavior.

This switch to StaticAutoPtr lets us remove a handful of explicit 'delete'
statements, since StaticAutoPtr automatically deletes its contents atomically
when it gets cleared. This reduces the potential for double-free errors.

As part of this patch, I'm also converting one local variable to a smart
pointer (UniquePtr), since it takes ownership of data from one of the global
variables that I'm touching in ths patch, and we need to perform that
ownership-transfer without deleting the data.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: Bug 1733727 part 2: Use StaticAutoPtr to manage lifetime of various "sFoo" static global variables in layout. r?#layout-reviewers

This lets us remove a handful of explicit 'delete' statements, by making
deletion happen automatically when these variables are cleared.

Depends on D127335

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: Bug 1733727 part 3: Use StaticAutoPtr to manage lifetime of various "gFoo" static global variables in layout. r?#layout-reviewers

This lets us remove a handful of explicit 'delete' statements, by making
deletion happen automatically when these variables are cleared.

Depends on D127336

Comment 4

[Daniel Holbert [:dholbert]]

Note, I've broken this up into 3 parts:
In part (1) I've given nsRefreshDriver its own patch since it has a tiny bit of extra complexity from the fact that it hands off the data from a global variable to a local variable.

In part (2), I'm handling variables that start with "s" (for static), which I discovered by grepping for delete s.

In part (3), I'm handling variables that start with "g" (for global), which I discovered by grepping for delete g.

Comment 5

[Daniel Holbert [:dholbert]]

Try run: https://treeherder.mozilla.org/jobs?repo=try&revision=69e34796ced304d8211399400d962ccf97d73ccd

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c270c9e129e3
part 1: Use smart pointers to manage lifetime of global variables in nsRefreshDriver.cpp. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/88d4b2d95850
part 2: Use StaticAutoPtr to manage lifetime of various "sFoo" static global variables in layout. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/376240aed4be
part 3: Use StaticAutoPtr to manage lifetime of various "gFoo" static global variables in layout. r=jfkthame

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/c270c9e129e3
https://hg.mozilla.org/mozilla-central/rev/88d4b2d95850
https://hg.mozilla.org/mozilla-central/rev/376240aed4be