With OMTC, we call DrawWindowOverlay and DrawWindowUnderlay on the compositing thread, it looks like this might race in ShowsResizeIndicator. We should fix that.

From mattwoodrow, https://bugzilla.mozilla.org/show_bug.cgi?id=861490#c11 Could we trigger code to run every time that the result of ShowsResizeIndicator might change? Then we could compute this and cache it on the main thread, and just read it from the compositor thread.

An alternative idea is to create 'closures' on the main thread, and then run them on whatever the drawing thread is (main thread currently, compositor thread with OMTC). Something like: // Needs to be entirely self contained, or thread-safe. class WindowEffectsClosure { virtual ~WindowEffectsClosure(); virtual void DrawEffects(LayerManager* aManager, nsIntRect aRect); }; // Both called on the main thread from the shadowable manager. WindowEffectsClosure* CreateWindowOverlayClosure(); WindowEffectsClosure* CreateWindowUnderlayClosure(); I'm sure we can get thread-safety without doing this, but this seems much clearer and mistake-proof.

note we also have to release our TextureImage on the right thread too - see https://bugzilla.mozilla.org/show_bug.cgi?id=861490#c16

(In reply to Nick Cameron [:nrc] from comment #1) > Could we trigger code to run every time that the result of > ShowsResizeIndicator might change? By the way, that happens very rarely. On 10.7 and up, ShowsResizeIndicator is always false, and on 10.6 it can only change when a resizable window enters or exits fullscreen mode, otherwise it's constant per window.

I think this fixes all possible races.

Comment on attachment 738879 [details] [diff] [review] Thread safety Review of attachment 738879 [details] [diff] [review]: ----------------------------------------------------------------- LGTM ::: widget/cocoa/nsChildView.h @@ +599,5 @@ > nsRefPtr<gfxASurface> mTempThebesSurface; > + > + mozilla::Mutex mEffectsLock; > + > + // Both threads Please expand the comment a little - something along the lines of "May be called from any thread, protected by mEffectsLock"

Sorry I only come in to discuss that now, but are we allowed to use code blocks in Mac OS sources ? I've been using them in similar cases (related to CoreData, not UI), and found them really handy.

Code blocks (developer.apple.com/library/mac/#documentation/cocoa/Conceptual/Blocks/Articles/00_Introduction.html) are supported on OS X 10.6 and up. So yes, we can use them now that we've dropped support for OS X 10.5. Note, though, that they aren't yet widely used in our codebase, so we don't yet have much experience with them (or their bugs). And yes, they certainly *do* have bugs :-) Which means we should be cautious in our use of them, at least at first. We should slowly start using them, here and there, and keep an eye out for bugs. I'll post a reference to one bug we've already found in my next comment.

The code blocks bug I'm referring to is that, when block code is compiled using gcc/g++ (instead of clang), copying a C++ object into a block causes its destructor to be called prematurely (immediately after the copy is created). This is discussed at bug 682445 and bug 678607 (particularly bug 678607 comment #70). The discussion is pretty hard to read, though, because we kept changing our minds :-( This bug doesn't happen using clang (which we now use by default). And yes, Apple's stopped updating gcc/g++, so it's not too surprising to find bad bugs there. But if this kind of bug can exist in one of Apple's implementations of code blocks, there are surely others, and we need to keep our eyes open for them.