Closed Bug 1121297 Opened 10 years ago Closed 10 years ago

Make VolatileBuffer threadsafe


(Core :: Memory Allocator, defect)

Not set



Tracking Status
firefox36 --- unaffected
firefox37 + fixed
firefox38 --- fixed


(Reporter: seth, Assigned: seth)




(2 files, 7 obsolete files)

We are currently experiencing a lot of bugs in ImageLib because, as we move things off-main-thread, we're starting to access VolatileBuffer objects from multiple threads at once. Unforunately, VolatileBuffer isn't threadsafe, and we can't easily work around the issue in ImageLib for a variety of reasons. The best solution is to make VolatileBuffer threadsafe, especially since ImageLib is the only place where VolatileBuffers are used right now.
This patch moves VolatileBuffer into libxul, which will make writing part 2 a
lot easier since we can use a platform-agnostic mutex.

It's worth reviewing |volatilebuffer/| carefully, as there were a
couple of things I copied from |mozalloc/| without knowing what they
Attachment #8548592 - Flags: review?(mh+mozilla)
This patch adds a mutex to VolatileBuffer and updates all of the implementations
to use it.
Attachment #8548593 - Flags: review?(mh+mozilla)
Given that there are different implementations per-platform and this code is touched by anything that draws images, I think we're going to need a pretty complete try job on this one:
Comment on attachment 8548592 [details] [diff] [review]
(Part 1) - Move VolatileBuffer into libxul

Review of attachment 8548592 [details] [diff] [review]:

Don't copy, you don't need DIST_INSTALL in that directory.

::: memory/volatilebuffer/
@@ -68,4 @@
>  TEST_DIRS += ['tests']
>  GENERATED_INCLUDES += ['/xpcom']

This can probably be removed.

@@ -69,5 @@
>  TEST_DIRS += ['tests']
>  GENERATED_INCLUDES += ['/xpcom']

This can definitely be removed.

@@ -71,5 @@
>  GENERATED_INCLUDES += ['/xpcom']

You should check if this condition can be lifted in either memory/volatilebuffer or memory/mozalloc.

@@ +43,5 @@
>          DIRS += [
>              'mozglue',
>              'memory/mozalloc',
> +            'memory/volatilebuffer',

make it memory/volatile.
Attachment #8548592 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8548593 [details] [diff] [review]
(Part 2) - Make VolatileBuffer threadsafe

Review of attachment 8548593 [details] [diff] [review]:

::: memory/volatilebuffer/VolatileBuffer.h
@@ +41,5 @@
>   */
>  namespace mozilla {
> +class MOZALLOC_EXPORT VolatileBuffer

Ah, here is something that should be done in part 1: remove MOZALLOC_EXPORT from these files.
Attachment #8548593 - Flags: review?(mh+mozilla) → review+
Thanks for the reviews! I'll make those changes.
Addressed review comments.
Attachment #8548592 - Attachment is obsolete: true
Rebased version of part 2.
Attachment #8548593 - Attachment is obsolete: true
Let's double check that this still builds everywhere, since I changed the warnings-as-errors behavior:
I've attempted to switch the tests over to GTest to fix the build errors on that
try job, but I haven't had any luck getting the build system to pick the new
tests up.
Attachment #8549413 - Attachment is obsolete: true
OK, this version should address the issues in the previous version of part 3.
Attachment #8549416 - Flags: review?(mh+mozilla)
Attachment #8549416 - Attachment description: 1121297-switch-the-volatilebuffer-test-to-use-gtest.patch → (Part 3) - Switch the VolatileBuffer tests to use GTest
Updated this patch to check the return value of posix_memalign /
moz_posix_memalign, because not doing so led to a warning that caused the
valgrind build to fail.
Attachment #8549145 - Attachment is obsolete: true
One more try job, to make sure that the valgrind build now works correctly. (And that I haven't inadvertently busted anything with the change in comment 13.)
Comment on attachment 8549416 [details] [diff] [review]
(Part 3) - Switch the VolatileBuffer tests to use GTest

Review of attachment 8549416 [details] [diff] [review]:

This should be folded with part 1.
Attachment #8549416 - Flags: review?(mh+mozilla) → review+
Thanks for the review! Part 3 folded into part 1 as requested.
Attachment #8549875 - Attachment is obsolete: true
Attachment #8549416 - Attachment is obsolete: true
Looks like this needs to clobber.
Attachment #8550548 - Attachment is obsolete: true
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment on attachment 8550597 [details] [diff] [review]
(Part 1) - Move VolatileBuffer into libxul

Approval Request Comment
[Feature/regressing bug #]: Bug 1116733 is what made this issue visible, though it existed before.
[User impact if declined]: Image textures suddenly disappearing or becoming corrupt despite being locked. Affects any platform where we use volatile buffers - Android, B2G, OS X, Windows.
[Describe test coverage new/current, TBPL]: Will have been on central for 4 days by the time it gets uplifted; plenty of time to detect any issues for such a simple change. This feature has test coverage.
[Risks and why]: Low risk.
[String/UUID change made/needed]: None.
Attachment #8550597 - Flags: approval-mozilla-aurora?
Comment on attachment 8549149 [details] [diff] [review]
(Part 2) - Make VolatileBuffer threadsafe

Part 2 needs to come along as well. (It does the real work!)
Attachment #8549149 - Flags: approval-mozilla-aurora?
Tracking 37 as I want to be alerted if this bug is reopened.
Comment on attachment 8550597 [details] [diff] [review]
(Part 1) - Move VolatileBuffer into libxul

It's early enough in Aurora to take a change like this. Aurora+
Attachment #8550597 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8549149 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.