1007577 - [RTSP] Remove nsIInterfaceRequestor from RtspControllerParent

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RTSP, defect)

Description

[Ethan Tseng [:ethan]]

RtspControllerParent inherits from the interface nsIInterfaceRequestor.
But actually we think no object would call RtspControllerParent's GetInterface().
Besides, RtspControllerParent only implements the interface nsIStreamingProtocolListener. It seems there is not much value to provide GetInterface() utility.

Thus, I suggest to remove it from RtspControllerParent.

Comment 1

[Ethan Tseng [:ethan]]

By the way, I was aware of this issue while working on bug 1006530.
RtspControllerParent inherits from nsIInterfaceRequestor and nsIStreamingProtocolListener, which both inherit from nsISupports.
This causes an "ambiguous base" compile error while passing RtspControllerParent pointer to a function as an nsISupports argument.
We can remedy this kind of error by static_cast or using NS_INTERFACE_MAP_ENTRY_AMBIGUOUS().
However, since RtspControlleParent doesn't need nsIInterfaceRequestor at all, we should simply remove it.

Comment 2

[Ethan Tseng [:ethan]]

Attachment: bug-1007577-fix.patch

Steve, can you help to review this minor fix?

Comment 3

[Steve Workman [:sworkman] (INACTIVE)]

How sure are you that nothing calls GetInterface? It might be an idea to have a patch that calls MOZ_CRASH in GetInterface. Land that and then run it for a couple of weeks just to make sure.

Just an idea. If you're convinced that GetInterface isn't called, I'll r+ the current patch.

Comment 4

[Ethan Tseng [:ethan]]

Attachment: Remove nsIInterfaceRequestor from RtspControllerParent

Rebase because of bug 1006530.

Comment 5

[Ethan Tseng [:ethan]]

(In reply to Steve Workman [:sworkman] from comment #3)
> How sure are you that nothing calls GetInterface? It might be an idea to
> have a patch that calls MOZ_CRASH in GetInterface. Land that and then run it
> for a couple of weeks just to make sure.
> Just an idea. If you're convinced that GetInterface isn't called, I'll r+
> the current patch.

This is what I think.

Before anyone calls GetInterface() of class Foo, it would first obtain an instance or pointer/reference of Foo.
Currently, RtspControllerParent is only created in NeckoParent::AllocPRtspControllerParent() and is only responsible to pass IPC events between RtspControllerChild.
Although it implements the interface nsIStreamingProtocolListener, the use of this interface is also only in RTSP codes and we are sure nothing in our codes calls RtspControllerParent::GetInterface() through this interface.

Is this argument convincing enough?
If you still have concern, I could apply your suggestion to call MOZ_CRASH first.

Comment 6

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8419947 [details] [diff] [review]
Remove nsIInterfaceRequestor from RtspControllerParent

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

You convinced me! :) Thanks for taking the time to explain your thinking. r=me.

Comment 7

[Ethan Tseng [:ethan]]

(In reply to Steve Workman [:sworkman] from comment #6)
> Review of attachment 8419947 [details] [diff] [review]:
> You convinced me! :) Thanks for taking the time to explain your thinking.
> r=me.

YA! You are welcome. ^^
I'm glad you propose the question so that I could further sort our my thought.

Comment 8

[Ethan Tseng [:ethan]]

Attachment: Remove nsIInterfaceRequestor from RtspControllerParent. r=sworkman

Update commit message "r=sworkman".

Comment 9

[Ethan Tseng [:ethan]]

This patch is minor and B2G-specific. Would not affect test cases, so just run builds and B2G crash test.

Try server
Syntax: – try: -b o -p all -u crashtest-1,crashtest-2,crashtest-3 -t none
Result: https://tbpl.mozilla.org/?tree=Try&rev=26077966db82

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/bc2092f2f3be

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bc2092f2f3be