If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

ISimpleDOMNode::get_localInterface needs to be remote-friendly

RESOLVED FIXED in Firefox 55

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: aklotz, Assigned: aklotz)

Tracking

(Blocks: 1 bug)

Trunk
mozilla55
Unspecified
Windows
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [JAWS])

Attachments

(1 attachment)

The current implementation of ISimpleDOMNode.idl doesn't work right with a11y+e10s:

1) Proxied ISimpleDOMNode objects will have the vtable slot for get_localInterface set to nullptr. Clients who previously called this function will now crash the browser.
2) The interceptor doesn't like this at all.

I'm going to use the same trick that many system interfaces use. I'm going to declare a "remote" variant of this function in the idl by using the call_as attribute.

This does not change the layout of the vtable in any way, however it *does* change how COM sees the interface. In particular, the code generated by MIDL allows me to add stubs that return E_NOTIMPL, so clients will get that instead of a crash.

Note that the remote variant has to use IUnknown** instead of void** for its outparam (because COM) but this isn't really a problem in practice because the remote variant cannot be invoked anyway.
Blocks: 1350984
Created attachment 8855555 [details] [diff] [review]
Add dummy remoteLocalInterface method to ISimpleDOMNode.idl

See the bug description for patch details.
TL;DR: vtable layout unchanged, but now we can return E_NOTIMPL instead of hitting a null vtable entry.
Attachment #8855555 - Flags: review?(tbsaunde+mozbugs)
Comment on attachment 8855555 [details] [diff] [review]
Add dummy remoteLocalInterface method to ISimpleDOMNode.idl

>+ISimpleDOMNode_get_localInterface_Proxy(ISimpleDOMNode * This,
>+                                        void **localInterface)

might be good for that to be IUnknown** not that it really matters since its C and its unused.
you could also leave it unnamed, but I don't know that worrying about unused variable warnings is worthwhile.

>+ISimpleDOMNode_get_localInterface_Stub(ISimpleDOMNode * This,

I guess the dll has code to call it on the object and the proxy for that object? and that's why you need both.

>   [propget, local] HRESULT localInterface([out][retval] void **localInterface);
> 
>+  [propget, call_as(get_localInterface)]
>+  HRESULT remoteLocalInterface([out][retval] IUnknown **localInterface);

is it really right to have the two definitions?  I guess it is, but midl is weird.
Attachment #8855555 - Flags: review?(tbsaunde+mozbugs) → review+
(In reply to Trevor Saunders (:tbsaunde) from comment #2)
> Comment on attachment 8855555 [details] [diff] [review]
> Add dummy remoteLocalInterface method to ISimpleDOMNode.idl
> 
> >+ISimpleDOMNode_get_localInterface_Proxy(ISimpleDOMNode * This,
> >+                                        void **localInterface)
> 
> might be good for that to be IUnknown** not that it really matters since its
> C and its unused.
> you could also leave it unnamed, but I don't know that worrying about unused
> variable warnings is worthwhile.

I'm more inclined to leave this as-is due to unknown unknowns. (heh)

> 
> >+ISimpleDOMNode_get_localInterface_Stub(ISimpleDOMNode * This,
> 
> I guess the dll has code to call it on the object and the proxy for that
> object? and that's why you need both.

Correct.

> 
> >   [propget, local] HRESULT localInterface([out][retval] void **localInterface);
> > 
> >+  [propget, call_as(get_localInterface)]
> >+  HRESULT remoteLocalInterface([out][retval] IUnknown **localInterface);
> 
> is it really right to have the two definitions?  I guess it is, but midl is
> weird.

Yeah, midl uses the local one for the vtable but the call_as one for the proxy bits.
https://hg.mozilla.org/integration/mozilla-inbound/rev/00cd9d69ecde30cdc6510d81816d12c6256ad926
Bug 1354333: Use call_as to produce a non-null vtable entry for ISimpleDOMNode::get_localInterface; r=tbsaunde

Comment 5

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/00cd9d69ecde
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.