Bug 111714 – [GTK][Regression] webkit_dom_html_table_element_insert_row returns value that doesn't pass WEBKIT_DOM_IS_HTML_TABLE_ROW_ELEMENT macro

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Tomas Popela]

webkit_dom_html_table_element_insert_row returns value that doesn't pass WEBKIT_DOM_IS_HTML_TABLE_ROW_ELEMENT macro. It is caused by https://trac.webkit.org/changeset/137074 (git bisect get me to this). Here is some example code:

WebKitDOMElement* table = webkit_dom_document_get_element_by_id ( dom_document, table_id);

WebKitDOMHTMLElement* row = webkit_dom_html_table_element_insert_row ( WEBKIT_DOM_HTML_TABLE_ELEMENT (table), -1, NULL);

ASSERT (!WEBKIT_DOM_IS_HTML_TABLE_ROW_ELEMENT ((WebKitDOMHTMLTableRowElement *) row))

-> if you call this instead of ASSERT
WebKitDOMHTMLElement* cell = webkit_dom_html_table_row_element_insert_cell ( (WebKitDOMHTMLTableRowElement*) row, -1, NULL)
-> you get
** (evolution:16227): CRITICAL **: WebKitDOMHTMLElement* webkit_dom_html_table_row_element_insert_cell(WebKitDOMHTMLTableRowElement*, glong, GError**): assertion `WEBKIT_DOM_IS_HTML_TABLE_ROW_ELEMENT(self)' failed

Here is some code that is used in evolution:
https://git.gnome.org/browse/evolution/tree/modules/itip-formatter/itip-view.c?h=gnome-3-6#n909

Also this patch introduced another thing. When you receive invitation to some event in Evolution you will see TEXTAREA where you can comment this event. After this patch, when you are typing the comment and you will hit SPACE key it doesn't put space into TEXTAREA but instead it just goes to the next unread message (which is normal when you don't have focus on that TEXTAREA). Before this patchset everything was working fine.

[Xan Lopez]

This is odd because the GObject wrapping should be using the actual type of C++ object to create the wrapper, as told by the whatever localName is returning in the C++ instance. So if the function is creating a table row the GObject type should be a table row, independently of what the IDL or WebCore method says.

To debug this you can go to Source/Webcore/bindings/gobject/WebKitDOMBinding.cpp and Source/Webcore/bindings/gobject/WebKitHTMLElementWrappingFactory.cpp and see what specific type is being assigned to the instance you care about.

[Carlos Garcia Campos]

It actually affects all the cases where we are creating DOM object from a polymorphic object using a base class. I'm currently working on it. For the stable branch I'll probably use a quick workaround to cover the HTMLElement cases, but for trunk we need to rework the way we create the objects, I have an initial patch already.

[Carlos Garcia Campos]

Created attachment 194698 [details]
Patch

[Carlos Garcia Campos]

This patch is a simple workaround to cover the HTML elements case, so that we can apply it to the stable branch before releasing without much risk. But the real problem affects all polymorphic objects and requires a lot more work. See bug #113132.

[Martin Robinson]

Comment on attachment 194698 [details]
Patch

Yeah, this seems like something we need to fix more generally. :(

[WebKit Review Bot]

Comment on attachment 194698 [details]
Patch

Clearing flags on attachment: 194698

Committed r146721: <http://trac.webkit.org/changeset/146721>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Xan Lopez]

(In reply to comment #4)
> This patch is a simple workaround to cover the HTML elements case, so that we can apply it to the stable branch before releasing without much risk. But the real problem affects all polymorphic objects and requires a lot more work. See bug #113132.

As we have discussed at length I don't see the point of doing a patch that purely fixes the HTMLElement case, since all HTMLElement subclasses are also broken.

As I suggested I think the simple fix for the stable branch is to change the ::kit implementation in CodeGeneratorGObject for all HTMLElement subclasses (including HTMLElement itself) to do what your patch does only for HTMLElement. This is obviously correct and IMHO low risk, and fixes a giant bug that we should not have in 2.0.

What we can do after 2.0 is fix the completely general case, which would be still broken with the patch I suggest.

[Xan Lopez]

A few more comments after grepping some code:

- I think the only case where we have intermediate HTMLElement subclasses that could trigger this bug is HTMLMediaElement subclasses (Audio and Video element). So adding those to the manually generated ::kit methods would be enough, but I think is a must for 2.0 (in fact they can just proxy HTMLElement, which IMHO should also just proxy Element).

- Our Node ::kit method just calls createWrapper, which in turn just does the right thing for Element, HTMLElement, etc, so all our manual ::kit implementations can be reduced to just this one method IMHO.

Depending on the amount on non Node objects we ship it might be easier to just have a manual list of those and give everything else a ::kit method like the one we use in Node right now, but we can discuss that in the other bug.

[Carlos Garcia Campos]

(In reply to comment #8)
> (In reply to comment #4)
> > This patch is a simple workaround to cover the HTML elements case, so that we can apply it to the stable branch before releasing without much risk. But the real problem affects all polymorphic objects and requires a lot more work. See bug #113132.
> 
> As we have discussed at length I don't see the point of doing a patch that purely fixes the HTMLElement case, since all HTMLElement subclasses are also broken.
> 
> As I suggested I think the simple fix for the stable branch is to change the ::kit implementation in CodeGeneratorGObject for all HTMLElement subclasses (including HTMLElement itself) to do what your patch does only for HTMLElement. This is obviously correct and IMHO low risk, and fixes a giant bug that we should not have in 2.0.

I used the approach I thought it had the lowest risk to be applied in the stable branch two days before the release.

> What we can do after 2.0 is fix the completely general case, which would be still broken with the patch I suggest.

It has always been broken and nobody has noticed it until now, so I don't think it's that important. We can fix the general case in master and try it out for a while, so that when we are sure it works fine, we can merge it into the stable branch for 2.0.1.

[Carlos Garcia Campos]

(In reply to comment #9)
> A few more comments after grepping some code:
> 
> - I think the only case where we have intermediate HTMLElement subclasses that could trigger this bug is HTMLMediaElement subclasses (Audio and Video element). So adding those to the manually generated ::kit methods would be enough, but I think is a must for 2.0 (in fact they can just proxy HTMLElement, which IMHO should also just proxy Element).
> 
> - Our Node ::kit method just calls createWrapper, which in turn just does the right thing for Element, HTMLElement, etc, so all our manual ::kit implementations can be reduced to just this one method IMHO.
> 
> Depending on the amount on non Node objects we ship it might be easier to just have a manual list of those and give everything else a ::kit method like the one we use in Node right now, but we can discuss that in the other bug.

See my patch in the other bug, it uses a different approach based on having a kit method for all objects and some custom wrap methods, more similar to what Objc bindings do.