Bug 153117

Summary: [GTK OSX] libGObjectDOMBindings.a is not linked into libwebkit2gtk-4.0.37.dylib
Product: WebKit Reporter: Jeremy Huddleston Sequoia <jeremyhu>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, commit-queue, mcatanzaro, philip.chimento
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Mac   
OS: OS X 10.11   
See Also: https://bugs.webkit.org/show_bug.cgi?id=144557
Bug Depends on:    
Bug Blocks: 126492    
Attachments:
Description Flags
full build log
none
build log
none
patch to add -Wl,-all_load
mcatanzaro: review+, commit-queue: commit-queue-
Updated patch with ChangeLog entry
jeremyhu: review+
Updated patch to also include feedback for removal of now-irrelevant build hack for gobject-intorspection
jeremyhu: review+, mcatanzaro: commit-queue-
Updated reviewers in patch none

Description Jeremy Huddleston Sequoia 2016-01-14 22:05:04 PST
As reported at https://trac.macports.org/ticket/50334

We configure using:

cmake -DCMAKE_INSTALL_PREFIX=/opt/local -DCMAKE_VERBOSE_MAKEFILE=ON -DCMAKE_COLOR_MAKEFILE=ON -DCMAKE_BUILD_TYPE=Release -DCMAKE_BUILD_WITH_INSTALL_RPATH=ON -DCMAKE_INSTALL_RPATH=/opt/local/lib -DCMAKE_INSTALL_NAME_DIR=/opt/local/lib -DCMAKE_SYSTEM_PREFIX_PATH="/opt/local;/usr" -DCMAKE_MODULE_PATH=/opt/local/share/cmake/Modules -DCMAKE_FIND_FRAMEWORK=LAST -Wno-dev -DPORT=GTK -DENABLE_X11_TARGET=ON -DENABLE_QUARTZ_TARGET=OFF -DENABLE_INTROSPECTION=OFF -DLLVM_CONFIG_EXE=/opt/local/bin/llvm-config-mp-3.7 -DENABLE_VIDEO=ON -DENABLE_PLUGIN_PROCESS_GTK2=ON -DCMAKE_C_FLAGS_RELEASE="-DNDEBUG" -DCMAKE_CXX_FLAGS_RELEASE="-DNDEBUG" -DCMAKE_OSX_ARCHITECTURES="x86_64" -DCMAKE_OSX_DEPLOYMENT_TARGET="10.11" -DCMAKE_OSX_SYSROOT="/"

And install libwebkit2gtk-4.0.37.13.0.dylib by copying it from the lib directory to workaround bug #152651.

The resulting libwebkit2gtk-4.0.37.13.0.dylib is missing some symbols, and dependent projects fail to link:

Undefined symbols for architecture x86_64:
  "_webkit_dom_document_get_elements_by_tag_name", referenced from:
      _ephy_web_overview_document_loaded in libephywebextension_la-ephy-web-overview.o
      _ephy_web_dom_utils_get_application_title in libephymisc.a(libephymisc_la-ephy-web-dom-utils.o)
      _ephy_web_dom_utils_get_best_icon in libephymisc.a(libephymisc_la-ephy-web-dom-utils.o)
  "_webkit_dom_dom_window_webkit_message_handlers_post_message", referenced from:
      _request_decision_on_storing in libephywebextension_la-ephy-web-extension.o
  "_webkit_dom_html_input_element_is_edited", referenced from:
      _ephy_web_dom_utils_has_modified_forms in libephymisc.a(libephymisc_la-ephy-web-dom-utils.o)
  "_webkit_dom_html_text_area_element_is_edited", referenced from:
      _ephy_web_dom_utils_has_modified_forms in libephymisc.a(libephymisc_la-ephy-web-dom-utils.o)

---

$ nm /opt/local/lib/libwebkit2gtk-4.0.dylib | egrep '(_webkit_dom_document_get_elements_by_tag_name|_webkit_dom_dom_window_webkit_message_handlers_post_message|_webkit_dom_html_input_element_is_edited|_webkit_dom_html_text_area_element_is_edited)'
0000000000d68f42 T _webkit_dom_document_get_elements_by_tag_name_as_html_collection
0000000000d6968f T _webkit_dom_document_get_elements_by_tag_name_ns_as_html_collection
Comment 1 Jeremy Huddleston Sequoia 2016-01-14 22:24:31 PST
Created attachment 269033 [details]
full build log
Comment 2 Jeremy Huddleston Sequoia 2016-01-14 22:33:39 PST
Those symbols are found in:

Source/WebCore/bindings/gobject/WebKitDOMDeprecated.cpp
Source/WebCore/bindings/gobject/WebKitDOMCustom.cpp

which are linked into libGObjectDOMBindings.a, which is not linked into libwebkit2gtk-4.0.37.13.0.dylib
Comment 3 Jeremy Huddleston Sequoia 2016-01-14 22:44:42 PST
Created attachment 269035 [details]
build log
Comment 4 Jeremy Huddleston Sequoia 2016-01-14 23:19:21 PST
Actually, libGObjectDOMBindings.a is on the link line, but we're missing -all_load.
Comment 5 Jeremy Huddleston Sequoia 2016-01-14 23:21:24 PST
This was caused by an incorrect fix for bug #144557
Comment 6 Jeremy Huddleston Sequoia 2016-01-15 01:23:02 PST
Created attachment 269043 [details]
patch to add -Wl,-all_load
Comment 7 Michael Catanzaro 2016-01-15 07:06:28 PST
Comment on attachment 269043 [details]
patch to add -Wl,-all_load

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

