Closed Bug 715767 Opened 10 years ago Closed 9 years ago

LayerManagerD3D10::ReportFailure is not thread-safe but gets called off the main thread

Categories

(Core :: Graphics, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: roc, Assigned: drexler)

Details

(Whiteboard: [lang=c++][mentor=joe@drew.ca])

Attachments

(1 file)

It gets called by PlanarYCbCrImageD3D10::AllocateTextures off the main thread, but it calls gfx::LogFailure which uses nsIGfxInfo which is not thread-safe. At least, nsIGfxInfo does not use thread-safe refcounting.
Seems like this actually could be racy/crashy in release builds since GfxInfoBase::LogFailure is not thread-safe either. We'd better fix it!
Probably the best thing to do is to make GfxInfo refcounting thread-safe, and make LogFailure thread-safe.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> Probably the best thing to do is to make GfxInfo refcounting thread-safe,
> and make LogFailure thread-safe.

Yes please.
This should be pretty straightforward. The first step will be to make widget/xpwidgets/GfxInfoBase.cpp use the _THREADSAFE variants of the NS_DECL_ISUPPORTS*-variant macros defined in nsISupportsImpl.h. The second step will be to make sure that there aren't any issues with any of the GfxInfo derived types; I don't *think* I expect any, but I haven't audited what usage of GfxInfoBase the derived types use nowadays.

Finally, you'll need to add a Mutex member to GfxInfoBase, and lock it on entrance to GfxInfoBase::LogFailure().
Whiteboard: [lang=c++][mentor=joe@drew.ca]
Hi

I'd be extremely interested in working on it. Obviously if I can count on your help, when necessary.

BR
Attached patch patchSplinter Review
Assignee: nobody → andrew.quartey
Attachment #715224 - Flags: review?(joe)
Comment on attachment 715224 [details] [diff] [review]
patch

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

Lovely.
Attachment #715224 - Flags: review?(joe) → review+
Andrew, do you have try access? If so, can you push this to try?
(In reply to Joe Drew (:JOEDREW! \o/) from comment #8)
> Andrew, do you have try access? If so, can you push this to try?

Yes, i did before posting the patch: https://tbpl.mozilla.org/?tree=Try&rev=dd4be570b902.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0470103ac4a8
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.