Closed Bug 1007577 Opened 6 years ago Closed 6 years ago

[RTSP] Remove nsIInterfaceRequestor from RtspControllerParent

Categories

(Firefox OS Graveyard :: RTSP, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.0 S2 (23may)

People

(Reporter: ethan, Assigned: ethan)

References

Details

(Whiteboard: [p=1])

Attachments

(1 file, 2 obsolete files)

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.
Assignee: nobody → ettseng
Whiteboard: [p=1]
Target Milestone: --- → 2.0 S2 (23may)
Status: NEW → ASSIGNED
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.
Attached patch bug-1007577-fix.patch (obsolete) — Splinter Review
Steve, can you help to review this minor fix?
Attachment #8419297 - Flags: review?(sworkman)
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.
Rebase because of bug 1006530.
Attachment #8419297 - Attachment is obsolete: true
Attachment #8419297 - Flags: review?(sworkman)
(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.
Flags: needinfo?(sworkman)
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.
Attachment #8419947 - Flags: review+
Flags: needinfo?(sworkman)
(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.
Update commit message "r=sworkman".
Attachment #8419947 - Attachment is obsolete: true
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
Keywords: checkin-needed
Attachment #8420504 - Attachment description: bug-1007577-fix.patch → Remove nsIInterfaceRequestor from RtspControllerParent. r=sworkman
https://hg.mozilla.org/mozilla-central/rev/bc2092f2f3be
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Blocks: b2g-RTSP-2.0
You need to log in before you can comment on or make changes to this bug.