Closed Bug 862556 Opened 8 years ago Closed 8 years ago

Make nsChildView.mm thread safe

Categories

(Core :: Widget: Cocoa, defect)

21 Branch
x86_64
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: nrc, Unassigned)

References

Details

Attachments

(1 file)

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.
Summary: Make nsChildView.nm thread safe → Make nsChildView.mm thread safe
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.
Attached patch Thread safetySplinter Review
I think this fixes all possible races.
Attachment #738879 - Flags: review?(ncameron)
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"
Attachment #738879 - Flags: review?(ncameron) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/890ed498bae6
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.