1121297 - Make VolatileBuffer threadsafe

Status: RESOLVED FIXED

(Core :: Memory Allocator, defect)

Description

[Seth Fowler [:seth] [:s2h]]

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.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Move VolatileBuffer into libxul

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.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Make VolatileBuffer threadsafe

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

Comment 3

[Seth Fowler [:seth] [:s2h]]

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 4

[Mike Hommey [:glandium]]

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 5

[Mike Hommey [:glandium]]

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.

Comment 6

[Seth Fowler [:seth] [:s2h]]

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

Comment 7

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Move VolatileBuffer into libxul

Addressed review comments.

Comment 8

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Make VolatileBuffer threadsafe

Rebased version of part 2.

Comment 9

[Seth Fowler [:seth] [:s2h]]

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

Comment 10

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Switch the VolatileBuffer tests to use GTest

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.

Comment 11

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Switch the VolatileBuffer tests to use GTest

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

Comment 12

[Seth Fowler [:seth] [:s2h]]

Here's a new try job:

https://tbpl.mozilla.org/?tree=Try&rev=4d713169d3a7

Comment 13

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Move VolatileBuffer into libxul

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.

Comment 14

[Seth Fowler [:seth] [:s2h]]

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 15

[Mike Hommey [:glandium]]

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.

Comment 16

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Move VolatileBuffer into libxul

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

Comment 17

[Seth Fowler [:seth] [:s2h]]

Pushed:

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

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 19

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Move VolatileBuffer into libxul

Looks like this needs to clobber.

Comment 20

[Seth Fowler [:seth] [:s2h]]

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

Comment 21

[Phil Ringnalda (:philor)]

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

Comment 22

[Seth Fowler [:seth] [:s2h]]

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 23

[Seth Fowler [:seth] [:s2h]]

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

Comment 24

[Lawrence Mandel [:lmandel] (use needinfo)]

Tracking 37 as I want to be alerted if this bug is reopened.

Comment 25

[Lawrence Mandel [:lmandel] (use needinfo)]

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+

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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