Closed
Bug 1454052
Opened 6 years ago
Closed 6 years ago
immediately destroyed MakeScopeExit() is a bit of a footgun
Categories
(Core :: MFBT, enhancement)
Core
MFBT
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: bkelly, Assigned: froydnj)
Details
Attachments
(1 file)
2.16 KB,
patch
|
mozbugz
:
review+
|
Details | Diff | Splinter Review |
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?
Assignee | ||
Comment 1•6 years ago
|
||
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.
Assignee | ||
Comment 2•6 years ago
|
||
(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?
Reporter | ||
Comment 3•6 years ago
|
||
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. ;-)
Assignee | ||
Comment 5•6 years ago
|
||
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.
Assignee | ||
Comment 6•6 years ago
|
||
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
Comment 9•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b49bd215b26e
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•