Bug 114901

Summary: [GTK] Plugin process broken due to a missing symbol
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKit2Assignee: Zan Dobersek <zan>
Status: RESOLVED FIXED    
Severity: Normal CC: gustavo, mrobinson, pnormand, rego+ews, xan.lopez, zan
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
WIP
none
Patch
none
Provisional updated patch
none
Patch none

Description Carlos Garcia Campos 2013-04-20 07:03:02 PDT
Programs/WebKitPluginProcess /usr/lib/mozilla/plugins/flashplugin-alternative.so 
WebKitBuild/Release/Programs/.libs/lt-WebKitPluginProcess: symbol lookup error: WebKitBuild/Release/Programs/.libs/lt-WebKitPluginProcess: undefined symbol: _ZTVN7WebCore14DelayDSPKernelE

I guess it only happens with web-audio enabled. I don't know when it started because unit tests are broken in the bots and I didn't know it.
Comment 1 Zan Dobersek 2013-04-20 07:32:47 PDT
Will fix.
Comment 2 Carlos Garcia Campos 2013-04-20 10:02:11 PDT
Building without web-audio I still get undefined symbols:

Programs/WebKitPluginProcess /usr/lib/mozilla/plugins/flashplugin-alternative.so 
WebKitBuild/Release/Programs/.libs/lt-WebKitPluginProcess: symbol lookup error: WebKitBuild/Release/Programs/.libs/lt-WebKitPluginProcess: undefined symbol: _ZN7WebCore21WebKitFontFamilyNames14standardFamilyE

Have you guys changed the libraries recently? I'm quite sure I tested it when I added libWebKit2Platform.
Comment 3 Zan Dobersek 2013-04-21 07:57:51 PDT
No, no such changes were made recently.

The following build is the first occurrence of this problem (the crashing WebKitPluginProcess due to undefined symbols):
http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release/builds/36869

That makes this revision the most likely suspect:
http://trac.webkit.org/changeset/148519

