Bug 93477 – 1.9.90 drops symbols, breaking compatibility

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

The Debian,Ubuntu build system do keep track of the symbols exported by libraries, the 1.9.2 to 1.9.6 update ran into warnings about symbols being dropped, some were in the public api (install .h)

+#MISSING: 1.9.6# (optional|c++)"DumpRenderTreeSupportGtk::counterValueForElementById(_WebKitWebFrame*, char const*)@Base" 1.3.13
+#MISSING: 1.9.6# (optional|c++)"DumpRenderTreeSupportGtk::decrementAccessibilityValue(_AtkObject*)@Base" 1.5.1
+#MISSING: 1.9.6# (optional|c++)"DumpRenderTreeSupportGtk::dumpConfigurationForViewport(_WebKitWebView*, int, int, int, int, int)@Base" 1.3.13
+#MISSING: 1.9.6# (optional|c++)"DumpRenderTreeSupportGtk::incrementAccessibilityValue(_AtkObject*)@Base" 1.5.1
+#MISSING: 1.9.6# (optional|c++)"DumpRenderTreeSupportGtk::isPageBoxVisible(_WebKitWebFrame*, int)@Base" 1.3.13
+#MISSING: 1.9.6# (optional|c++)"DumpRenderTreeSupportGtk::pageNumberForElementById(_WebKitWebFrame*, char const*, float, float)@Base" 1.3.13
+#MISSING: 1.9.6# (optional|c++)"DumpRenderTreeSupportGtk::resumeAnimations(_WebKitWebFrame*)@Base" 1.3.13
+#MISSING: 1.9.6# (optional|c++)"DumpRenderTreeSupportGtk::setHixie76WebSocketProtocolEnabled(_WebKitWebView*, bool)@Base" 1.7.4
+#MISSING: 1.9.6# (optional|c++)"DumpRenderTreeSupportGtk::suspendAnimations(_WebKitWebFrame*)@Base" 1.3.13
+#MISSING: 1.9.6# GetBehavior@Base 1.7.4
+#MISSING: 1.9.6# (optional|c++)"WebCore::ClientRectList::ClientRectList(WTF::Vector<WebCore::FloatQuad, 0ul> const&)@Base" 1.9.2
+#MISSING: 1.9.6# (optional|c++)"WebCore::ComposedShadowTreeWalker::ComposedShadowTreeWalker(WebCore::Node const*, WebCore::ComposedShadowTreeWalker::Policy)@Base" 1.9.2
+#MISSING: 1.9.6# (optional|c++)"WebCore::Element::hasShadowRoot() const@Base" 1.7.90
+#MISSING: 1.9.6# (optional|c++)"WebCore::Element::shadowTree() const@Base" 1.9.2
+#MISSING: 1.9.6# (optional|c++)"WebCore::FrameDestructionObserver::FrameDestructionObserver(WebCore::Frame*)@Base" 1.7.5
+#MISSING: 1.9.6# (optional|c++)"WebCore::FrameDestructionObserver::frameDestroyed()@Base" 1.7.5
+#MISSING: 1.9.6# (optional|c++)"WebCore::FrameDestructionObserver::observeFrame(WebCore::Frame*)@Base" 1.7.5
+#MISSING: 1.9.6# (optional|c++)"WebCore::FrameDestructionObserver::willDetachPage()@Base" 1.9.2
+#MISSING: 1.9.6# (optional|c++)"WebCore::FrameDestructionObserver::~FrameDestructionObserver()@Base" 1.7.5
+#MISSING: 1.9.6# (optional|c++)"WebCore::ShadowTree::removeAllShadowRoots()@Base" 1.9.2
+#MISSING: 1.9.6# (optional|c++)"WebCore::TextIterator::getLocationAndLengthFromRange(WebCore::Element*, WebCore::Range const*, unsigned long&, unsigned long&)@Base" 1.7.4
+#MISSING: 1.9.6# (optional|c++)"WebCore::TextIterator::rangeFromLocationAndLength(WebCore::Element*, int, int, bool)@Base" 1.7.4
+#MISSING: 1.9.6# (optional|c++)"WebCore::jsStringSlowCase(JSC::ExecState*, WTF::HashMap<WTF::StringImpl*, JSC::Weak<JSC::JSString>, WTF::StringHash, WTF::HashTraits<WTF::StringImpl*>, WTF::HashTraits<JSC::Weak<JSC::JSString> > >&, WTF::StringImpl*)@Base" 1.5.2
+#MISSING: 1.9.6# (optional|c++)"WebCore::overrideUserPreferredLanguages(WTF::Vector<WTF::String, 0ul> const&)@Base" 1.7.5
...
+#MISSING: 1.9.6# webkit_dom_document_set_document_uri@Base 1.3.10
+#MISSING: 1.9.6# webkit_dom_html_element_get_class_name@Base 1.3.10
+#MISSING: 1.9.6# webkit_dom_html_element_set_class_name@Base 1.3.10
...
+#MISSING: 1.9.6# webkit_dom_webkit_named_flow_get_content_nodes@Base 1.9.2
+#MISSING: 1.9.6# webkit_dom_webkit_named_flow_get_overflow@Base 1.7.90
+#MISSING: 1.9.6# webkit_dom_webkit_named_flow_get_regions_by_content_node@Base 1.9.2
...
+#MISSING: 1.9.6# webkit_video_sink_get_type@Base 1.3.10
+#MISSING: 1.9.6# webkit_video_sink_new@Base 1.3.10


