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/moz.build| carefully, as there were a couple of things I copied from |mozalloc/moz.build| without knowing what they do.

This patch adds a mutex to VolatileBuffer and updates all of the implementations to use it.

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: https://tbpl.mozilla.org/?tree=Try&rev=0384ac54a56a

Comment on attachment 8548592 [details] [diff] [review] (Part 1) - Move VolatileBuffer into libxul Review of attachment 8548592 [details] [diff] [review]: ----------------------------------------------------------------- Don't copy Makefile.in, you don't need DIST_INSTALL in that directory. ::: memory/volatilebuffer/moz.build @@ -68,4 @@ > > TEST_DIRS += ['tests'] > > GENERATED_INCLUDES += ['/xpcom'] This can probably be removed. @@ -69,5 @@ > TEST_DIRS += ['tests'] > > GENERATED_INCLUDES += ['/xpcom'] > > DISABLE_STL_WRAPPING = True This can definitely be removed. @@ -71,5 @@ > GENERATED_INCLUDES += ['/xpcom'] > > DISABLE_STL_WRAPPING = True > > if CONFIG['CLANG_CXX'] or CONFIG['_MSC_VER']: You should check if this condition can be lifted in either memory/volatilebuffer or memory/mozalloc. ::: moz.build @@ +43,5 @@ > > DIRS += [ > 'mozglue', > 'memory/mozalloc', > + 'memory/volatilebuffer', make it memory/volatile.

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.

Thanks for the reviews! I'll make those changes.

Addressed review comments.

Rebased version of part 2.

Let's double check that this still builds everywhere, since I changed the warnings-as-errors behavior: https://tbpl.mozilla.org/?tree=Try&rev=e3f0a2f2cdb8

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.

OK, this version should address the issues in the previous version of part 3.

Here's a new try job: https://tbpl.mozilla.org/?tree=Try&rev=4d713169d3a7

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.

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.) https://tbpl.mozilla.org/?tree=Try&rev=d1034a677feb

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.

Thanks for the review! Part 3 folded into part 1 as requested.

Pushed: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/005cf05954e7 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/28960bb167ef

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a7b0fe4de103 for Cpp unittest bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=5571306&repo=mozilla-inbound

Looks like this needs to clobber.

Pushed again now that inbound has reopened: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e10a79c4ac9f remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/26cdb770b821

https://hg.mozilla.org/mozilla-central/rev/e10a79c4ac9f https://hg.mozilla.org/mozilla-central/rev/26cdb770b821

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.

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!)

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+

https://hg.mozilla.org/releases/mozilla-aurora/rev/a783f8cccdd6 https://hg.mozilla.org/releases/mozilla-aurora/rev/8bffa3a6b4e1