RgnRectMemoryAllocator fails thread-safety assertions even when used only on non-main thread

RESOLVED FIXED

Status

()

Core
Graphics
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: cjones, Assigned: cjones)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

RgnRectMemoryAllocator wants to run only on a single thread, presumably so that it can avoid locking overhead.  It does this by saving the thread on which it was created and asserting that its methods are only invoked on that thread.

The use of RgnRectMemoryAllocator in nsRegion presents a problem, however.  nsRegion uses a static RgnRectMemoryAllocator, which is thus always initialized on the main thread.  This means that embedders who want to use libxul on a thread that's not main have to deal with heaps of assertion failure errors, even though that code is only being used on that non-main thread.  Electrolysis is one such embedder.

I don't know the best way to solve this issue.  I'll go fishing with my first patch.
Created attachment 388426 [details] [diff] [review]
Adds a |nsRegion::MigrateToCurrentThread()| class method

I'm using this patch in Electrolysis by invoking it when our "Gecko" thread is initialized in the child process.
Attachment #388426 - Flags: review?(vladimir)
Created attachment 388433 [details] [diff] [review]
Use the Migrate() function added by the previous patch in gfxPlatform::Init()

Corrected to bz's original idea.
Attachment #388426 - Attachment is obsolete: true
Attachment #388433 - Flags: review?(vladimir)
Attachment #388426 - Flags: review?(vladimir)
So if that works... why not just make this heap-allocated (via new) from inside gfxPlatform::Init()?
(In reply to comment #3)
> So if that works... why not just make this heap-allocated (via new) from inside
> gfxPlatform::Init()?

Two potential problems ... first, another statically-initialized variable is using RgnRectMemoryAllocator (potential tar baby in fixing that).  Second, changing when this mini-heap is allocated might cause some obscure performance issue.

What gfx performance tests should I look at to see if this change is detrimental?
Created attachment 388552 [details] [diff] [review]
Heap allocate the region allocator static, init in layoutstatics
Attachment #388433 - Attachment is obsolete: true
Attachment #388552 - Flags: review?(vladimir)
Attachment #388433 - Flags: review?(vladimir)
Pushed http://hg.mozilla.org/mozilla-central/rev/f5fe0e60f666
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Depends on: 515595
You need to log in before you can comment on or make changes to this bug.