Use CallGetCustomInterface to get nsIDOMXUL* interfaces from native code

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P3
normal
RESOLVED FIXED
9 months ago
6 months ago

People

(Reporter: enndeakin, Assigned: enndeakin)

Tracking

(Blocks 1 bug)

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

9 months ago
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.
(Assignee)

Updated

9 months ago
Priority: -- → P3
(Assignee)

Comment 1

9 months ago
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.
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 3

9 months ago
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.
(Assignee)

Comment 4

9 months ago
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.
Assignee: nobody → enndeakin
Blocks: 1481949
Blocks: 1441935
(Assignee)

Comment 5

9 months ago
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.
> 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...
Posted patch testcase (obsolete) — Splinter Review
Additional tests for this feature.
Blocks: 1455433
(Assignee)

Comment 8

8 months ago
Attachment #8998543 - Attachment is obsolete: true
Attachment #9004381 - Attachment is obsolete: true
(Assignee)

Comment 9

8 months ago
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.
Attachment #9005654 - Flags: review?(bzbarsky)
Attachment #9005654 - Flags: feedback?(bgrinstead)
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.
Attachment #9005654 - Flags: feedback?(bgrinstead) → feedback+
(Assignee)

Comment 11

8 months ago
This fixes the issue mentioned with implementing nsIFrameLoaderOwner.
Attachment #9006422 - Flags: review?(bgrinstead)
(Assignee)

Updated

8 months ago
Attachment #9006422 - Attachment description: qi-noagg → Remove unneeded QueryInterface implementation for custom XUL elements as this is now done by nsXULElement::QueryInterface
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
Attachment #9006422 - Flags: review?(bgrinstead) → review+
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 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|.
(Assignee)

Comment 15

7 months ago
Attachment #9005654 - Attachment is obsolete: true
Attachment #9005654 - Flags: review?(bzbarsky)
Attachment #9008402 - Flags: review?(bzbarsky)
(Assignee)

Updated

7 months ago
Attachment #9008402 - Flags: review?(bzbarsky) → review?(peterv)
Is there a followup for converting to option 1?
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....
Attachment #9008402 - Flags: review?(peterv) → review+
(Assignee)

Updated

7 months ago
Blocks: 1492326
(Assignee)

Comment 18

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

Comment 19

7 months ago
Pushed by neil@mozilla.com:
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

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7e69cb7b9206
https://hg.mozilla.org/mozilla-central/rev/f29ac1523010
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.