[Skia] Implement a version of SkMemory for Mozilla that uses the infallible mozalloc allocators

RESOLVED FIXED in mozilla17

Status

()

Core
Graphics
--
major
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gw280, Assigned: gw280)

Tracking

(Depends on: 1 bug)

unspecified
mozilla17
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

This causes us to do invalid frees when we allocate skia objects inside 2d/. A simple fix is to set -DSK_OVERRIDE_GLOBAL_NEW for Android, but this may be indicative of a bigger build issue.
(Assignee)

Updated

5 years ago
Blocks: 725119
Are we building skia as a separate .so?
(In reply to Chris Jones [:cjones] [:warhammer] from comment #1)
> Are we building skia as a separate .so?

Nope. Linked statically.
Do all skia allocations get routed through a choke point?
(In reply to Chris Jones [:cjones] [:warhammer] from comment #3)
> Do all skia allocations get routed through a choke point?

Malloc does (http://mxr.mozilla.org/mozilla-central/source/gfx/skia/src/ports/SkMemory_malloc.cpp) but actually this bug is poorly named. I meant that new and delete goes through bionic's rather than using our overridden ones. Those are not overridden by Skia unless we set -DSK_OVERRIDE_GLOBAL_NEW, but this is only the case for Android as (on Linux at least) Mozilla seems to already override it for us.
Summary: [Skia] On Android, malloc is system malloc inside Skia, but mozilla's malloc outside → [Skia] On Android, operator new is system (bionic) new inside Skia, but mozilla's operator new outside
Including "mozalloc.h" in SkTypes.h should fix this, assuming that the SK_OVERRIDE_GLOBAL_NEW there indeed wraps all skia new/deletes.
Created attachment 618697 [details] [diff] [review]
Ensure that Skia uses our allocators instead of the system ones. This requires skia-npapi to be built later to ensure it can include mozalloc.h properly.
Attachment #618697 - Flags: review?(jones.chris.g)
Comment on attachment 618697 [details] [diff] [review]
Ensure that Skia uses our allocators instead of the system ones. This requires skia-npapi to be built later to ensure it can include mozalloc.h properly.

Review of attachment 618697 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/skia/include/config/SkUserConfig.h
@@ +41,5 @@
> +
> +#include "mozilla/mozalloc.h"
> +#define malloc moz_xmalloc
> +#define calloc moz_xcalloc
> +#define realloc moz_xrealloc

I think this is what mozalloc_macro_wrappers.h is for
(In reply to Ms2ger from comment #7)
> > +#include "mozilla/mozalloc.h"
> > +#define malloc moz_xmalloc
> > +#define calloc moz_xcalloc
> > +#define realloc moz_xrealloc
> 
> I think this is what mozalloc_macro_wrappers.h is for

That uses moz_malloc etc, rather than moz_xmalloc. My understanding is that we want to use infallible here.
Created attachment 619184 [details] [diff] [review]
Bug 740194 - Ensure that Skia uses our allocators instead of the system ones. This requires skia-npapi to be built later to ensure it can include mozalloc.h properly.
Attachment #619184 - Flags: review?(jones.chris.g)
(Assignee)

Updated

5 years ago
Attachment #618697 - Attachment is obsolete: true
Attachment #618697 - Flags: review?(jones.chris.g)
Oops, sign that I need to pack it in on a Friday night when I accidentally upload the same patch for review again... :)
Comment on attachment 619184 [details] [diff] [review]
Bug 740194 - Ensure that Skia uses our allocators instead of the system ones. This requires skia-npapi to be built later to ensure it can include mozalloc.h properly.

Including mozalloc.h here to override ::operator new/delete is what we want, but I think doing this #define hack is going to lead to pain.  Skia already makes a port-specific allocator impl easy for you; why not set all this up in an SkMemory_mozalloc.cpp port?
Attachment #619184 - Flags: review?(jones.chris.g)
I've filed a bug upstream at http://code.google.com/p/skia/issues/detail?id=598

That will take a while to get fixed as there's a lot of code that needs to be fixed, but until then I think we should go with Chris' suggestion to include mozalloc.h in SkTypes.h, but also implement an SkMemory_mozalloc.cpp instead of using the defines.
Created attachment 624122 [details] [diff] [review]
Bug 740194 - Ensure that Skia uses our allocators instead of the system ones. This requires skia-npapi to be built later to ensure it can include mozalloc.h properly.
Attachment #624122 - Flags: review?(jones.chris.g)
(Assignee)

Updated

5 years ago
Attachment #619184 - Attachment is obsolete: true
Comment on attachment 624122 [details] [diff] [review]
Bug 740194 - Ensure that Skia uses our allocators instead of the system ones. This requires skia-npapi to be built later to ensure it can include mozalloc.h properly.

>diff --git a/gfx/skia/src/ports/SkMemory_mozalloc.cpp b/gfx/skia/src/ports/SkMemory_mozalloc.cpp

>+
>+/*
>+ * Copyright 2011 Google Inc.
>+ * Copyright 2012 Mozilla Foundation

Are you going to try to upstream this?  If so, then I'm not sure
Google would accept code copyrighted by Moz.  We should ask.

If not, use the MPL2 for this file.

>+void sk_throw() {
>+    SkDEBUGFAIL("sk_throw");
>+    abort();

Use mozalloc_abort().

>+void sk_out_of_memory(void) {
>+    SkDEBUGFAIL("sk_out_of_memory");
>+    abort();

mozalloc_handle_oom()

>+void* sk_realloc_throw(void* addr, size_t size) {
>+    if (size == 0) {
>+        return p;
>+    }
>+    if (p == NULL) {
>+        sk_throw();
>+    }

moz_xrealloc() does all this for you.

>+void sk_free(void* p) {
>+    if (p) {
>+        moz_free(p);

moz_free() does this check for you.

>+void* sk_malloc_flags(size_t size, unsigned flags) {
>+    void* p = moz_xmalloc(size);
>+    if (p == NULL) {

moz_xmalloc() never returns NULL.  I think you want to do

  return (flags & SK_MALLOC_THROW) ? moz_xmalloc() : moz_malloc();
Attachment #624122 - Flags: review?(jones.chris.g)

Updated

5 years ago
Depends on: 758010
Depends on: 758858
Created attachment 651844 [details] [diff] [review]
updated patch

Updated patch incorporating Chris' suggestions
Attachment #624122 - Attachment is obsolete: true
Attachment #651844 - Flags: review?(jones.chris.g)
(Assignee)

Updated

5 years ago
Summary: [Skia] On Android, operator new is system (bionic) new inside Skia, but mozilla's operator new outside → [Skia] Implement a version of SkMemory for Mozilla that uses the infallible mozalloc allocators
Attachment #651844 - Flags: review?(jones.chris.g) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2475674e105
Backed this out due to build bustage..
https://hg.mozilla.org/integration/mozilla-inbound/rev/da866a8a19e3
https://hg.mozilla.org/mozilla-central/rev/da866a8a19e3
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.