201166 - allow XBL elements to QI to base interfaces

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[[e-mail]]

As of bug 112789 radiogroups now implement nsIDOMXULControlElement so that the
event state manager can easily tell if they are disabled or not. While I was
fixing that bug I noticed that although nobody had previously implemented
nsIDOMXULControlElement nsFormControlAccessible.cpp tries to query for this
interface. I was wondering if you had been expecting this to succeed at all?

Comment 1

[John Gaunt (redfive)]

if you check lxr (
http://lxr.mozilla.org/seamonkey/search?string=nsIDOMXULControlElement ) you'll
see there are actually a couple places that impl this interface. So yes, we do
expect it to succeed. :-)

Comment 2

[[e-mail]]

Sorry, I wasn't clear, nobody claimed to implement nsIDOMXULControlElement until
support was recently added to radio.xml - note that implementing a derived
interface is not sufficient for QueryInterface(nsIDOMXULControlElement).

Comment 3

[John Gaunt (redfive)]

I'm still not sure what the bug here is? Are you saying nsFormControlAccessible
is QIing to something that will never succeed? I see XBL files that implement
nsIDOMXULSelectControlElement which inherits from nsIDOMXULControlElement:
radio.xml, listbox.xml, tabbox.xml, menulist.xml. If you look where we QI in
nsFormAccessible.cpp
(http://lxr.mozilla.org/seamonkey/source/accessible/src/base/nsFormControlAccessible.cpp#62)
you should see that we are getting the value for the accessbility state from a
XUL form control (the test for the HTML form control having failed in the
beginning of the method. This check was added for radio groups, IIRC.

So is it that QI you are saying should fail or is failing?

And if I implement nsIFoo, which inherits from nsIBar and then try to QI to
nsIBar, that should work. Unless the implementation (the QI method itself) is
messed up. That's one of the main points behind interface pointers, the ability
to get different veiws/APIs of an object that might implement many interfaces.

So I'm confused, if you can straighten me out on what exactly the bug is here
I'll glady tackle it. :-)

Comment 4

[[e-mail]]

OK, here's the spiel: QI doesn't know about inheritance. So, just beacuse you
can QI to nsIDOMXULSelectControlElement doesn't mean that you can QI to
nsIDOMXULControlElement. And you can't NS_STATIC_CAST either, because that
breaks the laws of XPCOM. To get radiogroups to play nice with the ESM we had to
explicitly list nsIDOMXULControlElement in the allowed QI list.

Hope that makes sense.

Comment 5

[John Gaunt (redfive)]

So I talked to jst and think I understand your arguement now. And after looking
at the code I agree with you that as written it sure doesn't look like it would
work, but I'm SURE I tested it when I wrote it. I can't explain why it worked
then and doesn't work now.

But, this is good because it has exposed a possible problem with the way QI
tables are created for objects defined in XBL. The only interfaces you can QI to
are the ones listed in the XBL file in the |implements| property. So I'm going
to work up a patch that populates the QI table with the inherited interfaces as
well. See, in C++ you get a chance to do that using the QI macros, but not so in
XBL. 

Of course, fixing this means solutions like the one in bug 112789 (specifically
adding the base interface to the implements list) are no longer neccessary. In
fact in talking with jst he felt that might be detrimental, causing methods to
be added multiple times through xpconnect. I didn't understand it all, but it
seemed plausible and reasonable. I'm sure he'll comment if he feels the need. :-)

I've run the concept by bryner and jst and they agree and I've sent an email to
hyatt. cc-ing all here. 

Neil, if I'm still missing the boat on this bug let me know.

changing summary to align with actual focus of bug.

Comment 6

[[e-mail]]

Sounds good to me. Thanks for looking into this.

Comment 7

[John Gaunt (redfive)]

Attachment: add parent interfaces to binding's interface table

Check for parents of interfaces defined in the xbl file, add them to the table.
Exclude nsISupports as the DOMNode and not the binding should handle that. Also
check to make sure the binding is never asked for the nsISupports (the
NS_ASSERTION in nsBindingManager.cpp)

Comment 8

[John Gaunt (redfive)]

Also requesting hyatt take a look at the patch, in addition to the 2 reviewers
specified in the patch tool thingy.

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 122272 [details] [diff] [review]
add parent interfaces to binding's interface table

- In nsXBLPrototypeBinding::ConstructInterfaceTable():

	 // We found a valid iid.  Add it to our table.
	 nsIIDKey key(*iid);
	 mInterfaceTable->Put(&key, mBinding);
+
+	 // get the InterfaceInfo for the iid
+	 nsCOMPtr<nsIInterfaceInfo> iinfo;
+	 infoManager->GetInfoForIID(iid, getter_AddRefs(iinfo));

This'll work, but it's a bit backwards. First we get the IID for the interface
name (hash lookup), then we get the nsIInterfaceInfo for that IID (another hash
lookup). In stead of doing that, you should change more of this code to get the
nsIInterfaceInfo for the interface name, then get the IID from that; this would
cut out one hash lookup. Also, above the code you changed, there's this:

    char* str = ToNewCString(aImpls);

and below, str is freed. There are two problems with that, if someone passes in
a unicode string, we could find an IID we don't want since we're stripping out
bits from the data when converting the unicode string to ASCII. Replace that
with:

    char* str = NS_CONST_CAST(char *, NS_ConvertUCS2toUTF8(aImpls).get());

and eliminate the free of str.

...
+	     // don't add nsISupports to the table
+	     if (!iid || iid->Equals(NS_GET_IID(nsISupports)))
+	       break;
...
+
+	 // free the nsMemory::Clone()ed iid
	 nsMemory::Free(iid);

If the above loop breaks out because !iid, then you'd free null here. Add a
null check, or return early if you can.

Fix that, and I'll have one more look.

Comment 10

[[e-mail]]

Comment on attachment 122272 [details] [diff] [review]
add parent interfaces to binding's interface table

Don't forget to back out the workaround in radio.xml

Comment 11

[John Gaunt (redfive)]

Attachment: revised patch, including fixes to xbl files

points from jst's review addressed

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 122361 [details] [diff] [review]
revised patch, including fixes to xbl files

+    char* str = NS_CONST_CAST(char *, NS_ConvertUCS2toUTF8(aImpls).get());

This is wrong (my bad), this'll crash (on some platforms, at least) since the
data you're pointing at goes out of scope and can be overwritten immediately
after this line. Make that:

+    NS_ConvertUCS2toUTF8 utf8impl(aImpls);
+    char* str = NS_CONST_CAST(char *, utf8impl.get());

That way str is valid until utf8impl goes out os scope.

+	   while (NS_SUCCEEDED(iinfo->GetParent(getter_AddRefs(parentInfo))) &&
parentInfo) {
...
+	     // look for the next parent
+	     iinfo = parentInfo;
+	     parentInfo = nsnull;
+	   }

No need to null out parentInof here, getter_AddRefs() does that for you (when
on the next iteration).

sr=jst with those changes.

Comment 13

[Brian Ryner (not reading)]

Comment on attachment 122361 [details] [diff] [review]
revised patch, including fixes to xbl files

fix the same things jst mentioned, r=bryner.

Comment 14

[John Gaunt (redfive)]

thanks bryner, jst. 

Those fixes have been made, just waiting on approval now.

Comment 15

[Asa Dotzler [:asa]]

Comment on attachment 122361 [details] [diff] [review]
revised patch, including fixes to xbl files

a=asa (on behalf of drivers) for checkin to 1.4

Comment 16

[John Gaunt (redfive)]

checked in