1478372 - Use CallGetCustomInterface to get nsIDOMXUL* interfaces from native code

Status: RESOLVED FIXED

(Core :: XUL, task, P3)

Description

[Neil Deakin]

Method 1.

Change callers that currently QueryInterface to instead use CallGetCustomInterface via a custom method defined in Element (or nsXULElement)

Some specific callers could check tag names instead where the QI is only used to verify an interface rather than call a method on it.

The disadvantage is that some cases won't be handled. For example, some callers call nsIDOMXULSelectControlElement::GetSelectedItem and then QI the result to nsIContent, which doesn't work. There may be others that might be hard to track down without more extensive testing.

Method 2.

Use a technique like the current implements='nsIXXX' uses where Element::QueryInterface is modified to CallGetCustomInterface. Alternatively, change nsXULElement:QueryInterface and remove the special cased Element::QueryInterface.

The advantage is that existing code (for example the C++ code in accessibility) doesn't need to change, nor script callers.

Comment 1

[Neil Deakin]

Brian and Paolo also favoured option 2 as being less work and less likely to cause regressions.

One other note is that XBL implements="..." is only currently used on XUL elements, so the special-case QI could be moved from Element to nsXULElement, although there was the suggestion that this might not always be the case in the future.

Brian suggested that bz had some thoughts.

Comment 2

[Boris Zbarsky [:bzbarsky]]

So is method 2 to do the thing we have right now where you get back a thing you can actually QI back to the original element?  Or is it to have a way to QI from nsIContent to nsIDOMXULFoo but not be able to QI back?  The former is a bit of a pain, though maybe not too much.  The latter violates expections about how QI works and wuld have the "some cases won't be handled" problem described above for method 1.

My personal preference in the long term is method 1, not least because I want to make QI on elements faster (or ideally eliminate it altogether).  I agree that method 1 requires more work and a bit more risk if our audits miss something, but I think it leaves us in a better architectural place long-term.  This is where I was coming from in the discussion and conclusions referenced in bug 1461742 comment 0 and my API proposal there.

Comment 3

[Neil Deakin]

Attachment: Allow QueryInterface to be used for custom element implemented interfaces

This shows method 2. The test for bug 950909 fails with this patch. I assume we will either need to do the aggregating thing done by GetBindingImplementation or remove this test.

Comment 4

[Neil Deakin]

Attachment: Work in progress, changes needed for method 1

This patch changes most callers that need to call JS-implemented methods of nsIDOMXUL* interfaces. The patch is unfinished and some cases don't work.

Comment 5

[Neil Deakin]

One option that was discussed was to try option 2 to allow work to continue, and then move to option 1 in a followup bug over time where we can handle each or each set of interfaces separately.

Method 2 seems to pass tests, although I'm unclear what portions of GetBindingImplementation may actually be needed (but not covered by tests). I would expect that the aggregating QI may be needed, once we start having some custom elements implementing these interfaces, but the wrapper-related code within GetBindingImplementation is bbeyond my knowledge.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> One option that was discussed was to try option 2 to allow work to continue, and then
> move to option 1 in a followup bug over time where we can handle each or each set of
> interfaces separately.

I would probably be OK with it.

Chances are you will need all the bits that GetBindingImplementation does right now to make option 2 work...

Comment 7

[Dave Townsend [:mossop]]

Attachment: testcase

Additional tests for this feature.

Comment 8

[Neil Deakin]

Attachment: Method 2 - Allow QueryInterface to be used for custom element implemented interfaces

Comment 9

[Neil Deakin]

Comment on attachment 9005654 [details] [diff] [review]
Method 2 - Allow QueryInterface to be used for custom element implemented interfaces

I've tested this quite a bit and it seems to work. I added the check just for nsXULElements.

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 9005654 [details] [diff] [review]
Method 2 - Allow QueryInterface to be used for custom element implemented interfaces

I have a work in progress set of patches for <browser> and can confirm that `browser instanceof Ci.nsIBrowser` and `browser.QueryInterface(Ci.nsIBrowser)` work.

One comment I had was about XUL element subclasses like XULFrameElement. I noticed was that I had to explicitly call:

  MozXULElement.implementCustomInterface(MozBrowser, [Ci.nsIBrowser, Ci.nsIFrameLoaderOwner]);

