Bug 273175 – [Skia] Use bmalloc directly instead of fastMalloc

• Status: REOPENED
• Component: WebKit Misc.

[Adrian Perez]

In issue #269572 we made Skia use WTF::fastMalloc() and friends,
but the only advantage of using those over the bmalloc::api::*
functions is that the WTF ones will fallback to plain malloc()
the build is configured with USE_SYSTEM_MALLOC=ON -- but in this
case we anyway build “SkMemory_malloc.cpp” so there is really
no gain in using WTF.

At the same time, many things pulled in by “wtf/FastMalloc.h” are
completely unneeded by Skia, so we would simplify the build a bit
(hopefully shaving a few seconds).

While at it, the custom allocator source should be in
“Source/ThirdParty/skia/src/ports/SkMemory_*.cpp” along with the
other ones. Maybe even contributed to upstream Skia =)

[Adrian Perez]

Pull request: https://github.com/WebKit/WebKit/pull/27930

[EWS]

Committed 278185@main (29160125ef02): <https://commits.webkit.org/278185@main>

Reviewed commits have been landed. Closing PR #27930 and removing active labels.

[Radar WebKit Bug Importer]

<rdar://problem/127317715>

[Adrian Perez]

Reopened Bugzilla.
Broke running tests with Skia enabled, tracking revert in https://bugs.webkit.org/show_bug.cgi?id=273767.

[Adrian Perez]

Pull request: https://github.com/WebKit/WebKit/pull/34058

[Adrian Perez]

(In reply to Adrian Perez from comment #5)
> Pull request: https://github.com/WebKit/WebKit/pull/34058

Okay, this made API tests fail, and I expect the post-commit bots would
have even *more* trouble... there are a number of crashes which were not happening before, see https://ews-build.webkit.org/#/builders/21/builds/62016
for an example.

The situation is, let's say complicated. From the POV “who uses Skia”, we
have an easy case:

- ImageDiff uses Skia, but this is a standalone usage (no WebCore nor
  other dependencies). No problem in making it use Skia w/malloc.

And the complicated ones:

- (WPE only) MiniBrowser uses WPEToolingBackends and in turn
  WPEToolingBackends use Skia.
- TestWebKitAPI also use WPEToolingBackends.
- WKTR uses Skia directly and also WPEToolingBackends.

In these three later cases we end up with more than one copy of bmalloc
being linked in, as it is an OBJECT library so its .o files get pulled
into the linker invocation for each involved target (bad idea!). Or with
part of Skia using bmalloc, other parts using malloc (terrible idea!).

Before trying to fix this issue (and also bug #280124), for developer
builds we would make everything that uses Skia pick the *headers* only
except for the WebCore and WebKit targets, so when linked the symbols
would be resolved from the libSkia.a passed as it was PUBLIC for
Web{Core,Kit} so it “bubbles up” when linking programs that depend on
them.

This made everything work by chance because ideally Skia should be
PRIVATE to the WebKit target! But that wasn't the case so “leaking”
the implementation detail worked as long as WebKit would expose all
symbols (true in developer builds). On release (non-developer) builds
we pass a linker script to hide non-public symbols, so linking Skia
again into them results in multiple copies of Skia as well.

The possible solutions I see are:

 - Workaround things enough to solve bug #280124 leaving the mess
   as-is for now, and fix the mess later.
 - Make Skia use system malloc, forget for now about integrating
   with WTF::fastMalloc/bmalloc for now and revisit later.

In both cases we would need to end up figuring out what to do to make
sure Skia can be left as a private implementation detail in non-developer
builds, and this would involve *avoiding* Skia in WPEViewBackends. For
developer builds I think it's okay to rely on the WebKit library exposing
all its symbols because that's already the assumption elsewhere (e.g. in
TestWebCore to access internals).

[Michael Catanzaro]

There's some relevant history in bug #179914