1413287 - Interceptors should allow their own interfaces to be able to be marshaled into handler payloads

Status: VERIFIED FIXED

(Core :: IPC: MSCOM, defect)

Description

[(No longer employed by Mozilla) Aaron Klotz]

This is essentially a problem of reentrancy.

What I think we should do is add a reentry sentinel to the interceptor's marshaling code such that we only do handler marshaling on the outermost marshal request. Inner marshal requests use standard marshaling.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Prevent reentry into handler stuff

For your evaluation

Comment 2

[James Teh [:Jamie] (away returning 28 Jan)]

Comment on attachment 8923975 [details] [diff] [review]
Prevent reentry into handler stuff

Yup, makes sense to me. I had more or less the same idea, but was only able to prevent re-entry for the payload, not GetClassForHandler. In the final patch, it'd be good to have a brief comment explaining why we want to prevent re-entry (so we can marshal interface pointers in the handler payload) just to be super clear.

Comment 3

[James Teh [:Jamie] (away returning 28 Jan)]

I had a play with this today combined with my work on including interface pointers in the IA2 payload. Unfortunately, I'm still seeing infinite recursion on the server side when trying to serialize the payload.

I'm not certain yet, but I think that even when it tries to marshal the IAccessibleText pointer, it seems to be calling the original interceptor's marshaling methods rather than calling it on the IAccessibleText interceptor. The sentinel's mCurInterceptor in the various GetMarshalSizeMax frames refers to the same interceptor. I'm still trying to figure out why.

On another note, it'd be nice if we could include a pointer to the interface being marshaled in its own payload. While this might seem redundant/silly, the nice thing about it is that we don't have to worry about which interface we're marshaling on the handler side. For example, if the caller got an IAccessibleHyperlink directly, we need the IA2_3 pointer in the payload... but it'd be nice not to have to special case this.

Finally, a question: every interceptor has an mInterceptorMap and that is used by GetInterceptorForIID, among other things. But what if someone, for example, QIs from IA2_3 to IAHyperlink and then from IAHyperlink to IA2_3? Wouldn't we end up with two separate interceptors for IA2_3 in that case (the second coming from IAHyperlink's map), since GetInterceptorForIID uses the map but not the live set?

Comment 4

[James Teh [:Jamie] (away returning 28 Jan)]

Interceptors created by GetInterceptorIID delegate their IUnknown to the creating interceptor:

  // (3) Create a new COM interceptor to that interface that delegates its
  // IUnknown to |this|.

Does that not mean QueryInterface for IMarshal will get the IMarshal returned by the outer IUnknown (creating interceptor)? Which in turn means that all marshaling is done by the outer interceptor and thus the re-entry check will never return false.

What that does mean, though, is that it's probably enough to just have a threadsafe variable on the Interceptor instance.

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to James Teh [:Jamie] from comment #4)
> What that does mean, though, is that it's probably enough to just have a
> threadsafe variable on the Interceptor instance.

That's essentially what the patch does, however I didn't write it correctly (I blame my insomnia).

I'm rewriting this to do it right. In particular, I'd like to also be able to handle the case where we are trying to marshal a completely different interceptor (with its own payload) within our payload. Essentially we want to be able to allow reentry only if we have not yet seen that particular interceptor on the stack.

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Prevent reentry into handler stuff (r2)

This one should work much better. Each instance links itself into a thread-local list of active sentinels. When IsOutermost() is called, the top sentinel probes all previous sentinels to see if they are marshaling the same interceptor. If the answer is no, then it returns true.

Comment 7

[James Teh [:Jamie] (away returning 28 Jan)]

This patch was initially causing marshaling to fail for me. The reason is that ThreadSafeQueryInterface creates a marshaler, which ends up calling ThreadSafeQueryInterface to get IStdMarshalInfo. IsOutermost ends up returning false for the IStdMarshalInfo QI, which breaks things. This can be fixed by scoping the ReentrySentinel in ThreadSafeQueryInterface so that it only covers the IStdMarshalInfo bit (which is the only bit where we care about reentry).

While that fixes things on the content side, I'm still running into a problem in the handler with objects retrieved from IAHypertext::hyperlink. In that case, I do provide an IA2 interface pointer in the payload, but that pointer ends up resolving to the handler itself (this), thus causing infinite recursion. I don't understand why this is happening because in that case, the IAccessibleHyperlink is the HandlerProvider's target iid, so I would have thought IA2_3 would be a different pointer.