Not sure about the c++ ones but at least those are listed in the public apis:

webkit_dom_webkit_named_flow_get_overflow
webkit_dom_document_set_document_uri
webkit_dom_html_element_get_class_name
webkit_dom_html_element_set_class_name

[Xan Lopez]

Created attachment 157433 [details]
Patch

[Xan Lopez]

As for the named flow issue, the property was renamed from 'overflow' to 'overset', that's why it's missing. I'd suggest to just add dummy accessors to keep API compatibility, and make them print a warning explaining the property has changed its name. I think this makes more sense because:

- I bet this thing is pretty much not used at all.
- Adding a proxy property that forwards to the new one would be cumbersome.
- We already did this once (check WebKitDOMCustom), without any real problems.

[WebKit Review Bot]

Comment on attachment 157433 [details]
Patch

Rejecting attachment 157433 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ONFLICT (content): Merge conflict in Source/WebKit/chromium/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Web Inspector: improve large array logging experience

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.

Full output: http://queues.webkit.org/results/13452994

[Xan Lopez]

Comment on attachment 157433 [details]
Patch

mmm, ok, let's try again

[WebKit Review Bot]

Comment on attachment 157433 [details]
Patch

Clearing flags on attachment: 157433

Committed r125183: <http://trac.webkit.org/changeset/125183>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Garcia Campos]

