Last Comment Bug 740194 - [Skia] Implement a version of SkMemory for Mozilla that uses the infallible mozalloc allocators
: [Skia] Implement a version of SkMemory for Mozilla that uses the infallible m...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: ARM Android
: -- major (vote)
: mozilla17
Assigned To: George Wright (:gw280) (:gwright)
:
Mentors:
Depends on: 758858 758010
Blocks: 687187 725119
  Show dependency treegraph
 
Reported: 2012-03-28 16:08 PDT by George Wright (:gw280) (:gwright)
Modified: 2012-08-16 06:17 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
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. (2.44 KB, patch)
2012-04-26 09:54 PDT, George Wright (:gw280) (:gwright)
no flags 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. (2.45 KB, patch)
2012-04-27 14:41 PDT, George Wright (:gw280) (:gwright)
no flags 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. (4.22 KB, patch)
2012-05-15 11:40 PDT, George Wright (:gw280) (:gwright)
no flags Details | Diff | Review
updated patch (3.27 KB, patch)
2012-08-14 11:55 PDT, George Wright (:gw280) (:gwright)
cjones.bugs: review+
Details | Diff | Review

Description George Wright (:gw280) (:gwright) 2012-03-28 16:08:13 PDT
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.
Comment 1 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-03-28 18:14:38 PDT
Are we building skia as a separate .so?
Comment 2 George Wright (:gw280) (:gwright) 2012-03-28 19:17:53 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #1)
> Are we building skia as a separate .so?

Nope. Linked statically.
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-03-28 19:24:04 PDT
Do all skia allocations get routed through a choke point?
Comment 4 George Wright (:gw280) (:gwright) 2012-03-28 19:32:51 PDT
(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.
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-03-28 19:39:39 PDT
Including "mozalloc.h" in SkTypes.h should fix this, assuming that the SK_OVERRIDE_GLOBAL_NEW there indeed wraps all skia new/deletes.
Comment 6 George Wright (:gw280) (:gwright) 2012-04-26 09:54:44 PDT
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.
Comment 7 :Ms2ger 2012-04-26 10:11:56 PDT
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
Comment 8 George Wright (:gw280) (:gwright) 2012-04-26 10:18:00 PDT
(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.
Comment 9 George Wright (:gw280) (:gwright) 2012-04-27 14:41:22 PDT
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.
Comment 10 George Wright (:gw280) (:gwright) 2012-04-27 14:42:46 PDT
Oops, sign that I need to pack it in on a Friday night when I accidentally upload the same patch for review again... :)
Comment 11 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-04-30 23:07:37 PDT
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?
Comment 12 George Wright (:gw280) (:gwright) 2012-05-15 11:10:27 PDT
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.
Comment 13 George Wright (:gw280) (:gwright) 2012-05-15 11:40:10 PDT
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.
Comment 14 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-23 15:06:57 PDT
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();
Comment 15 George Wright (:gw280) (:gwright) 2012-08-14 11:55:25 PDT
Created attachment 651844 [details] [diff] [review]
updated patch

Updated patch incorporating Chris' suggestions
Comment 16 George Wright (:gw280) (:gwright) 2012-08-15 14:10:43 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2475674e105
Comment 17 George Wright (:gw280) (:gwright) 2012-08-15 14:35:20 PDT
Backed this out due to build bustage..
Comment 18 George Wright (:gw280) (:gwright) 2012-08-15 21:18:23 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/da866a8a19e3
Comment 19 Ed Morley [:emorley] 2012-08-16 06:17:13 PDT
https://hg.mozilla.org/mozilla-central/rev/da866a8a19e3

Note You need to log in before you can comment on or make changes to this bug.