Bug 181438 – REGRESSION(r226266): [GTK] RELEASE_ASSERT(reservedZoneSize >= minimumReservedZoneSize) in JSC::VM::updateStackLimits

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Guilaume Ayoub]

When I use Geary (a GTK-based mail client using WebKit-GTK), I get this trace:

1   0x7fbfa97e5c67 /usr/lib64/libjavascriptcoregtk-4.0.so.18(+0xaabc67) [0x7fbfa97e5c67]
2   0x7fbfa96e9ee5 /usr/lib64/libjavascriptcoregtk-4.0.so.18(+0x9afee5) [0x7fbfa96e9ee5]
3   0x7fbfa95d3bc7 /usr/lib64/libjavascriptcoregtk-4.0.so.18(+0x899bc7) [0x7fbfa95d3bc7]
4   0x7fbfa8ded210 /usr/lib64/libjavascriptcoregtk-4.0.so.18(JSValueIsNumber+0x20) [0x7fbfa8ded210]
5   0x67b24e geary(geary_js_to_number+0x2e) [0x67b24e]
6   0x5099a0 geary(web_kit_util_to_number+0x40) [0x5099a0]
7   0x49c94e geary() [0x49c94e]
8   0x7fbfb00b1ebd /usr/lib64/libgobject-2.0.so.0(g_closure_invoke+0x19d) [0x7fbfb00b1ebd]
9   0x7fbfb00c4ee3 /usr/lib64/libgobject-2.0.so.0(+0x22ee3) [0x7fbfb00c4ee3]
10  0x7fbfb00cd905 /usr/lib64/libgobject-2.0.so.0(g_signal_emit_valist+0xa65) [0x7fbfb00cd905]
11  0x7fbfb00ce2ca /usr/lib64/libgobject-2.0.so.0(g_signal_emit+0x8a) [0x7fbfb00ce2ca]
12  0x7fbfabc133b9 /usr/lib64/libwebkit2gtk-4.0.so.37(+0x8f13b9) [0x7fbfabc133b9]
13  0x7fbfababfa03 /usr/lib64/libwebkit2gtk-4.0.so.37(+0x79da03) [0x7fbfababfa03]
14  0x7fbfabd7b1be /usr/lib64/libwebkit2gtk-4.0.so.37(+0xa591be) [0x7fbfabd7b1be]
15  0x7fbfab97b54e /usr/lib64/libwebkit2gtk-4.0.so.37(+0x65954e) [0x7fbfab97b54e]
16  0x7fbfaba58ec2 /usr/lib64/libwebkit2gtk-4.0.so.37(+0x736ec2) [0x7fbfaba58ec2]
17  0x7fbfab976e60 /usr/lib64/libwebkit2gtk-4.0.so.37(+0x654e60) [0x7fbfab976e60]
18  0x7fbfab977728 /usr/lib64/libwebkit2gtk-4.0.so.37(+0x655728) [0x7fbfab977728]
19  0x7fbfadb222f3 /usr/lib64/libwebkit2gtk-4.0.so.37(+0x28002f3) [0x7fbfadb222f3]
20  0x7fbfadb505a9 /usr/lib64/libwebkit2gtk-4.0.so.37(+0x282e5a9) [0x7fbfadb505a9]
21  0x7fbfafdd8585 /usr/lib64/libglib-2.0.so.0(g_main_context_dispatch+0x135) [0x7fbfafdd8585]
22  0x7fbfafdd8928 /usr/lib64/libglib-2.0.so.0(+0x4a928) [0x7fbfafdd8928]
23  0x7fbfafdd89ac /usr/lib64/libglib-2.0.so.0(g_main_context_iteration+0x2c) [0x7fbfafdd89ac]
24  0x7fbfb039ce1d /usr/lib64/libgio-2.0.so.0(g_application_run+0x1ed) [0x7fbfb039ce1d]
25  0x46b9a3 geary(_vala_main+0x53) [0x46b9a3]
26  0x7fbfa8479f3a /lib64/libc.so.6(__libc_start_main+0xea) [0x7fbfa8479f3a]
27  0x46b87a geary(_start+0x2a) [0x46b87a]

The problem happens with WebKit-GTK 2.19.4 but didn't happen with 2.19.3.

[Radar WebKit Bug Importer]

<rdar://problem/36376724>

[Michael Catanzaro]

We need a real backtrace with debuginfo, taken with gdb, if you want us to investigate it.

[Guilaume Ayoub]

Sorry, I don't know gdb very well…  I've tried to run Geary with gdb but I don't get much more than the trace I posted previously. How can I get more debug info?

[Michael Catanzaro]

See https://wiki.gnome.org/Community/GettingInTouch/Bugzilla/GettingTraces for help with getting stack traces. Once you've figured out how to install the needed debuginfo, if you use a modern distro with coredumpctl enabled, it should be as easy as:

$ coredumpctl gdb
(gdb) bt full

[Guilaume Ayoub]

