Closed Bug 740194 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: Graphics, defect)

ARM
Android
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: gw280, Assigned: gw280)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

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.
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.
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.
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.
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)
Depends on: 758010
Depends on: 758858
Attached patch updated patchSplinter Review
Updated patch incorporating Chris' suggestions
Attachment #624122 - Attachment is obsolete: true
Attachment #651844 - Flags: review?(jones.chris.g)
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+
Backed this out due to build bustage..
https://hg.mozilla.org/mozilla-central/rev/da866a8a19e3
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.