1354333 - ISimpleDOMNode::get_localInterface needs to be remote-friendly

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[(No longer employed by Mozilla) Aaron Klotz]

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.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: 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.

Comment 2

[Trevor Saunders (:tbsaunde)]

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.

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

(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.

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/00cd9d69ecde