I haven't had a chance to muddle through that one further. For now, I'm focusing on IMultiQI (bug 1414497), as it'd be good to prove that caching interfaces helps a little before investing a huge amount of time into this one.

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to James Teh [:Jamie] from comment #7)
> While that fixes things on the content side, I'm still running into a
> problem in the handler with objects retrieved from IAHypertext::hyperlink.
> In that case, I do provide an IA2 interface pointer in the payload, but that
> pointer ends up resolving to the handler itself (this), thus causing
> infinite recursion. I don't understand why this is happening because in that
> case, the IAccessibleHyperlink is the HandlerProvider's target iid, so I
> would have thought IA2_3 would be a different pointer.

We'll have to take a closer look at that.

OTOH, as a test I included the objects IEnumVARIANT in the payload, which does marshal correctly. Of course, since it is aggregated, this creates a circular reference.

I have been thinking about our discussions about when (and when not) to release payload interfaces, and I have just realized that we can get the mscom handler code to do this for us: We can detect when our own object is being unmarshaled by a payload and automatically release the interface from there.

Though I suppose that still leaves us with the problem of the remaining interfaces that *do* need releasing... I guess this is only a partial solution. Nonetheless, it's a start and worth noting here for the future.

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Aaron Klotz [:aklotz] (a11y work receiving priority right now, please send interceptor reviews to dmajor or handyman) from comment #8)
> Though I suppose that still leaves us with the problem of the remaining
> interfaces that *do* need releasing... I guess this is only a partial
> solution. Nonetheless, it's a start and worth noting here for the future.

Actually, this is simpler than I think: Once we receive the marshaled interface and it is aggregated into the object's proxy manager, we don't really need that pointer in our payload anymore; we can just null it out.

Any further QIs on the proxy manager will successfully (and locally) resolve to the interface proxy. The payload doesn't even need the interface pointer anymore.

Comment 10

[James Teh [:Jamie] (away returning 28 Jan)]

(In reply to Aaron Klotz [:aklotz] (a11y work receiving priority right now, please send interceptor reviews to dmajor or handyman) from comment #9)
> Actually, this is simpler than I think: Once we receive the marshaled
> interface and it is aggregated into the object's proxy manager, we don't
> really need that pointer in our payload anymore; we can just null it out.
> 
> Any further QIs on the proxy manager will successfully (and locally) resolve
> to the interface proxy. The payload doesn't even need the interface pointer
> anymore.

Ah. This is what I was asking about a few weeks ago when I asked whether COM would just figure out that the pointers in the payload were associated with the same object and just locally resolve the QI. I believe your thoughts at that time were that COM would probably figure this out, but only after you called QI and it made a cross-process request. Is there anything that changed your thinking on this or just more time to think on it? Is there any way to prove that a QI for an interface pointer included in the payload does not make a cross-proc call?

I have a patch set here which already does this for IA2 interfaces. This of course doesn't prevent cross-proc calls for interfaces which aren't available, so I'll need to update it to handle that.

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Prevent reentry into handler stuff (r3)

This revision includes the reduced scope inside the QI.

FYI the logging stuff in this patch is just a straight move into a different location -- some minor code cleanup.

Comment 12

[James Teh [:Jamie] (away returning 28 Jan)]

Comment on attachment 8928332 [details] [diff] [review]
Prevent reentry into handler stuff (r3)

Nice. Ta!

Comment 13

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/664e95ac797c85e6a323abfbaf15d6d3b097974f
Bug 1413287: Ensure that interceptors do not marshal a handler payload unless they are the outermost marshal request; r=Jamie

Comment 14

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/664e95ac797c

Comment 15

[James Teh [:Jamie] (away returning 28 Jan)]

[Tracking Requested - why for this release]:
This is needed for bug 1416986, which significantly improves accessibility performance (which was severely regressed in 57).

Comment 16

[James Teh [:Jamie] (away returning 28 Jan)]

Sorry, misunderstood the flags.

Comment 17

[James Teh [:Jamie] (away returning 28 Jan)]

Comment on attachment 8928332 [details] [diff] [review]
Prevent reentry into handler stuff (r3)

Approval Request Comment
[Feature/Bug causing the regression]: Windows e10s accessibility.
[User impact if declined]: Continued poor performance of Windows accessibility.
[Is this code covered by automated tests?]: No, because there is no framework for automated platform accessibility testing.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: This is needed for bug 1416986.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Straightforward patch to prevent reentry. Only affects accessibility.
[String changes made/needed]: None.

Comment 18

[James Teh [:Jamie] (away returning 28 Jan)]

Used by bug 1416986, which has been verified fixed in Nightly.

Comment 19

[Gerry Chang [:gchang]]

Comment on attachment 8928332 [details] [diff] [review]
Prevent reentry into handler stuff (r3)

Same as comment #35 in bug 1416986. Beta58+.

Comment 20

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/releases/mozilla-beta/rev/780900bb9091