Bug 182496 – bmalloc::PerProcess<T> is not really per-process, can lead to mistakes / duplication (IsoTLSDeallocatorEntry<IsoConfig<40u>>)

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: bmalloc

[Joseph Pecoraro]

bmalloc::PerProcess<T> is not really per-process, can lead to mistakes / duplication

The templated type doesn't export symbols, so each image (library / framework) that includes code with PerProcess<T> gets its own storage and its own instance.

The only existing duplication right now is PerProcess<IsoTLSDeallocatorEntry<IsoConfig<40u>>> gets one instance in JavaScriptCore and one in WebCore:

    JavaScriptCore`bmalloc::PerProcess<bmalloc::IsoTLSDeallocatorEntry<bmalloc::IsoConfig<40u>>>
    WebCore`bmalloc::PerProcess<bmalloc::IsoTLSDeallocatorEntry<bmalloc::IsoConfig<40u>>>

See also in bug 182474 where PerProcess<Scavenger> created multiple instances (again 1 from JSC and WebCore). This case was noticed because the constructor spawns its own thread so the duplication was causing multiple bmalloc scavenger threads.

[Radar WebKit Bug Importer]

<rdar://problem/37243492>

[Michael Catanzaro]

This is basically identical to bug #179914. I wonder why you never saw crashes on Mac. I assumed it wasn't a problem on Mac because you don't use a symbol map to hide symbols that would otherwise be exported, but I didn't consider that the symbols aren't exported at all.

PerProcess would be better-named PerLibrary or PerFramework, but that would be really terrible because every port makes completely different decisions about which WebKit subprojects are built into which shared libraries. It's safe to use everywhere on WPE, and at WebCore levels and above for GTK, but only in WebKit and WebKitLegacy on Mac; that's way too confusing.

I guess I don't understand why you created SafePerProcess that works properly (good job with that ;) and leaving PerProcess unchanged and broken, instead of just fixing PerProcess. The symbols do have to be exported for it to work (unless the symbol is used only in the toplevel WebKit subproject). So unless you have a better idea, my suggestion is to delete PerProcess, rename SafePerProcess to PerProcess, and change all use of PerProcess to use DECLARE/DEFINE_SAFE_PER_PROCESS_STORAGE.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #2)
> It's safe to use everywhere on WPE, and at WebCore levels and above for GTK, but
> only in WebKit and WebKitLegacy on Mac; that's way too confusing.

Well, that would be true if we were using -fvisibility=hidden, but currently we don't, so everything is exported, then hidden by the symbol map. (That's why GTK is fine right now.)

[Joseph Pecoraro]

(In reply to Michael Catanzaro from comment #2)
> This is basically identical to bug #179914. I wonder why you never saw
> crashes on Mac. I assumed it wasn't a problem on Mac because you don't use a
> symbol map to hide symbols that would otherwise be exported, but I didn't
> consider that the symbols aren't exported at all.

Whoa! Very interesting, that is exactly what we were seeing.

> PerProcess would be better-named PerLibrary or PerFramework, but that would
> be really terrible because every port makes completely different decisions
> about which WebKit subprojects are built into which shared libraries. It's
> safe to use everywhere on WPE, and at WebCore levels and above for GTK, but
> only in WebKit and WebKitLegacy on Mac; that's way too confusing.

Agreed.


> I guess I don't understand why you created SafePerProcess that works
> properly (good job with that ;) and leaving PerProcess unchanged and broken,
> instead of just fixing PerProcess. The symbols do have to be exported for it
> to work (unless the symbol is used only in the toplevel WebKit subproject).
> So unless you have a better idea, my suggestion is to delete PerProcess,
> rename SafePerProcess to PerProcess, and change all use of PerProcess to use
> DECLARE/DEFINE_SAFE_PER_PROCESS_STORAGE.

The problem with this was that SafePerProcess<T> requires the macro declarations for each of its specializations. For IsoTLSDeallocatorEntry there are currently 53 that are subject to change if the sizeof JavaScriptCore / WebCore classes change, and that list may grow if more types are created. That is unmaintainable for bmalloc to know about all the users outside of it. And I don't know how to create an ideal solution.

The only idea I had was moving off of templates, and moving to singletons / lookups.

[Michael Catanzaro]

Hm, looking at the symbol map in r227057, I expect you will have multiple copies of at least bmalloc::PerProcess<bmalloc::IsoTLSDeallocatorEntry<bmalloc::IsoConfig<40u> > >. (So why does it not crash?) But that map is only comprehensive for WebKitGTK+; Mac ports uses separate shared libs for each framework, which means you might have more duplicated symbols than we do.

[Michael Catanzaro]

Sorry: immediately after posting that comment, when the page refreshed, I noticed that symbol is in the title of the bug, and you discuss it in the first comment. I really shouldn't Bugzilla this late at night!

[Joseph Pecoraro]

I think this was fixed in bug 183545.

*** This bug has been marked as a duplicate of bug 183545 ***