Created attachment 330972 [details]
Backtrace

Thanks a lot for your help, here's the trace with Geary and WebKit-GTK symbols (I hope it's enough).

[Michael Catanzaro]

Thanks, that was what I was looking for. Unfortunately I can't reproduce with trunk, and it's only been one day since 2.19.4.

While something is clearly wrong here, there's no sign that this crash on startup is also a web-exposed security issue. And it only affects an unstable release, so I'm going to make this bug public.

[Michael Catanzaro]

It's hitting a release assert:

    // We should have already ensured that Options::reservedZoneSize() >= minimumReserveZoneSize at
    // options initialization time, and the option value should not have been changed thereafter.
    // We don't have the ability to assert here that it hasn't changed, but we can at least assert
    // that the value is sane.
    RELEASE_ASSERT(reservedZoneSize >= minimumReservedZoneSize);

[Oliver Hunt]

Can we get a symbolicated trace?

[Oliver Hunt]

(In reply to Oliver Hunt from comment #8)
> Can we get a symbolicated trace?

Sorry was looking at a different bug. The internet is hard.

[Mark Lam]

(In reply to Michael Catanzaro from comment #7)
> It's hitting a release assert:
> 
>     // We should have already ensured that Options::reservedZoneSize() >=
> minimumReserveZoneSize at
>     // options initialization time, and the option value should not have
> been changed thereafter.
>     // We don't have the ability to assert here that it hasn't changed, but
> we can at least assert
>     // that the value is sane.
>     RELEASE_ASSERT(reservedZoneSize >= minimumReservedZoneSize);

Options::reservedZoneSize() is initialized in recomputeDependentOptions(), which is called from Options::initialize().  Options::initialize() is called from initializeThreading().

The only way this assertion can fail is if initializeThreading() was not called in the initialization process.

[Carlos Garcia Campos]

This is called after a message has been received from the web process, so threading has been initialized for sure. The first thing InitializeWebKit2() does is calling JSC::initializeThreading(). Guillaume, could you check the value of reservedZoneSize right before the assert?

[Guilaume Ayoub]

(In reply to Carlos Garcia Campos from comment #11)
> Guillaume, could you check the
> value of reservedZoneSize right before the assert?

It's 0.

[Yusuke Suzuki]

(In reply to Guilaume Ayoub from comment #12)
> (In reply to Carlos Garcia Campos from comment #11)
> > Guillaume, could you check the
> > value of reservedZoneSize right before the assert?
> 
> It's 0.

Can we ensure that Options::initialize() is called?
My rough guess is that this is similar to bug 179914 issue.

[Michael Catanzaro]

I can reproduce 100% by loading https://mail.gnome.org/mailman/admindb/epiphany-list in 2.19.5. The spam arriving at epiphany-list will grow unchecked. :P

So now that I have a reproducer, I can help with debugging.

(In reply to Yusuke Suzuki from comment #13)
> Can we ensure that Options::initialize() is called?
> My rough guess is that this is similar to bug 179914 issue.

My fear is that we'll discover my solution to bug #179914 is somehow insufficient. Which would be quite a shame.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #14)
> I can reproduce 100% by loading
> https://mail.gnome.org/mailman/admindb/epiphany-list in 2.19.5.

You have to load that in Epiphany since the crash depends on Epiphany web process messages (same as what Geary is doing).

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #15)
> You have to load that in Epiphany since the crash depends on Epiphany web
> process messages (same as what Geary is doing).

Every webpage with a password form crashes the UI process. :D

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #14)
> My fear is that we'll discover my solution to bug #179914 is somehow
> insufficient. Which would be quite a shame.

I can only reproduce when I build without -DDEVELOPER_MODE=ON. That's a bad sign.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #17)
> I can only reproduce when I build without -DDEVELOPER_MODE=ON. That's a bad
> sign.

And indeed, there are two different instances of the function JSC::Options::Initialize in the process address space, according to some simple debugging:

// Above the RELEASE_ASSERT
WTFLogAlways("%s: pid=%u Options::Initialize=%p", __PRETTY_FUNCTION__, getpid(), &Options::initialize);


Output snippet:

void JSC::VM::updateStackLimits(): pid=10373 Options::Initialize=0x7fe2b2387af6
void JSC::VM::updateStackLimits(): pid=10373 Options::Initialize=0x7fe2ad41091e

^ note the address of the function has changed....

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #11)
> This is called after a message has been received from the web process, so
> threading has been initialized for sure. The first thing InitializeWebKit2()
> does is calling JSC::initializeThreading(). Guillaume, could you check the
> value of reservedZoneSize right before the assert?

The problem is that it's only been called in libwebkit2gtk's copy of JSC, not in libjavascriptcoregtk's copy of JSC. Remember that after bug #179914, both libraries contain their own separate copies of JSC, so both will need to be initialized.

Maybe we can do this in a library constructor.

[Yusuke Suzuki]

(In reply to Michael Catanzaro from comment #19)
> (In reply to Carlos Garcia Campos from comment #11)
> > This is called after a message has been received from the web process, so
> > threading has been initialized for sure. The first thing InitializeWebKit2()
> > does is calling JSC::initializeThreading(). Guillaume, could you check the
> > value of reservedZoneSize right before the assert?
> 
> The problem is that it's only been called in libwebkit2gtk's copy of JSC,
> not in libjavascriptcoregtk's copy of JSC. Remember that after bug #179914,
> both libraries contain their own separate copies of JSC, so both will need
> to be initialized.
> 
> Maybe we can do this in a library constructor.

How do we solve such a problem in macOS?

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #20)
> (In reply to Michael Catanzaro from comment #19)
> > (In reply to Carlos Garcia Campos from comment #11)
> > > This is called after a message has been received from the web process, so
> > > threading has been initialized for sure. The first thing InitializeWebKit2()
> > > does is calling JSC::initializeThreading(). Guillaume, could you check the
> > > value of reservedZoneSize right before the assert?
> > 
> > The problem is that it's only been called in libwebkit2gtk's copy of JSC,
> > not in libjavascriptcoregtk's copy of JSC. Remember that after bug #179914,
> > both libraries contain their own separate copies of JSC, so both will need
> > to be initialized.
> > 
> > Maybe we can do this in a library constructor.
> 
> How do we solve such a problem in macOS?

If we the app can mix libjavascriptcoregtk and libwebkit2gtk, having 2 copies of JSC looks dangerous to me.

[Oliver Hunt]

> How do we solve such a problem in macOS?

on osx JavaScriptCore is not included in WebKit, so there's only one.

The problem is that (I would guess) webkitgtk can't simply depend on a distinct jsc package due the the (non-api) binary dependencies between webkit and jsc.

Because webkitgtk and javascriptcoregtk are separately packaged they can be updated out of sync, and so webkitgtk must contain its own JSC. This means that you encounter a problem if you have an application that uses webkitgtk and javacrbiptcoregtk at the same time.

On osx we just ensure that webkit and javascriptcore releases are built in lock step, and if there is ever an update to only (strictly speaking) requires JSC to update we still rebuild and update the entire dependency chain.

[Oliver Hunt]

> If we the app can mix libjavascriptcoregtk and libwebkit2gtk, having 2
> copies of JSC looks dangerous to me.

Yes, the fix for this is probably to prevent an application from linking both.

Basically imagine I do the following:

(in my app):
JSObjectRef obj = JSObjectMake(GtkWebViewGetContext(), ...)
GtkWebViewDoCoolThing(obj)

If my app links to libjavascriptgtk then JSObjectMake will hit that implementation, and pass a JSContextRef that points to a context from the webkitgtk jsc, which may not be binary compatible. Even if it is, it will then create a JSObject using the ljscgtk object layout and pass the the webkitgtk's jsc implementation, and so on and so forth with each step getting increasingly more likely to go wrong.

Also I suspect the GC(s) will have a fit :-/

[Michael Catanzaro]

Created attachment 331155 [details]
Patch

[Oliver Hunt]

I'm unsure what a good fix for this is. Literally the reason JSC and WK c an be different frameworks on osx is because B&I has binary (e.g. non-api,spi) dependency chains recorded for all projects, so when an update needs one project all the depending projects are also rebuilt and placed in the update.

[Oliver Hunt]

Comment on attachment 331155 [details]
Patch

I am not convinced this is safe (see my prior comment) -- if the two jsc binaries cannot be guaranteed to be the same, then objects can't be shared.  Also I just realized that I have no idea what happens with things like the GC, or even gigacages, will work if there are different instances of the same library.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #24)
> Created attachment 331155 [details]
> Patch

This broken patch fixes the reported crash, and I think it's the only way to make this work. But it introduces a UI process hang when closing any web process, which I have not debugged yet.

It's also possible that passing JSValues between two different JSC VMs will, er... not work very stupendously. Not sure. Might be that we have to roll out r226266.

[Michael Catanzaro]

(In reply to Oliver Hunt from comment #26) 
> I am not convinced this is safe (see my prior comment) -- if the two jsc
> binaries cannot be guaranteed to be the same, then objects can't be shared. 
> Also I just realized that I have no idea what happens with things like the
> GC, or even gigacages, will work if there are different instances of the
> same library.

Yeah, I think we need to go back to the drawing board in bug #179914.

[Oliver Hunt]

(In reply to Michael Catanzaro from comment #28)
> (In reply to Oliver Hunt from comment #26) 
> > I am not convinced this is safe (see my prior comment) -- if the two jsc
> > binaries cannot be guaranteed to be the same, then objects can't be shared. 
> > Also I just realized that I have no idea what happens with things like the
> > GC, or even gigacages, will work if there are different instances of the
> > same library.
> 
> Yeah, I think we need to go back to the drawing board in bug #179914.

Yeah, looking at that you're correct.

It's not even mixing between VMs (which isn't safe anyway :p ) it's sharing objects with the same C type between libraries that may have different ideas of what the C type actually is.

Ideally there would be a single libjavascriptcore that webkitgtk (or qt) could link to. The problem is that you need to have a way to ensure lock step updates when they're necessary

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #28) 
> Yeah, I think we need to go back to the drawing board in bug #179914.

It seemed to work so well at first, too. :(

Carlos, we have a few options:

 a) We could export the JSC API in libwebkit2gtk, and change libwebkit2gtk to not link to libjavascriptcoregtk. The downside is an application will likely blow up if it accidentally links to both libs.

 b) We could get rid of separate libjavascriptcoregtk, which was probably a mistake. I think we can do it now, since nothing except seed git master depends on it, and it has not released in years. If anyone starts working on seed again, we can tell them to package the JSCOnly port instead. That does have the same problem as the point above, though: any app that accidentally links to our libwebkit2gtk and the hypothetical libjsconly will likely blow up due to symbol clashes.

 c) If you don't want to get rid of separate libjavascriptcoregtk, then we can do a partial revert of r226266, stop static linking libwebkit2gtk to JavaScriptCore but keep -fvisibility=hidden. Then we need to sabotage all the internal export macros (JSC_EXPORT, WEBCORE_EXPORT) to do nothing. Then we can drop the linker version script entirely. I *think* that would solve everything.

Let me try (c). Disabling the internal export macros is something I really should have done in the previous bug, anyway.

[Michael Catanzaro]

(In reply to Oliver Hunt from comment #29)
> It's not even mixing between VMs (which isn't safe anyway :p ) it's sharing
> objects with the same C type between libraries that may have different ideas
> of what the C type actually is.

No they're guaranteed to have the same idea of what the C type is, because they're literally built from the same source. The static lib inside both is exactly the same. In both traditional Linux distributions and in new deployment models like Flatpak, they'll always be updated at the same time, and it would not be our fault if that wasn't the case.

The problem is the library state, like the VMs, being unexpectedly different. If I was smarter, I would not have even tried to do this. But, hey, it seemed like it worked well... for about three weeks!

> Ideally there would be a single libjavascriptcore that webkitgtk (or qt)
> could link to. The problem is that you need to have a way to ensure lock
> step updates when they're necessary

It would be way too hard. And the first step would be to bring back PLATFORM(QT) upstream. I guess Konstantin would like that. :D

[Oliver Hunt]

(In reply to Michael Catanzaro from comment #31)
> (In reply to Oliver Hunt from comment #29)
> > It's not even mixing between VMs (which isn't safe anyway :p ) it's sharing
> > objects with the same C type between libraries that may have different ideas
> > of what the C type actually is.
> 
> No they're guaranteed to have the same idea of what the C type is, because
> they're literally built from the same source. The static lib inside both is
> exactly the same. In both traditional Linux distributions and in new
> deployment models like Flatpak, they'll always be updated at the same time,
> and it would not be our fault if that wasn't the case.
> 
> The problem is the library state, like the VMs, being unexpectedly
> different. If I was smarter, I would not have even tried to do this. But,
> hey, it seemed like it worked well... for about three weeks!


If they're the same library my inclination would be to have libwebkitgtk (or whatever it is called now) just link libjavascriptcore, then you wouldn't need to worry about duplicate linking as you only get a single library loaded

> 
> > Ideally there would be a single libjavascriptcore that webkitgtk (or qt)
> > could link to. The problem is that you need to have a way to ensure lock
> > step updates when they're necessary
> 
> It would be way too hard. And the first step would be to bring back
> PLATFORM(QT) upstream. I guess Konstantin would like that. :D

I was joking :D

But I mean an ideal world would be a single libjavascriptcore that all platforms could use -- the problem would be if there are multiple webkits for different toolkits (with different build flags, etc) and different release schedules.

[Michael Catanzaro]

(In reply to Oliver Hunt from comment #32) 
> If they're the same library my inclination would be to have libwebkitgtk (or
> whatever it is called now) just link libjavascriptcore, then you wouldn't
> need to worry about duplicate linking as you only get a single library loaded

Yes, that's what we need to go back to, but we need to do it slightly differently than before, because what we had before blew up too (bug #179914). Patch incoming....

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #30)
> Let me try (c). Disabling the internal export macros is something I really
> should have done in the previous bug, anyway.

I think it might work, but I'm not sure. I need another day to test it, since it requires doing full rebuilds with and without DEVELOPER_MODE enabled.

It turns out the export macros were *already* doing nothing on Linux ports. So removing the version scripts should be possible. But the risk is that -fvisibility=hidden might cause bug #179914, even without the version script. Not sure.

[Oliver Hunt]

(In reply to Michael Catanzaro from comment #34)
> (In reply to Michael Catanzaro from comment #30)
> > Let me try (c). Disabling the internal export macros is something I really
> > should have done in the previous bug, anyway.
> 
> I think it might work, but I'm not sure. I need another day to test it,
> since it requires doing full rebuilds with and without DEVELOPER_MODE
> enabled.
> 
> It turns out the export macros were *already* doing nothing on Linux ports.
> So removing the version scripts should be possible. But the risk is that
> -fvisibility=hidden might cause bug #179914, even without the version
> script. Not sure.

Hmmm, I just found myself wondering if JSC uses identity of function pointers anywhere (I mean compiled in function pointers, rather than js ones :D )

[JF Bastien]

(In reply to Oliver Hunt from comment #35)
> (In reply to Michael Catanzaro from comment #34)
> > (In reply to Michael Catanzaro from comment #30)
> > > Let me try (c). Disabling the internal export macros is something I really
> > > should have done in the previous bug, anyway.
> > 
> > I think it might work, but I'm not sure. I need another day to test it,
> > since it requires doing full rebuilds with and without DEVELOPER_MODE
> > enabled.
> > 
> > It turns out the export macros were *already* doing nothing on Linux ports.
> > So removing the version scripts should be possible. But the risk is that
> > -fvisibility=hidden might cause bug #179914, even without the version
> > script. Not sure.
> 
> Hmmm, I just found myself wondering if JSC uses identity of function
> pointers anywhere (I mean compiled in function pointers, rather than js ones
> :D )

At least one place that I want to remove eventually:
  Source/JavaScriptCore/wasm/WasmThunks.cpp

void Thunks::setThrowWasmException(ThrowWasmException throwWasmException)
{
    auto locker = holdLock(m_lock);
    // The thunks are unique for the entire process, therefore changing the throwing function changes it for all uses of WebAssembly.
    RELEASE_ASSERT(!m_throwWasmException || m_throwWasmException == throwWasmException);
    m_throwWasmException = throwWasmException;
}

[Carlos Garcia Campos]

Let me explain how things have worked before r226266. 

 - We have libwk.so and libjsc.so. 
 - Applications only link to libwk.so and libwk links to libjsc.so.
 - Both libraries are distributed by a single package libwebkitgtk, so they are always built together, it's not possible to have different versions mixed.
 - libwk uses a version script to ensure only public API symbols were exported.
 - libjsc exports all symbols marked as default visibility in the code, because WebCore uses internal libjsc symbols.

In bug #179914 we realized that when defining symbols in the header we might end up with the same symbol in both libjsc and libwk (in this case the problem was a PerProcess<> impl what caused the issues). The symbol is expected to be a libjsc one, but also defined in libwk because the header is included in WebCore. That duplicated symbol was global (public) in libjsc, but local (private) in libwk. See more details in https://bugs.webkit.org/show_bug.cgi?id=179914#c49. The thing is that both symbols might end up being used.

My first idea was to make those symbols also global in libwk, because when duplicated symbols are unique global, only one is ensured to be used. But Michael's solution seemed to work and it included other improvements, like reducing the amount of exported symbols in libwk and no longer exporting everything in libjsc.

[Michael Catanzaro]

*** Bug 181581 has been marked as a duplicate of this bug. ***

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #30)
> Let me try (c). Disabling the internal export macros is something I really
> should have done in the previous bug, anyway.

It doesn't work, because the jsc shell and WebDriver need internal JSC/WTF symbols.

Still trying a few different things....

[Michael Catanzaro]

The jsc shell totally prevents building JSC only as a shared library and also not export internal symbols. I think we have to (a) give up on exporting only public API symbols from libjsc (something we had not previously been doing), or (b) give up on having separate libjavascriptcoregtk, or (c) adopt Carlos's suggestion in https://bugs.webkit.org/show_bug.cgi?id=179914#c56 to programatically generate a linker version script that dynamically searches for unique global symbols that are present in the bss of both libjavascriptcoregtk and libwebkit2gtk and ensures they are not marked local.

My preference is (b), since it allows us to keep everything except separate JSC, and separate JSC is the cause of all these problems. But I presume that's not likely to be accepted.

So I'll probably try to implement (a). With this approach, we'll need to export all the same internal symbols from bmalloc, WTF, and JSC that are exported on other ports. That is, we'll need to make BEXPORT, WTF_EXPORT, and JS_EXPORT actually export. (BEXPORT is special -- it already does! -- but the others currently do nothing for GTK and WPE.) This is not great, because we don't want to expose any of those symbols, but it'll still be better than what we had before, because before we exported everything in libjavascriptcoregtk, including all the symbols that were not marked for export. I suppose exporting internal symbols is not the end of the world, and of course it will only be those symbols that are actually needed in libwebkit2gtk, jsc shell, or WebDriver. There's also one advantage for WebKit developers: it means we'll notice build failures on Linux when we forget to mark something with e.g. WEBCORE_EXPORT, instead of having to wait for the Windows EWS to fail.

(c) would be useful an optimization to reduce as greatly as possible the set of symbols that get exported from libjavascriptcoregtk, though it's also overly-complex. If we take this approach, we have to decide how to keep the jsc shell working. I think it could be done by static linking JSC to the jsc shell (as we do WTF to the WebDriver), but *not* to libwebkit2gtk (since that is causing this very bug). This is starting to seem very complex, but it might be worthwhile.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #40)
> (c) adopt Carlos's suggestion in
> https://bugs.webkit.org/show_bug.cgi?id=179914#c56 to programatically
> generate a linker version script that dynamically searches for unique global
> symbols that are present in the bss of both libjavascriptcoregtk and
> libwebkit2gtk and ensures they are not marked local.

Or the script could fail the build if this situation is ever detected.

[Oliver Hunt]

(In reply to Michael Catanzaro from comment #40)
> The jsc shell totally prevents building JSC only as a shared library and
> also not export internal symbols. I think we have to (a) give up on
> exporting only public API symbols from libjsc (something we had not
> previously been doing), or (b) give up on having separate
> libjavascriptcoregtk, or (c) adopt Carlos's suggestion in
> https://bugs.webkit.org/show_bug.cgi?id=179914#c56 to programatically
> generate a linker version script that dynamically searches for unique global
> symbols that are present in the bss of both libjavascriptcoregtk and
> libwebkit2gtk and ensures they are not marked local.
> 

Seriously you shouldn't include the jsc binary in distribution (it's only in macOS by accident leading to people using it and causing it to be needed to avoid breaking people).

The jsc binary is a testing tool so isn't remotely hardened against arbitrary JS execution, and some of the test functions trust input to be valid in ways that aren't secure.

That said, I believe that you will need at least some of the non-api symbols to be publicly exported, although I have no idea how that works now that the exports files are gone :D

[Yusuke Suzuki]

I like (a). Aligning our ports to the other ones makes less problematic. And noticing missing EXPORT in Linux build is very nice for Linux WebKit devs.

[Yusuke Suzuki]

I like (a). Aligning our ports to the other ones makes less problematic. And noticing missing EXPORT in Linux build is very nice for Linux WebKit devs.

[Michael Catanzaro]

(In reply to Oliver Hunt from comment #42)
> Seriously you shouldn't include the jsc binary in distribution (it's only in
> macOS by accident leading to people using it and causing it to be needed to
> avoid breaking people).

I think it should be safe to remove, because I think nobody should be using it.

(In reply to Michael Catanzaro from comment #40)
> There's also one advantage for
> WebKit developers: it means we'll notice build failures on Linux when we
> forget to mark something with e.g. WEBCORE_EXPORT, instead of having to wait
> for the Windows EWS to fail.

Sorry, it was late and I wasn't thinking properly. What I said was right, until I provided a bogus example. You'll only notice missing JS_EXPORT[_PRIVATE] or WTF_EXPORT. WEBCORE_EXPORT will still be a no-op because WebCore is only in libwebkit2gtk, so the exports aren't needed. The only way those could possibly do anything is if we build WebCore as a shared library, which would just reduce performance for no benefit.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #40)
> The jsc shell totally prevents building JSC only as a shared library and
> also not export internal symbols.

That's not really true either. What I was trying to do yesterday was build JSC as two shared libraries: one an internal library with all the internal symbols exported, and one with them stripped out. The entire approach was silly because that's not how visibility works: one shared lib cannot hide the symbols of another lib. So jsc shell isn't actually causing any problems here. The root of all the problems is the separate libjavascriptcoregtk, which IMO is serving no good purpose.

Anyway, I'll try to implement (a) today. Remains to be seen how -fvisibility=hidden affects unique global symbols, but in the worst case, if we get crashes in the future because something that ought to be unique global is instead local, it should be solvable by exporting the affected symbol.

(b) is still my preference. We should at least drop the separate libjavascriptcoregtk from future API versions, if not from libwebkit2gtk-4.0, because it has proved to have been quite a mistake.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #46)
> Anyway, I'll try to implement (a) today.

The problem I didn't anticipate is Source/ThirdParty. It's easy enough to control our own export macros in the WebKit projects, but many subprojects in Source/ThirdParty use these too. It's going to be a real pain to stay on top of these: we will no doubt start exporting new symbols by accident after random ANGLE and libwebrtc updates, each of which pull in a bunch of other bundled libraries that will change or get added....

[Oliver Hunt]

(In reply to Michael Catanzaro from comment #47)
> (In reply to Michael Catanzaro from comment #46)
> > Anyway, I'll try to implement (a) today.
> 
> The problem I didn't anticipate is Source/ThirdParty. It's easy enough to
> control our own export macros in the WebKit projects, but many subprojects
> in Source/ThirdParty use these too. It's going to be a real pain to stay on
> top of these: we will no doubt start exporting new symbols by accident after
> random ANGLE and libwebrtc updates, each of which pull in a bunch of other
> bundled libraries that will change or get added....

angle, etc are above jsc so their changes shouldn't impact libjsc.

[Michael Catanzaro]

Right, but it matters because it makes it very difficult to drop the libwebkit2gtk version script, which was the cause of bug #179914.

I'm attaching a patch that fixes the immediate issue, but without removing the version script. This patch reintroduces the underlying problem in bug #179914, but that's OK because it does not currently blow up.

[Michael Catanzaro]

Created attachment 331301 [details]
Patch

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #40)
> The jsc shell totally prevents building JSC only as a shared library and
> also not export internal symbols. I think we have to (a) give up on
> exporting only public API symbols from libjsc (something we had not
> previously been doing), or (b) give up on having separate
> libjavascriptcoregtk, or (c) adopt Carlos's suggestion in
> https://bugs.webkit.org/show_bug.cgi?id=179914#c56 to programatically
> generate a linker version script that dynamically searches for unique global
> symbols that are present in the bss of both libjavascriptcoregtk and
> libwebkit2gtk and ensures they are not marked local.

I don't think a) and b) are mutually exclusive. Actually, only doing a) is like what we had before, but exporting fewer symbols, the PerProcess<> problem would not be fixed I think.

> My preference is (b), since it allows us to keep everything except separate
> JSC, and separate JSC is the cause of all these problems. But I presume
> that's not likely to be accepted.

No, it's not. Seed still exists in debian and probably in other distros. It's true that currently nobody uses it, but we plan to add a glib wrapper to JSC, similar to the Objc bindings, that will allow to do seed the right way. At least in debian also libproxy1-plugin-webkit depends only on libjsc, so it's not acceptable to remove libjsc right now.

> So I'll probably try to implement (a). With this approach, we'll need to
> export all the same internal symbols from bmalloc, WTF, and JSC that are
> exported on other ports. That is, we'll need to make BEXPORT, WTF_EXPORT,
> and JS_EXPORT actually export. (BEXPORT is special -- it already does! --
> but the others currently do nothing for GTK and WPE.) This is not great,
> because we don't want to expose any of those symbols, but it'll still be
> better than what we had before, because before we exported everything in
> libjavascriptcoregtk, including all the symbols that were not marked for
> export. I suppose exporting internal symbols is not the end of the world,
> and of course it will only be those symbols that are actually needed in
> libwebkit2gtk, jsc shell, or WebDriver. There's also one advantage for
> WebKit developers: it means we'll notice build failures on Linux when we
> forget to mark something with e.g. WEBCORE_EXPORT, instead of having to wait
> for the Windows EWS to fail.
> 
> (c) would be useful an optimization to reduce as greatly as possible the set
> of symbols that get exported from libjavascriptcoregtk, though it's also
> overly-complex. If we take this approach, we have to decide how to keep the
> jsc shell working. I think it could be done by static linking JSC to the jsc
> shell (as we do WTF to the WebDriver), but *not* to libwebkit2gtk (since
> that is causing this very bug). This is starting to seem very complex, but
> it might be worthwhile.

No, that was not the idea of c). This idea was based on the fact the we exported all symbols in libjsc, so in case of having the same symbol in libwk (and in bss), we also exported it in libwk to ensure it's not local. This way the symbols become unique global and only one is used. I don't know what happens if the symbol is local in both...

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #46)
> (In reply to Michael Catanzaro from comment #40)
> > The jsc shell totally prevents building JSC only as a shared library and
> > also not export internal symbols.
> 
> That's not really true either. What I was trying to do yesterday was build
> JSC as two shared libraries: one an internal library with all the internal
> symbols exported, and one with them stripped out. The entire approach was
> silly because that's not how visibility works: one shared lib cannot hide
> the symbols of another lib. So jsc shell isn't actually causing any problems
> here. The root of all the problems is the separate libjavascriptcoregtk,
> which IMO is serving no good purpose.
> 
> Anyway, I'll try to implement (a) today. Remains to be seen how
> -fvisibility=hidden affects unique global symbols, but in the worst case, if
> we get crashes in the future because something that ought to be unique
> global is instead local, it should be solvable by exporting the affected
> symbol.
> 
> (b) is still my preference. We should at least drop the separate
> libjavascriptcoregtk from future API versions, if not from
> libwebkit2gtk-4.0, because it has proved to have been quite a mistake.

That's your opinion.

[Carlos Garcia Campos]

(In reply to Carlos Garcia Campos from comment #51)
> (In reply to Michael Catanzaro from comment #40)
> > The jsc shell totally prevents building JSC only as a shared library and
> > also not export internal symbols. I think we have to (a) give up on
> > exporting only public API symbols from libjsc (something we had not
> > previously been doing), or (b) give up on having separate
> > libjavascriptcoregtk, or (c) adopt Carlos's suggestion in
> > https://bugs.webkit.org/show_bug.cgi?id=179914#c56 to programatically
> > generate a linker version script that dynamically searches for unique global
> > symbols that are present in the bss of both libjavascriptcoregtk and
> > libwebkit2gtk and ensures they are not marked local.
> 
> I don't think a) and b) are mutually exclusive. Actually, only doing a) is
> like what we had before, but exporting fewer symbols, the PerProcess<>
> problem would not be fixed I think.

I meant a) and c), sorry.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #49)
> Right, but it matters because it makes it very difficult to drop the
> libwebkit2gtk version script, which was the cause of bug #179914.

I don't think the libwk version script caused the bug, and I don't think we should drop it either.

> I'm attaching a patch that fixes the immediate issue, but without removing
> the version script. This patch reintroduces the underlying problem in bug
> #179914, but that's OK because it does not currently blow up.

So, we still need c)

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #51)
> > My preference is (b), since it allows us to keep everything except separate
> > JSC, and separate JSC is the cause of all these problems. But I presume
> > that's not likely to be accepted.
> 
> No, it's not. Seed still exists in debian and probably in other distros.
> It's true that currently nobody uses it, but we plan to add a glib wrapper
> to JSC, similar to the Objc bindings, that will allow to do seed the right
> way. At least in debian also libproxy1-plugin-webkit depends only on libjsc,
> so it's not acceptable to remove libjsc right now.

I think this is not a very good example, for several reasons:

 * seed does not use libjavascriptcoregtk-4.0. The current release is over five years old and uses libjavascriptcoregtk-3.0. Only git master can use libjavascriptcoregtk-4.0, and there's no indication that a release is coming anytime soon.
 * seed has actually been removed from Fedora for the above reason. It's unlikely that it will ever return, even if so, because nothing depends on seed. gjs has thoroughly won. I'm surprised that seed has not been removed from Debian as well. Does Debian build a git snapshot?
 * Independently of all that, seed and any other application linking to libjavascriptcoregtk would continue to work just fine if we symlink libjavascriptcoregtk to libwebkit2gtk. The only problem is that it would be "unnecessarily" linked to libwebkit2gtk, for some value of unnecessarily. ;)

But anyway, it's fair that you don't want to do that, so let's move on to the other options....
 
> No, that was not the idea of c). This idea was based on the fact the we
> exported all symbols in libjsc, so in case of having the same symbol in
> libwk (and in bss), we also exported it in libwk to ensure it's not local.
> This way the symbols become unique global and only one is used. I don't know
> what happens if the symbol is local in both...

Yes, I understand. If the symbol is local in both, it would probably blow up. That's not the goal.

(In reply to Carlos Garcia Campos from comment #54)
> I don't think the libwk version script caused the bug, and I don't think we
> should drop it either.

It definitely caused the bug, by changing the unique global symbols to local symbols.

Anyway, my patch here retains that version script, it only removes the javascriptcoregtk version script.

> > I'm attaching a patch that fixes the immediate issue, but without removing
> > the version script. This patch reintroduces the underlying problem in bug
> > #179914, but that's OK because it does not currently blow up.
> 
> So, we still need c)

Yes, these solutions are not mutually-exclusive. My patch implements (a), which is sufficient to solve this bug and leaves us in a mildly-better position than we were originally. I've already committed this patch to the GNOME SDK builder to un-break WebKit there. If you want to implement (c), I'll review it in bug #179914, which I have reopened.

[Michael Catanzaro]

Comment on attachment 331301 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=331301&action=review

> Source/WTF/wtf/Platform.h:1073
> +#if !defined(USE_EXPORT_MACROS) && !PLATFORM(WPE)

A better condition to use here would be:

#if !defined(USE_EXPORT_MACROS) && (!OS(UNIX) || PLATFORM(GTK))

To clarify that GTK is the odd one out, and that there is nothing odd about WPE not using these.

[Michael Catanzaro]

I've unset r? because the patch is broken. I guess I did not test it properly yesterday; maybe I only tried running Epiphany in developer mode, and just checked that it built with developer mode off. I'm starting to feel rather unreliable. Anyway, it crashes immediately. Problem is that s_mainRunLoop is unset. I think it's caused because GTK port static links WebCore to WTF, causing us to have two copies of all the WTF stuff in both the shared libjavascriptcoregtk and libwebkit2gtk. That's not a new problem; I guess it's just luck that it ever worked before.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #57)
> I think it's caused because GTK port static links
> WebCore to WTF, causing us to have two copies of all the WTF stuff in both
> the shared libjavascriptcoregtk and libwebkit2gtk.

I've failed to fix it. My only suggestion now is to undo all of my functional changes in bug #179914 (including use of -fvisibility=hidden, which is a shame).

[Michael Catanzaro]

Created attachment 331317 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 331317 [details]
Patch

Yes, better let's go back to what we had and then let's try to find a way to improve it. Thanks!

[WebKit Commit Bot]

Comment on attachment 331317 [details]
Patch

Clearing flags on attachment: 331317

Committed r226945: <https://trac.webkit.org/changeset/226945>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Guilaume Ayoub]

The bug is gone in 2.19.6, thank you very much for your help and your hard work on WebKit-GTK <3 <3 <3.