(In reply to comment #2)
> As for the named flow issue, the property was renamed from 'overflow' to 'overset', that's why it's missing. I'd suggest to just add dummy accessors to keep API compatibility, and make them print a warning explaining the property has changed its name. I think this makes more sense because:
> 
> - I bet this thing is pretty much not used at all.
> - Adding a proxy property that forwards to the new one would be cumbersome.
> - We already did this once (check WebKitDOMCustom), without any real problems.

You mean adding dummy accessors, but not the property itself? Can't we add also the property in addition to the new one, with warnings in get/set_property?

[Xan Lopez]

(In reply to comment #7)
> You mean adding dummy accessors, but not the property itself? Can't we add also the property in addition to the new one, with warnings in get/set_property?

You can, but as I said it's more complicated. You have to change the perl scripts to special case this. IMHO it's not really worth it.

[Carlos Garcia Campos]

(In reply to comment #8)
> (In reply to comment #7)
> > You mean adding dummy accessors, but not the property itself? Can't we add also the property in addition to the new one, with warnings in get/set_property?
> 
> You can, but as I said it's more complicated. You have to change the perl scripts to special case this. IMHO it's not really worth it.

hmm, that would be an api break, I'm not sure if a removed property breaks the ABI too. If the ABI is broken, I think it's worth to properly fix it to not break it.

Reopening, 1.9.90 still breaks api, it drops those:

webkit_dom_element_get_webkit_region_overflow@Base 1.7.90
webkit_dom_html_element_get_class_list@Base 1.3.10
webkit_dom_webkit_named_flow_get_content_nodes@Base 1.9.2
webkit_dom_webkit_named_flow_get_regions_by_content_node@Base 1.9.2
webkit_video_sink_get_type@Base 1.3.10
webkit_video_sink_new@Base 1.3.10

the webkit_dom symbols are least used to be part of installed headers and are an api break

webkit_dom_html_element_get_class_list() for example seems to be used in geary (http://redmine.yorba.org/projects/geary/wiki)

it's used in pidgin as well...

[Adam Barth]

Is there some way we can catch these earlier?

> Is there some way we can catch these earlier?

If you want some details on the Debian packaging tool:
http://lists.debian.org/debian-devel/2007/06/msg00197.html

But basically run "nm -D libwebkit*.so > symbols.$version" for on n-1 and n (you could do it by commit granularity if you rebuild on every commit) and flag something red when T symbols are dropped

one other thing which should be easy to do is to diff the include directories of both version, if definitions got dropped from a public .h you have issues...

[Adam Barth]

Those are all too hard.  How about having the GTK bubble turn red when we screw this up?

[Martin Robinson]

We could probably add this as a build step.

[Xan Lopez]

Created attachment 161261 [details]
Patch

[Xan Lopez]

(In reply to comment #18)
> Created an attachment (id=161261) [details]
> Patch

This should do the trick, the changes are mostly harmless renames.

The only other API break I'm aware of is the one for cloneNode, but that one is trickier. Before solving it I'd like to get a reply to my question here https://bugs.webkit.org/show_bug.cgi?id=91704#c11, maybe Adam knows what's up with that.

[WebKit Review Bot]

Attachment 161261 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/bindings/gobject/WebKitDOMCustom.h:39:  The parameter name "element" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/gobject/WebKitDOMCustom.h:40:  The parameter name "element" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/gobject/WebKitDOMCustom.h:41:  The parameter name "namedFlow" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/gobject/WebKitDOMCustom.h:42:  The parameter name "namedFlow" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Xan Lopez]

(In reply to comment #16)
> Those are all too hard.  How about having the GTK bubble turn red when we screw this up?

The thing is that the tool gives a fair amount of false positives, so I'm not sure how easy it would be to completely automate this. We would probably need to keep a list of all the actual API we want to maintain, and make the bot turn red if anything in that list goes away or changes.

[Martin Robinson]

It seems like we could limit the (In reply to comment #21)
> (In reply to comment #16)
> > Those are all too hard.  How about having the GTK bubble turn red when we screw this up?
> 
> The thing is that the tool gives a fair amount of false positives, so I'm not sure how easy it would be to completely automate this. We would probably need to keep a list of all the actual API we want to maintain, and make the bot turn red if anything in that list goes away or changes.

Perhaps we could limit the false positives by limiting the scope of the API watcher to the DOM APIs, which shouldn't change. I imagine this could work something like the bindings tests.

[Xan Lopez]

Created attachment 161309 [details]
Patch

[Martin Robinson]

Comment on attachment 161309 [details]
Patch

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

> Source/WebCore/bindings/gobject/WebKitDOMCustom.cpp:76
> +WebKitDOMDOMTokenList*
> +webkit_dom_html_element_get_class_list(WebKitDOMHTMLElement* element)
> +{
> +    return webkit_dom_element_get_class_list(WEBKIT_DOM_ELEMENT(element));
> +}

You should probably follow WebKit style conventions here and keep the method signature on one line.

[Xan Lopez]

(In reply to comment #24)
> You should probably follow WebKit style conventions here and keep the method signature on one line.

Yeah, although amusingly the warning is not because of that. I'm just following the existing style, we should do a patch fixing everything.

[WebKit Review Bot]

Comment on attachment 161309 [details]
Patch

Clearing flags on attachment: 161309

Committed r127124: <http://trac.webkit.org/changeset/127124>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.