In order for it to properly Qi into nsIFrameLoaderOwner and get tests like this to pass: https://searchfox.org/mozilla-central/rev/5a18fb5aeeec99f1ca1c36a697082c221189a3b9/dom/tests/mochitest/chrome/test_docshell_swap.xul#32. I wasn't expecting this since the 'browser' XBL binding doesn't set nsIFrameLoaderOwner as [implements], so I assume it's automatically picked up by https://searchfox.org/mozilla-central/rev/5a18fb5aeeec99f1ca1c36a697082c221189a3b9/dom/xul/XULFrameElement.cpp#36 or something.

I don't really mind implementing the extra interface in this case, I'd just like to confirm that this is expected and that it won't cause other problems.

Comment 11

[Neil Deakin]

Attachment: Remove unneeded QueryInterface implementation for custom XUL elements as this is now done by nsXULElement::QueryInterface

This fixes the issue mentioned with implementing nsIFrameLoaderOwner.

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 9006422 [details] [diff] [review]
Remove unneeded QueryInterface implementation for custom XUL elements as this is now done by nsXULElement::QueryInterface

Review of attachment 9006422 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this fixes the issue

Comment 13

[Brian Grinstead [:bgrins]]

It looks like this is leaking - I thought it was my browser patch originally, but after doing some debugging with Andrew with I see a leak on test_custom_element.xul with just these patches applied:

> MOZ_CC_LOG_DIRECTORY=/tmp/cc-log MOZ_CC_LOG_SHUTDOWN=1 MOZ_CC_LOG_PROCESS="main" ./mach mochitest test_custom_element_base.xul

> grep nsGlobalWindow /tmp/cc-log/cc-edges.46502-4.log
...
0x10227f400 [rc=20] nsGlobalWindowInner # 29 inner chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_custom_element_base.xul
...

> python cc/find_roots.py /tmp/cc-log/cc-edges.46502-4.log 0x10227f400

0x132f6aa90 [nsXPCWrappedJS (nsIDOMXULControlElement)]
    --[mJSObj]--> 0x135f64240 [JS Object (Proxy)]
    --[group_global]--> 0x1226db420 [JS Object (Window)]
    --[UnwrapDOMObject(obj)]--> 0x10227f400 [nsGlobalWindowInner # 29 inner chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_custom_element_base.xul]

    Root 0x132f6aa90 is a ref counted object with 3 unknown edge(s).
    known edges:
       0x132f6aa90 [nsXPCWrappedJS (nsIDOMXULControlElement)] --[self]--> 0x132f6aa90

Comment 14

[Peter Van der Beken [:peterv]]

Comment on attachment 9005654 [details] [diff] [review]
Method 2 - Allow QueryInterface to be used for custom element implemented interfaces

Review of attachment 9005654 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/CustomElementRegistry.cpp
@@ +1305,4 @@
>            JSContext* cx = jsapi.cx();
> +
> +          void* resultWrapper;
> +          nsresult rv = xpConnect->WrapJSAggregatedToNative(aElement, cx, customInterface, aIID, &resultWrapper);

Isn't this leaking resultWrapper? You should be able to just has pass in |getter_AddRefs(supports)| here instead of |&resultWrapper|.

Comment 15

[Neil Deakin]

Attachment: allow QueryInterface to be used for custom element implemented interfaces

Comment 16

[Boris Zbarsky [:bzbarsky]]

Is there a followup for converting to option 1?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9008402 [details] [diff] [review]
allow QueryInterface to be used for custom element implemented interfaces

>+      iface.get()->QueryInterface(aIID, aInstancePtr);

Why do you need the .get()?

r=me, I guess, but I'd really like that followup to happen....

Comment 18

[Neil Deakin]

I filed bug 1492326 on that. surkov has been removing some references in accessibility code already.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e69cb7b9206
allow QueryInterface to be used for custom element implemented interfaces, r=bz,bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/f29ac1523010
remove unneeded QueryInterface implementation for custom XUL elements as this is now done by nsXULElement::QueryInterface, r=bgrins

Comment 20

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/7e69cb7b9206
https://hg.mozilla.org/mozilla-central/rev/f29ac1523010