Bug 123663 - [GTK] Missing symbols in 2.3.1, ABI break?
Summary: [GTK] Missing symbols in 2.3.1, ABI break?
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-11-02 04:24 PDT by Emilio Pozuelo Monfort
Modified: 2013-11-05 05:00 PST (History)
4 users (show)

See Also:


Attachments
Patch (5.76 KB, patch)
2013-11-03 15:18 PST, Emilio Pozuelo Monfort
no flags Details | Formatted Diff | Diff
Patch (5.84 KB, patch)
2013-11-05 03:59 PST, Emilio Pozuelo Monfort
no flags Details | Formatted Diff | Diff
Patch (5.82 KB, patch)
2013-11-05 04:26 PST, Emilio Pozuelo Monfort
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Emilio Pozuelo Monfort 2013-11-02 04:24:06 PDT
libwebkitgtk-1.0.so, libwebkitgtk-3.0.so and libwebkit2gtk-3.0.so used to have these symbols:

 webkit_dom_html_head_element_get_profile@Base 1.3.10
 webkit_dom_html_head_element_set_profile@Base 1.3.10
 webkit_dom_processing_instruction_get_data@Base 1.3.10
 webkit_dom_processing_instruction_set_data@Base 1.3.10

They were exported in public headers, but are now gone. Isn't this an ABI break or is there a special reason why these were considered private?
Comment 1 Zan Dobersek 2013-11-02 07:38:06 PDT
These are (were) generated GObject bindings functions that had their underlying implementation removed. Stubs should be put in place, I believe.
Comment 2 Carlos Garcia Campos 2013-11-02 07:53:28 PDT
Yes, we need to bring the symbols back.
Comment 3 Emilio Pozuelo Monfort 2013-11-03 07:17:19 PST
I'm testing a patch for this.
Comment 4 Emilio Pozuelo Monfort 2013-11-03 15:18:55 PST
Created attachment 215881 [details]
Patch
Comment 5 Emilio Pozuelo Monfort 2013-11-03 15:20:13 PST
(In reply to comment #4)
> Created an attachment (id=215881) [details]
> Patch

The patch is working fine for webkit_dom_processing_instruction_[gs]et_data but not for webkit_dom_html_head_element_[gs]et_profile for some reason...
Comment 6 Emilio Pozuelo Monfort 2013-11-03 15:28:24 PST
(In reply to comment #5)
> (In reply to comment #4)
> > Created an attachment (id=215881) [details] [details]
> > Patch
> 
> The patch is working fine for webkit_dom_processing_instruction_[gs]et_data but not for webkit_dom_html_head_element_[gs]et_profile for some reason...

Maybe I need to add them to WebCore/bindings/gobject/webkitdom.symbols instead of to WebKitDOMCustom.symbols. Carlos, you have written all this symbols logic, any idea about this?
Comment 7 Emilio Pozuelo Monfort 2013-11-03 15:31:16 PST
It looks like webkitdom.symbols is a superset of WebKitDOMCustom.symbols but is not autogenerated, perhaps WebKitDOMCustom.symbols is obsolete?
Comment 8 Carlos Garcia Campos 2013-11-03 23:38:59 PST
(In reply to comment #7)
> It looks like webkitdom.symbols is a superset of WebKitDOMCustom.symbols but is not autogenerated, perhaps WebKitDOMCustom.symbols is obsolete?

webkitdom.symbols contains all the API symbols and it's generated by the script gobject-run-api-break-test when manually run with --reset-results. The patch looks correct, you have to add the symbols to WebKitDOMCustom.symbols. What's exactly the error you are getting?
Comment 9 Emilio Pozuelo Monfort 2013-11-04 00:53:09 PST
I'm not getting any error, but the resulting libwebkitgtk-[13].0.so.0 export webkit_dom_processing_instruction_[gs]et_data but not webkit_dom_html_head_element_[gs]et_profile as they should.
Comment 10 Carlos Garcia Campos 2013-11-04 06:32:23 PST
(In reply to comment #9)
> I'm not getting any error, but the resulting libwebkitgtk-[13].0.so.0 export webkit_dom_processing_instruction_[gs]et_data but not webkit_dom_html_head_element_[gs]et_profile as they should.

That's weird. The .symbols file is only to check API breaks while building and to generate the API docs, it doesn't affect in any way the symbols actually exported in the lib. Any public symbol prefixed with WEBKIT_API should be public in the lib.
Comment 11 Emilio Pozuelo Monfort 2013-11-04 15:45:44 PST
I have tried with git master (I was testing on 2.3.1 before), same results:

emilio@titan:/opt/WebKit/lib$ nm -D libwebkitgtk-1.0.so.0 | grep webkit_dom_html_head_element_get_profile
emilio@titan:/opt/WebKit/lib$ nm -D libwebkitgtk-1.0.so.0 | grep webkit_dom_processing_instruction_get_data
0000000001091270 T webkit_dom_processing_instruction_get_data


Interesting though:
emilio@titan:/opt/WebKit/lib$ grep webkit_dom_html_head_element_get_profile libwebkitgtk-1.0.so.0.20.0
Binary file libwebkitgtk-1.0.so.0.20.0 matches


Poking a bit more:

emilio@titan:~/src/WebKit/.libs$ nm libGObjectDOMBindings.a | grep webkit_dom_html_head_element_get_profile
0000000000001020 T _Z40webkit_dom_html_head_element_get_profileP25_WebKitDOMHTMLHeadElement
00000000000000c0 r _ZZ40webkit_dom_html_head_element_get_profileP25_WebKitDOMHTMLHeadElementE8__func__
emilio@titan:~/src/WebKit/.libs$ nm libGObjectDOMBindings.a | grep webkit_dom_processing_instruction_get_data
0000000000001070 T webkit_dom_processing_instruction_get_data
0000000000000040 r _ZZ42webkit_dom_processing_instruction_get_dataE8__func__


emilio@titan:~/src/WebKit/.libs$ echo "_Z40webkit_dom_html_head_element_get_profileP25_WebKitDOMHTMLHeadElement" | c++filt 
webkit_dom_html_head_element_get_profile(_WebKitDOMHTMLHeadElement*)
emilio@titan:~/src/WebKit/.libs$ echo "_ZZ42webkit_dom_processing_instruction_get_dataE8__func__" | c++filt 
webkit_dom_processing_instruction_get_data::__func__


I don't know much about c++ symbol mangling but that seems suspicious.
Comment 12 Carlos Garcia Campos 2013-11-05 01:52:38 PST
I've just build with your patch (after fixing the conflicts) and it seems to work fine here:

$ nm -D libwebkitgtk-3.0.so.0.20.0 | grep webkit_dom_html_head_element_get_profile
00000000019a57a0 T webkit_dom_html_head_element_get_profile
$ nm -D libwebkitgtk-3.0.so.0.20.0 | grep webkit_dom_processing_instruction_get_data
00000000019a57f0 T webkit_dom_processing_instruction_get_data
$ nm -D libwebkit2gtk-3.0.so.25.5.0 | grep webkit_dom_html_head_element_get_profile
0000000001f1ceb0 T webkit_dom_html_head_element_get_profile
$ nm -D libwebkit2gtk-3.0.so.25.5.0 | grep webkit_dom_processing_instruction_get_data
0000000001f1cf00 T webkit_dom_processing_instruction_get_data
Comment 13 Emilio Pozuelo Monfort 2013-11-05 01:53:58 PST
Do you have GCC 4.8 ?
Comment 14 Carlos Garcia Campos 2013-11-05 01:55:29 PST
Comment on attachment 215881 [details]
Patch

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

Please make sure the patch applies in current git master.

> Source/WebCore/bindings/gobject/WebKitDOMCustom.cpp:563
> +// WebKitDOMHTMLElement

This should be WebKitDOMHTMLHeadElement

> Source/WebCore/bindings/gobject/WebKitDOMCustom.h:478
> + * Returns:

Add something after the Returns: tag to avoid a warning when generating docs. It could be just Returns: a #gchar

> Source/WebCore/bindings/gobject/WebKitDOMCustom.h:481
> +WEBKIT_API gchar* webkit_dom_html_head_element_get_profile(void* self);

Use WebKitDOMHTMLHeadElement* instead of void*. We use only void* when the whole class has been removed from the API.

> Source/WebCore/bindings/gobject/WebKitDOMCustom.h:493
> + * Returns:
> + *
> +**/
> +WEBKIT_API void webkit_dom_html_head_element_set_profile(void* self, const gchar* value);

Same comments here.
Comment 15 Carlos Garcia Campos 2013-11-05 01:56:23 PST
(In reply to comment #13)
> Do you have GCC 4.8 ?

g++ (Debian 4.8.2-1) 4.8.2
Comment 16 Emilio Pozuelo Monfort 2013-11-05 03:59:40 PST
Created attachment 216020 [details]
Patch
Comment 17 Emilio Pozuelo Monfort 2013-11-05 04:02:27 PST
New patch attached addressing all your comments.

(In reply to comment #14)
> > Source/WebCore/bindings/gobject/WebKitDOMCustom.h:481
> > +WEBKIT_API gchar* webkit_dom_html_head_element_get_profile(void* self);
> 
> Use WebKitDOMHTMLHeadElement* instead of void*. We use only void* when the whole class has been removed from the API.
> 
> > Source/WebCore/bindings/gobject/WebKitDOMCustom.h:493
> > + * Returns:
> > + *
> > +**/
> > +WEBKIT_API void webkit_dom_html_head_element_set_profile(void* self, const gchar* value);
> 
> Same comments here.

I think that may have been the cause of the problem I was seeing (which I don't anymore) as the prototype in WebKitDOMCustom.h and WebKitDOMCustom.cpp differed. Everything's working fine now.
Comment 18 Carlos Garcia Campos 2013-11-05 04:10:46 PST
Comment on attachment 216020 [details]
Patch

Thanks!
Comment 19 WebKit Commit Bot 2013-11-05 04:12:20 PST
Comment on attachment 216020 [details]
Patch

Rejecting attachment 216020 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 216020, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/19578976
Comment 20 Emilio Pozuelo Monfort 2013-11-05 04:26:49 PST
Created attachment 216023 [details]
Patch
Comment 21 WebKit Commit Bot 2013-11-05 05:00:56 PST
Comment on attachment 216023 [details]
Patch

Clearing flags on attachment: 216023

Committed r158662: <http://trac.webkit.org/changeset/158662>
Comment 22 WebKit Commit Bot 2013-11-05 05:00:59 PST
All reviewed patches have been landed.  Closing bug.