Thanks for your patch! In the future, when you want a review for your patch, please set the r? flag for your patch. If you want us to commit your patch (which you do if you don't have committer access), set the cq? flag. You'll want to set cq? after answering this question:

> Source/cmake/OptionsGTK.cmake:477
> +        list(APPEND ${_list_name} -Wl,-all_load)

Should this be -Wl,-all_load ${library} -Wl,-no-all_load, or does it not work the same as --whole-archive?
Comment 8 Jeremy Huddleston Sequoia 2016-01-15 08:57:43 PST
> Should this be -Wl,-all_load ${library} -Wl,-no-all_load

No.

> or does it not work the same as --whole-archive?

It does not.  There is nothing exactly analogous.  There are two similar options in ld64:

-all_load
Loads all members of static archive libraries.

-force_load path_to_archive
Loads all members of the specified static archive library.  Note: -all_load forces all members
of all archives to be loaded.  This option allows you to target a specific archive.


-force_load would be ideal if we had a contract that the arguments to the macro were always static archives that we built for inclusion in the library.  I tried that first, and it didn't work because the macro is called with arguments other than just static archives that we had built.  -all_load is thus the ideal choice, and it's also used elsewhere in the build already.
Comment 9 Philip Chimento 2016-01-15 09:17:11 PST
Do -all_load and/or -force_load work for you? For me, using either one did allow the build to complete, but MiniBrowser segfaulted on startup (see https://bugs.webkit.org/show_bug.cgi?id=144557#c1)
Comment 10 Jeremy Huddleston Sequoia 2016-01-15 12:16:03 PST
yes, they work for me.  They do what they are supposed to do and symbols are included in the library.

Without this change, clients fail to link due to the missing symbols.

With this change in place, clients are able to link, and we're seeing crashes at launch (https://trac.macports.org/ticket/50339) which we are starting to look at now.  I suspect your crash is likely the same as that one.
Comment 11 Michael Catanzaro 2016-01-15 13:37:59 PST
Hm, I just found this bit of code in Source/WebKit2/PlatformGTK.cmake:

# Manually add some libraries on OSX because we don't have the --whole-archive flag
if (CMAKE_SYSTEM_NAME MATCHES "Darwin")
    set(INTROSPECTION_ADDITIONAL_LIBRARIES --library=c++)
    set(INTROSPECTION_ADDITIONAL_LDFLAGS -lGObjectDOMBindings)
endif ()

Is that still needed?
Comment 12 Philip Chimento 2016-01-15 15:04:37 PST
Probably it can be removed if -all_load works as expected.
Comment 13 Jeremy Huddleston Sequoia 2016-01-15 15:08:03 PST
Yeah, I saw that as well.  I believe that is specific to the gobject-introspection code, but that is currently not working on OSX anyways.  We've disabled it in our (MacPorts) build.  See bug #152650.
Comment 14 WebKit Commit Bot 2016-01-15 19:17:16 PST
Comment on attachment 269043 [details]
patch to add -Wl,-all_load

Rejecting attachment 269043 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 269043, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
153117.
Found no modified ChangeLogs, cannot create a commit message.
All changes require a ChangeLog.  See:
 http://webkit.org/coding/contributing.html
Found no modified ChangeLogs, cannot create a commit message.
All changes require a ChangeLog.  See:
 http://webkit.org/coding/contributing.html
Found no modified ChangeLogs, cannot create a commit message.
All changes require a ChangeLog.  See:
 http://webkit.org/coding/contributing.html
Updating OpenSource
Current branch master is up to date.

Full output: http://webkit-queues.webkit.org/results/696742
Comment 15 Jeremy Huddleston Sequoia 2016-01-15 23:16:27 PST
Created attachment 269157 [details]
Updated patch with ChangeLog entry
Comment 16 Jeremy Huddleston Sequoia 2016-01-15 23:29:28 PST
Created attachment 269159 [details]
Updated patch to also include feedback for removal of now-irrelevant build hack for gobject-intorspection
Comment 17 Michael Catanzaro 2016-01-16 06:43:42 PST
Comment on attachment 269159 [details]
Updated patch to also include feedback for removal of now-irrelevant build hack for gobject-intorspection

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

> Source/WebKit2/ChangeLog:7
> +        Reviewed by Philip Chimento.

Philip is not a reviewer; this line has official significance, so please don't list people here unless they set the r+ flag in Bugzilla.

> Source/cmake/OptionsGTK.cmake:475
>  macro(ADD_WHOLE_ARCHIVE_TO_LIBRARIES _list_name)

It occurs to me that this macro does not belong in OptionsGTK.cmake as it has nothing to do with build options. Once we manage to land this patch, I will move it in a follow-up patch.
Comment 18 Jeremy Huddleston Sequoia 2016-01-16 07:59:54 PST
Created attachment 269168 [details]
Updated reviewers in patch
Comment 19 WebKit Commit Bot 2016-01-16 12:50:35 PST
Comment on attachment 269168 [details]
Updated reviewers in patch

Clearing flags on attachment: 269168

Committed r195174: <http://trac.webkit.org/changeset/195174>
Comment 20 WebKit Commit Bot 2016-01-16 12:50:39 PST
All reviewed patches have been landed.  Closing bug.