Make VolatileBuffer threadsafe

RESOLVED FIXED in Firefox 37

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

unspecified
mozilla38
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox36 unaffected, firefox37+ fixed, firefox38 fixed)

Details

Attachments

(2 attachments, 7 obsolete attachments)

Assignee

Description

4 years ago
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.
Assignee

Comment 1

4 years ago
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.
Attachment #8548592 - Flags: review?(mh+mozilla)
Assignee

Comment 2

4 years ago
This patch adds a mutex to VolatileBuffer and updates all of the implementations
to use it.
Attachment #8548593 - Flags: review?(mh+mozilla)
Assignee

Comment 3

4 years ago
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.
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+
Assignee

Comment 6

4 years ago
Thanks for the reviews! I'll make those changes.
Assignee

Comment 7

4 years ago
Addressed review comments.
Assignee

Updated

4 years ago
Attachment #8548592 - Attachment is obsolete: true
Assignee

Comment 8

4 years ago
Rebased version of part 2.
Assignee

Updated

4 years ago
Attachment #8548593 - Attachment is obsolete: true
Assignee

Comment 9

4 years ago
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
Assignee

Comment 10

4 years ago
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.
Assignee

Updated

4 years ago
Attachment #8549413 - Attachment is obsolete: true
Assignee

Comment 11

4 years ago
OK, this version should address the issues in the previous version of part 3.
Attachment #8549416 - Flags: review?(mh+mozilla)
Assignee

Updated

4 years ago
Attachment #8549416 - Attachment description: 1121297-switch-the-volatilebuffer-test-to-use-gtest.patch → (Part 3) - Switch the VolatileBuffer tests to use GTest
Assignee

Comment 13

4 years ago
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.
Assignee

Updated

4 years ago
Attachment #8549145 - Attachment is obsolete: true
Assignee

Comment 14

4 years ago
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.
Attachment #8549416 - Flags: review?(mh+mozilla) → review+
Assignee

Comment 16

4 years ago
Thanks for the review! Part 3 folded into part 1 as requested.
Assignee

Updated

4 years ago
Attachment #8549875 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8549416 - Attachment is obsolete: true
Assignee

Comment 19

4 years ago
Looks like this needs to clobber.
Assignee

Updated

4 years ago
Attachment #8550548 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/e10a79c4ac9f
https://hg.mozilla.org/mozilla-central/rev/26cdb770b821
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Assignee

Comment 22

4 years ago
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?
Assignee

Comment 23

4 years ago
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.