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

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: 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.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Use the Migrate() function added by the previous patch in gfxPlatform::Init()

Corrected to bz's original idea.

Comment 3

[Boris Zbarsky [:bzbarsky]]

So if that works... why not just make this heap-allocated (via new) from inside gfxPlatform::Init()?

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Huh.  What's the other variable?

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Heap allocate the region allocator static, init in layoutstatics

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Pushed http://hg.mozilla.org/mozilla-central/rev/f5fe0e60f666