Closed Bug 1454052 Opened 6 years ago Closed 6 years ago

immediately destroyed MakeScopeExit() is a bit of a footgun

Categories

(Core :: MFBT, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: bkelly, Assigned: froydnj)

Details

Attachments

(1 file)

Recently, I foolishly wrote this:

  MakeScopeExit([&] {
    // update state
  });

  // do stuff assuming old state

And it blew up.  The problem was that I didn't assign the return value from MakeScopeExit().

Is there anything we could do to make this not compile or complain loudly?
The mechanism I was thinking about using here doesn't work, because MakeScopeExit is a function, not a class (ScopeExit uses MOZ_GUARD_OBJECT* already, but nobody uses ScopeExit directly).  I think adding MOZ_MUST_USE to MakeScopeExit will work, though I'm not sure if the compiler trivially considers the returned object "used", as its destructor needs to execute.  Trying that now.
(In reply to Nathan Froyd [:froydnj] from comment #1)
> The mechanism I was thinking about using here doesn't work, because
> MakeScopeExit is a function, not a class (ScopeExit uses MOZ_GUARD_OBJECT*
> already, but nobody uses ScopeExit directly).  I think adding MOZ_MUST_USE
> to MakeScopeExit will work, though I'm not sure if the compiler trivially
> considers the returned object "used", as its destructor needs to execute. 
> Trying that now.

Adding MOZ_MUST_USE doesn't work, unfortunately. :(  We'd need something stronger: we have plans/desires to reimplement MOZ_GUARD_OBJECT* using a static analysis,  for instance.  But since MOZ_GUARD_OBJECT* isn't doing the right thing here, handling MakeScopeExit would need a slightly stronger analysis.

I think the right thing would happen if you instantiated ScopeExit directly, but that's a weird corner case, and by far the idiomatic thing is MakeScopeExit.  We could just remove MakeScopeExit and convert everything to use explicit ScopeExits instead?  I think that runs into some typing issues with using lambdas, though, and to avoid those, we'd need to start using std::function, which might not be as efficient as the current code.  Maybe it doesn't matter too much?
I don't think this issue is that critical.  Its more just something I found myself wasting some hours on and thought I would file in case we could prevent others from running into the same thing.
(In reply to Nathan Froyd [:froydnj] from comment #2)
> Adding MOZ_MUST_USE doesn't work, unfortunately. :(

This sounded wrong (not you, the compiler!), so I gave it a quick try before reporting a clang issue, and it worked for me in a small test: https://godbolt.org/g/Hsfyxa

And in m-c on Mac, it immediately found this doozy:
https://searchfox.org/mozilla-central/source/xpcom/threads/PrioritizedEventQueue.cpp#188

So I think we (Moz) must use MOZ_MUST_USE. ;-)
Huh!  Maybe I wasn't finding the right error in a sea of errors, or maybe I should have been compiling with warnings-as-errors.
Otherwise, one can do thinkos like:

  MakeScopeExit(...);

and the scope exiting function will execute much earlier than the
intended:

  auto guard = MakeScopeExit(...);
Attachment #8968526 - Flags: review?(gsquelart)
Assignee: nobody → nfroyd
Comment on attachment 8968526 [details] [diff] [review]
make MakeScopeExit a MOZ_MUST_USE function

Review of attachment 8968526 [details] [diff] [review]:
-----------------------------------------------------------------

r+ after learning the word "thinko". :-)
Attachment #8968526 - Flags: review?(gsquelart) → review+
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b49bd215b26e
make MakeScopeExit a MOZ_MUST_USE function; r=gerald
https://hg.mozilla.org/mozilla-central/rev/b49bd215b26e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.