1091921 - The Auto helpers for Monitor, ReentrantMonitor, and Mutex should not make operator delete private

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Seth Fowler [:seth] [:s2h]]

The Auto helpers for Monitor, ReentrantMonitor, and Mutex currently do not define |operator new| and |operator delete|  to prevent you from allocating them on the heap.

That makes a lot of sense, but not defining |operator new| is enough to achieve this goal. Not defining |operator delete|, unfortunately, prevents these classes from being used with placement new, since that causes the compiler to generate a "deleting destructor" that calls |operator delete|. Even though we never call that destructor, it causes a link failure. See here for a detailed discussion of a very closely related problem:

http://ehsanakhgari.org/blog/2012-12-11/c-deleting-destructors

Concretely, I want to use these classes with Maybe, which is a pattern we use in many places in the codebase: we may optionally acquire a RAII resource, so we wrap it in a Maybe and only actually construct it if we acquire it. Maybe uses placement new internally, so it runs into this problem.

What I want to do is remove the undefined |operator delete| from all of the Auto helpers for these classes. There should be no downside to this change; heap allocation will still be prevented.

Comment 1

[Seth Fowler [:seth] [:s2h]]

This is an example of the link failure that you get right now with Maybe<ReentrantMonitorAutoEnter>:

https://tbpl.mozilla.org/php/getParsedLog.php?id=51462522&tree=Try#error0

Comment 2

[Seth Fowler [:seth] [:s2h]]

Attachment: Don't disable operator delete for Monitor and Mutex Auto helpers

Here's the patch.

Comment 3

[Nathan Froyd [:froydnj]]

Thanks for the detailed explanation!

My immediate reaction is one of concern, it seems like it'd be easy to do something like:

  Maybe<AutoLockMutex> maybeLock;

  if (Condition()) {
    maybeLock.emplace(mMutex);
  }

  // This always needs to be called under a lock
  DoSomethingRequiringALock(...);

and that Condition() might not always hold.  It's no longer RAII, but RA-under-some-condition-which-might-not-be-true-later-II.  I think I'd be a little less concerned about this if we had a strong culture of |const MutexAutoLock& aProofOfLock| parameters or similar.

Why not do the alternate approach of:

  if (Condition()) {
    return LockThingsFirst(...);
  }

  DoInterestingThings(...);

  LockThingsFirst(...) {
    MutexAutoLock lock(...);
    return DoInterestingThings(...);
  }

It's more typing, but it seems to me to make the intention clearer.

Comment 4

[Seth Fowler [:seth] [:s2h]]

I can definitely imagine that some people might be inclined to misuse this, but I think our system of code review is a good mitigation against such issues.

FWIW, in the particular circumstance that mitigated me to use this, image decoding, the normal case is that racy accesses to texture contents are explicitly allowed. In certain circumstances, however, I want to synchronize the main thread and the decoder thread - in particular, when synchronous decoding is required, I want to be sure that the decoder has decoded as much as it can with the current data it has. Thus the fact that accesses to texture contents may be racy without the lock and only synchronized with the lock is by design, and the people working on that code understand it.

I have experience with the transformation you suggest, and there are many situations in which it's appropriate, but for the code I'm working on, Maybe<ReentrantMonitorAutoEnter> is really the most straightforward implementation strategy, and the most maintainable for people familiar with that code. And I suspect it won't be the only such case that comes up in our codebase.

Comment 5

[Seth Fowler [:seth] [:s2h]]

I think it's also worth noting that in the case of ImageLib, it's unlikely that we'd call methods that require the monitor without knowing about it, since all such methods assert that they are in the monitor at the very beginning, or acquire it themselves. The number of such methods is also limited, and getting smaller as I continue to refactor things. I'd hope that we follow similar practices everywhere we use threads, though anything is possible in Gecko. =)

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Seth Fowler [:seth] from comment #5)
> I think it's also worth noting that in the case of ImageLib, it's unlikely
> that we'd call methods that require the monitor without knowing about it,
> since all such methods assert that they are in the monitor at the very
> beginning, or acquire it themselves. The number of such methods is also
> limited, and getting smaller as I continue to refactor things. I'd hope that
> we follow similar practices everywhere we use threads, though anything is
> possible in Gecko. =)

Oh, I'd be willing to bet ImageLib can do good things with this, I'm just not completely convinced about all the other Gecko modules out there. :)

That said, it does look like JS has maybe-able autolocks, so maybe this is not too awful.  Let's give it a whirl.  Would you mind posting to dev-platform about this new capability, with an encouraging word about sprinkling AssertCurrentThreadOwns or similar liberally around any code that uses Maybe<MutexAutoLock> and friends?

Comment 7

[Seth Fowler [:seth] [:s2h]]

Thanks for the review! Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea62f34c430d

I also sent a message to dev-platform to give people a heads up.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ea62f34c430d