Nothing certain yet though, might be some earlier commit that only started causing problems from this build onwards (which would be messed up, really).
Comment 4 Carlos Garcia Campos 2013-04-21 08:30:40 PDT
(In reply to comment #3)
> No, no such changes were made recently.
> 
> The following build is the first occurrence of this problem (the crashing WebKitPluginProcess due to undefined symbols):
> http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release/builds/36869
> 
> That makes this revision the most likely suspect:
> http://trac.webkit.org/changeset/148519
> 
> Nothing certain yet though, might be some earlier commit that only started causing problems from this build onwards (which would be messed up, really).

Indeed, reverting r148519 fixes the issue, thanks for looking at it, not let's see why.
Comment 5 Carlos Garcia Campos 2013-04-21 09:46:54 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > No, no such changes were made recently.
> > 
> > The following build is the first occurrence of this problem (the crashing WebKitPluginProcess due to undefined symbols):
> > http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release/builds/36869
> > 
> > That makes this revision the most likely suspect:
> > http://trac.webkit.org/changeset/148519
> > 
> > Nothing certain yet though, might be some earlier commit that only started causing problems from this build onwards (which would be messed up, really).
> 
> Indeed, reverting r148519 fixes the issue, thanks for looking at it, not let's see why.

I don't understand why, but simply using ScriptController (not necessarily to initialize threading, but any use of ScriptController) makes the symbols to be defined in the binary. I've tried moving the order of the libraries in Programs_WebKitPluginProcess_LDADD like Gustavo did when we had similar issues while splitting SVG, but I haven't managed to make it work :-(
Comment 6 Zan Dobersek 2013-04-22 01:21:15 PDT
I've invested some time yesterday evening looking at this and I believe the general problem here is trying to cram every required static library into the WebKitPluginProcess. The current situation itself shows that this isn't really working out, at least not with the current setup.

I find the use of passing the '--unresolved-symbols=ignore-in-object-files' flag to the linker a bit problematic - I had to add multiple WK2 source files (like Shared/soup/PlatformCertificateInfo.*, UIProcess/Launcher/ProcessLauncher.*) to the build to remove some of the unresolved symbols. Again, this workaround is most likely in use due to throwing every static library that's required into the plugin process binary.

I've worked around this problem by creating a new shared library, at the moment called libwebkit2gtk-pluginlib-3.0.la. It's built from the same sources as the plugin process was, except the PluginMainUnix.cpp which is built into the new plugin process binary. This setup very much resembles how the WebKitWebProcess is built.

I'm not sure if having a separate shared library is a viable option, though. I'll upload the patch for reference and we can discuss it further.
Comment 7 Zan Dobersek 2013-04-22 01:39:57 PDT
Created attachment 199000 [details]
WIP

Just the raw WIP patch, showing the approach to setting up a new shared library that's used by the slimmed-down plugin process binary.
Comment 8 Martin Robinson 2013-04-22 08:21:30 PDT
(In reply to comment #6)

> I've worked around this problem by creating a new shared library, at the moment called libwebkit2gtk-pluginlib-3.0.la. It's built from the same sources as the plugin process was, except the PluginMainUnix.cpp which is built into the new plugin process binary. This setup very much resembles how the WebKitWebProcess is built.

I'm not sure I totally comprehend how linking is different if you compile many static libraries into a binary versus many static libraries into one giant static library and then that into a binary. In the past we've found that introducing more intermediary static libraries has slowed down the build and introduced problems.
Comment 9 Gustavo Noronha (kov) 2013-04-22 13:07:43 PDT
I don't understand it either, but it does indeed work, while linking the static libraries directly into the PluginProcess binary always causes unresolved symbols to show up... I also considered making a private 'pluginprocess' library to solve this in the past, I think it's a valid option for now tbh. I hope that problem will go away when we ditch libtool =/
Comment 10 Zan Dobersek 2013-04-25 02:00:15 PDT
Created attachment 199636 [details]
Patch
Comment 11 Zan Dobersek 2013-04-25 02:21:27 PDT
(In reply to comment #10)
> Created an attachment (id=199636) [details]
> Patch

Right, that fixes it. It works around _a lot_ of libtool problems.

Carlos recommended using the ld's --start-group and --end-group flags. I've put quite some effort into these but couldn't get them to work. My guess is they are only functional for shared libraries, though I haven't investigated the problem enough to be sure.

The required static libraries are listed up to three times, but in three different ways, just so libtool actually passess the complete and non-simplified list to the linker. Libtool provides the --preserve-dup-deps flag that should sort this out, but again, it doesn't. The suspicion I have is again that this only works for shared libraries.

All in all this just shows the layer violations are obviously present and need fixing, but are rarely showing through and causing problems. Though, when eventually they do show up, the solution is an ugly workaround that no one would like to see, but I guess is still required.
Comment 12 Zan Dobersek 2013-04-25 02:50:55 PDT
Needs polishing. Still sticking behind the approach though.
Comment 13 Zan Dobersek 2013-04-25 03:05:00 PDT
Created attachment 199647 [details]
Provisional updated patch
Comment 14 Zan Dobersek 2013-04-25 09:13:29 PDT
(In reply to comment #13)
> Created an attachment (id=199647) [details]
> Provisional updated patch

The patch sets up a phony rule for the .libs/libPlatformGtk2.la and .libs/libWebCoreGtk2.la files. EWS still complains, but I've compiled clean builds with the patch successfully using both the ld and the gold linker, so I think clean builds will be required after the patch lands.
Comment 15 Zan Dobersek 2013-04-25 10:13:48 PDT
Created attachment 199680 [details]
Patch
Comment 16 Zan Dobersek 2013-04-25 10:15:00 PDT
Turns out the EWS error was a valid one, should be fixed now.
Comment 17 Gustavo Noronha (kov) 2013-04-25 13:06:21 PDT
Comment on attachment 199680 [details]
Patch

I don't feel too dirty r+ing this.
Comment 18 Zan Dobersek 2013-04-25 23:48:57 PDT
Comment on attachment 199680 [details]
Patch

Clearing flags on attachment: 199680

Committed r149169: <http://trac.webkit.org/changeset/149169>
Comment 19 Zan Dobersek 2013-04-25 23:49:03 PDT
All reviewed patches have been landed.  Closing bug.