884702 - Make Rtsp non-exposed protocol and support rtsp in HTML <a> tag

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Vincent Chang[:vchang][changyihsin]]

Bug 831645 supports Rtsp protocol in video tag,  

<video id=home_video controls preload=none width=300 height=300>
  <source src="rtsp://x.x.x.x/x.mp4" />
</video>

We should also allow user to enter rtsp://xxxx in url bar. We may support this by 
1. pass the url by system message to trigger the video app. 
2. embedded video frame in browser app.

Comment 1

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Vincent Chang[:vchang] from comment #0)
> Bug 831645 supports Rtsp protocol in video tag,  
> 
> <video id=home_video controls preload=none width=300 height=300>
>   <source src="rtsp://x.x.x.x/x.mp4" />
> </video>
> 
> We should also allow user to enter rtsp://xxxx in url bar. We may support
> this by 
> 1. pass the url by system message to trigger the video app. 

Vote this. Much reasonable than 2.

> 2. embedded video frame in browser app.

Comment 2

[Ken Chang[:kenkai]]

Ethan is fixing this bug.

Comment 3

[Ethan Tseng [:ethan]]

Attachment: Bug 884702: Delegate RTSP request using broadcast system message.

What I did in this patch:
1. Modified nsDocShell::DoURILoad(): after the RTSP channel is created, create RtspDelegateChild to send the delegation request and break the URI loading.
2. Modified NeckoParent/Child: to support the new RtspDelegateParent/Child.
3. Added IPC RtspDelegateParent/Child: to pass the delegation request containing "url" spec.
4. RtspDelegateParent: broadcast system messages "rtsp-open-video".

In my unit test, if we modified the manifest of video app to register the "rtsp-open-video" message, the video app could be launched once we input an RTSP link in the browser URL bar. However, for now the video app was only launched but could not play the video successfully. More follow-ups are required.

Comment 4

[Ethan Tseng [:ethan]]

Attachment: bug-884402-fix-v2.patch

Remove a careless debugging macro in the previous patch.

Comment 5

[Ethan Tseng [:ethan]]

Attachment: Bug 884702: Delegate RTSP request using broadcast system message.

Resolved build errors on Windows XP platforms.

Comment 6

[Ethan Tseng [:ethan]]

Comment on attachment 827325 [details] [diff] [review]
Bug 884702: Delegate RTSP request using broadcast system message.

Hi, Stephen:

Could you help to review this patch?

Ethan

Comment 7

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 827325 [details] [diff] [review]
Bug 884702: Delegate RTSP request using broadcast system message.

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

Some cleanup things. r- for now, but almost there.

Please ask someone familiar with nsDocShell to review those changes.

::: docshell/base/nsDocShell.cpp
@@ +9663,5 @@
> +    /* Hack for RTSP protocol.
> +     * If the URL scheme is "rtsp", stop URI loading and delegate the RTSP
> +     * request to video apps.
> +     * See bug 884702 - Support rtsp protocol in URL bar.
> +     */

Please add to this comment: "Note: This only works for multi-process builds."

If we decide to add this to Firefox desktop/mobile at some point, then the code will have to be changed.

@@ +9669,5 @@
> +      nsAutoCString contentType;
> +      channel->GetContentType(contentType);
> +      if (contentType.LowerCaseEqualsLiteral("rtsp")) {
> +        URIParams uriParams;
> +        RtspDelegateChild *rdc = new RtspDelegateChild();

Do you need to use an nsRefPtr here?

@@ +9671,5 @@
> +      if (contentType.LowerCaseEqualsLiteral("rtsp")) {
> +        URIParams uriParams;
> +        RtspDelegateChild *rdc = new RtspDelegateChild();
> +        if (!rdc) {
> +           return NS_ERROR_OUT_OF_MEMORY;

You shouldn't need this. "new" is infallible.

@@ +9679,5 @@
> +        return NS_OK;
> +      }
> +    }
> +#endif
> +

You should get someone more familiar with nsDocShell to review this change.

::: netwerk/ipc/RtspDelegateChild.cpp
@@ +25,5 @@
> +NS_IMETHODIMP_(nsrefcnt) RtspDelegateChild::Release()
> +{
> +  NS_PRECONDITION(0 != mRefCnt, "dup release");
> +  // Enable this to find non-threadsafe destructors:
> +  // NS_ASSERT_OWNINGTHREAD(RtspDelegateChild);

If this class is only used on the main thread (see comment re class definition in header file), then you should uncomment this assertion.

@@ +44,5 @@
> +}
> +
> +NS_INTERFACE_MAP_BEGIN(RtspDelegateChild)
> +  NS_INTERFACE_MAP_ENTRY(nsISupports)
> +NS_INTERFACE_MAP_END

NS_IMPL_ISUPPORTS0(RtspDelegateChild) ?

::: netwerk/ipc/RtspDelegateChild.h
@@ +15,5 @@
> +
> +namespace mozilla {
> +namespace net {
> +
> +class RtspDelegateChild : public PRtspDelegateChild

Please add a brief comment to explain what this class does.

@@ +19,5 @@
> +class RtspDelegateChild : public PRtspDelegateChild
> +                        , public nsISupports
> +{
> +public:
> +  NS_DECL_THREADSAFE_ISUPPORTS

Does this class need to be threadsafe? Seems like it's only used in DocShell::LoadURI and that's main thread only, right?

@@ +26,5 @@
> +  ~RtspDelegateChild();
> +
> +  void AddIPDLReference();
> +  void ReleaseIPDLReference();
> +

I don't see SendDelegate being overridden in this class - is it definitely ok to use the generated version from PRtspDelegate.ipdl?

@@ +32,5 @@
> +  bool mIPCOpen;
> +  // RTSP URL refer to a stream or an aggregate of streams.
> +  nsCOMPtr<nsIURI> mURI;
> +  // ASCII encoded URL spec
> +  nsCString mSpec;

Do you need mSpec as a separate variable here? I don't see it being used anywhere.

::: netwerk/ipc/RtspDelegateParent.cpp
@@ +43,5 @@
> + */
> +bool
> +RtspDelegateParent::RecvDelegate(const URIParams& aURI)
> +{
> +  nsAutoString msgType(NS_LITERAL_STRING("rtsp-open-video"));

You could make this a static in the class definition:

static nsString msgType;

and define it in the cpp:

/*static*/ nsString RtspDelegateParent::msgType(NS_LITERAL_STRING("rtsp-open-video"));

@@ +46,5 @@
> +{
> +  nsAutoString msgType(NS_LITERAL_STRING("rtsp-open-video"));
> +  nsCOMPtr<nsIURI> uri = DeserializeURI(aURI);
> +  nsAutoCString spec;
> +  uri->GetAsciiSpec(spec);

Please get the scheme here and add an assertion for scheme == rtsp.

@@ +61,5 @@
> +  // Set the "url" property of the data.
> +  JS::Rooted<JS::Value> val(cx);
> +  JSString *urlStr = JS_NewStringCopyN(cx, spec.get(), spec.Length());
> +  NS_ENSURE_TRUE(urlStr, false);
> +  val = STRING_TO_JSVAL(urlStr);

Please put the three lines for urlStr in their own block:

JS::Rooted<JS::Value> val(cx);
{
  JSString *urlStr = JS_NewStringCopyN(cx, spec.get(), spec.Length());
  NS_ENSURE_TRUE(urlStr, false);
  val = STRING_TO_JSVAL(urlStr);
}

It isolates the urlStr symbol from the other code in the function, and the indentation can help with reading the code. Please apply this to other parts of the function as well.

@@ +63,5 @@
> +  JSString *urlStr = JS_NewStringCopyN(cx, spec.get(), spec.Length());
> +  NS_ENSURE_TRUE(urlStr, false);
> +  val = STRING_TO_JSVAL(urlStr);
> +  if (!JS_SetProperty(cx, dataObj, "url", val)) {
> +    return false;

Please add a boolean for the return value for JS_SetProperty, and then use NS_ENSURE_TRUE(rv, rv). This may help debugging in the future because the failure will show up on the console log. Do the same for the other calls to JS_SetProperty.

@@ +90,5 @@
> +  nsCOMPtr<nsISystemMessagesInternal> systemMessenger =
> +    do_GetService("@mozilla.org/system-message-internal;1");
> +  if (!systemMessenger) {
> +    return false;
> +  }

NS_ENSURE_TRUE(systemMessenger, false);

::: netwerk/ipc/moz.build
@@ +47,5 @@
>      'NeckoChannelParams.ipdlh',
>      'PNecko.ipdl',
>      'PRemoteOpenFile.ipdl',
>      'PRtspController.ipdl',
> +    'PRtspDelegate.ipdl',

Can you try putting both of these RTSP ipdl files in an "if CONFIG['MOZ_RTSP']" block please. I think it should work, and I think they should be isolated like this.

Comment 8

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 827325 [details] [diff] [review]
Bug 884702: Delegate RTSP request using broadcast system message.

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

Cancel the review request since sworkman has done the review.

Comment 9

[Ethan Tseng [:ethan]]

Hi, Steve, thanks for your review! I appreciated. :)
Here are my replies and questions about this review.

(In reply to Steve Workman [:sworkman] from comment #7)
> Comment on attachment 827325 [details] [diff] [review]
> Bug 884702: Delegate RTSP request using broadcast system message.
> 
> Review of attachment 827325 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some cleanup things. r- for now, but almost there.
> 
> Please ask someone familiar with nsDocShell to review those changes.
> 
Okay. I will talk with Vincent to consult a reviewer for nsDocShell.

> ::: docshell/base/nsDocShell.cpp
> @@ +9663,5 @@
> > +    /* Hack for RTSP protocol.
> > +     * If the URL scheme is "rtsp", stop URI loading and delegate the RTSP
> > +     * request to video apps.
> > +     * See bug 884702 - Support rtsp protocol in URL bar.
> > +     */
> 
> Please add to this comment: "Note: This only works for multi-process builds."
> 
> If we decide to add this to Firefox desktop/mobile at some point, then the
> code will have to be changed.
> 
> @@ +9669,5 @@
> > +      nsAutoCString contentType;
> > +      channel->GetContentType(contentType);
> > +      if (contentType.LowerCaseEqualsLiteral("rtsp")) {
> > +        URIParams uriParams;
> > +        RtspDelegateChild *rdc = new RtspDelegateChild();
> 
> Do you need to use an nsRefPtr here?
>
Good advice. I will add the comment and use nsRefPtr.

> @@ +9671,5 @@
> > +      if (contentType.LowerCaseEqualsLiteral("rtsp")) {
> > +        URIParams uriParams;
> > +        RtspDelegateChild *rdc = new RtspDelegateChild();
> > +        if (!rdc) {
> > +           return NS_ERROR_OUT_OF_MEMORY;
> 
> You shouldn't need this. "new" is infallible.
>
How about using "NS_ENSURE_TRUE(rdc, NS_ERROR_OUT_OF_MEMORY)" instead.
I think we still protect the code from the out-of-memory situation.

> @@ +9679,5 @@
> > +        return NS_OK;
> > +      }
> > +    }
> > +#endif
> > +
> 
> You should get someone more familiar with nsDocShell to review this change.
> 
> ::: netwerk/ipc/RtspDelegateChild.cpp
> @@ +25,5 @@
> > +NS_IMETHODIMP_(nsrefcnt) RtspDelegateChild::Release()
> > +{
> > +  NS_PRECONDITION(0 != mRefCnt, "dup release");
> > +  // Enable this to find non-threadsafe destructors:
> > +  // NS_ASSERT_OWNINGTHREAD(RtspDelegateChild);
> 
> If this class is only used on the main thread (see comment re class
> definition in header file), then you should uncomment this assertion.
> 
> @@ +44,5 @@
> > +}
> > +
> > +NS_INTERFACE_MAP_BEGIN(RtspDelegateChild)
> > +  NS_INTERFACE_MAP_ENTRY(nsISupports)
> > +NS_INTERFACE_MAP_END
> 
> NS_IMPL_ISUPPORTS0(RtspDelegateChild) ?
> 
It seems unnecessary to define our own Release() method here. I will remove my Release() and use NS_IMPL_ISUPPORTS0() instead.

> ::: netwerk/ipc/RtspDelegateChild.h
> @@ +15,5 @@
> > +
> > +namespace mozilla {
> > +namespace net {
> > +
> > +class RtspDelegateChild : public PRtspDelegateChild
> 
> Please add a brief comment to explain what this class does.
> 
Thanks for your reminder. I will add it.

> @@ +19,5 @@
> > +class RtspDelegateChild : public PRtspDelegateChild
> > +                        , public nsISupports
> > +{
> > +public:
> > +  NS_DECL_THREADSAFE_ISUPPORTS
> 
> Does this class need to be threadsafe? Seems like it's only used in
> DocShell::LoadURI and that's main thread only, right?
> 
Yes. It only used in DocShell::LoadURI(). I will further confirm it's main thread only.
If it is, I will remove the macro.

> @@ +26,5 @@
> > +  ~RtspDelegateChild();
> > +
> > +  void AddIPDLReference();
> > +  void ReleaseIPDLReference();
> > +
> 
> I don't see SendDelegate being overridden in this class - is it definitely
> ok to use the generated version from PRtspDelegate.ipdl?
> 
The only thing SendDelegate() does is to pass the URI, which is defined as "URIParams aURI", to the parent side.
I think it is enough to use the generated version. Or is there anything I should concern?

> @@ +32,5 @@
> > +  bool mIPCOpen;
> > +  // RTSP URL refer to a stream or an aggregate of streams.
> > +  nsCOMPtr<nsIURI> mURI;
> > +  // ASCII encoded URL spec
> > +  nsCString mSpec;
> 
> Do you need mSpec as a separate variable here? I don't see it being used
> anywhere.
> 
Oops. My mistake. I will remove it.

> ::: netwerk/ipc/RtspDelegateParent.cpp
> @@ +43,5 @@
> > + */
> > +bool
> > +RtspDelegateParent::RecvDelegate(const URIParams& aURI)
> > +{
> > +  nsAutoString msgType(NS_LITERAL_STRING("rtsp-open-video"));
> 
> You could make this a static in the class definition:
> 
> static nsString msgType;
> 
> and define it in the cpp:
> 
> /*static*/ nsString
> RtspDelegateParent::msgType(NS_LITERAL_STRING("rtsp-open-video"));
> 
> @@ +46,5 @@
> > +{
> > +  nsAutoString msgType(NS_LITERAL_STRING("rtsp-open-video"));
> > +  nsCOMPtr<nsIURI> uri = DeserializeURI(aURI);
> > +  nsAutoCString spec;
> > +  uri->GetAsciiSpec(spec);
> 
> Please get the scheme here and add an assertion for scheme == rtsp.
> 
> @@ +61,5 @@
> > +  // Set the "url" property of the data.
> > +  JS::Rooted<JS::Value> val(cx);
> > +  JSString *urlStr = JS_NewStringCopyN(cx, spec.get(), spec.Length());
> > +  NS_ENSURE_TRUE(urlStr, false);
> > +  val = STRING_TO_JSVAL(urlStr);
> 
> Please put the three lines for urlStr in their own block:
> 
> JS::Rooted<JS::Value> val(cx);
> {
>   JSString *urlStr = JS_NewStringCopyN(cx, spec.get(), spec.Length());
>   NS_ENSURE_TRUE(urlStr, false);
>   val = STRING_TO_JSVAL(urlStr);
> }
> 
> It isolates the urlStr symbol from the other code in the function, and the
> indentation can help with reading the code. Please apply this to other parts
> of the function as well.
> 
> @@ +63,5 @@
> > +  JSString *urlStr = JS_NewStringCopyN(cx, spec.get(), spec.Length());
> > +  NS_ENSURE_TRUE(urlStr, false);
> > +  val = STRING_TO_JSVAL(urlStr);
> > +  if (!JS_SetProperty(cx, dataObj, "url", val)) {
> > +    return false;
> 
> Please add a boolean for the return value for JS_SetProperty, and then use
> NS_ENSURE_TRUE(rv, rv). This may help debugging in the future because the
> failure will show up on the console log. Do the same for the other calls to
> JS_SetProperty.
>
> @@ +90,5 @@
> > +  nsCOMPtr<nsISystemMessagesInternal> systemMessenger =
> > +    do_GetService("@mozilla.org/system-message-internal;1");
> > +  if (!systemMessenger) {
> > +    return false;
> > +  }
> 
> NS_ENSURE_TRUE(systemMessenger, false);
>
Thanks for your great advice for RtspDelegateParent.cpp. I will follow your suggestions. :)
 
> ::: netwerk/ipc/moz.build
> @@ +47,5 @@
> >      'NeckoChannelParams.ipdlh',
> >      'PNecko.ipdl',
> >      'PRemoteOpenFile.ipdl',
> >      'PRtspController.ipdl',
> > +    'PRtspDelegate.ipdl',
> 
> Can you try putting both of these RTSP ipdl files in an "if
> CONFIG['MOZ_RTSP']" block please. I think it should work, and I think they
> should be isolated like this.
Sure. I will try it, thanks!

Comment 10

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Ethan Tseng [:ethan] from comment #9)
> > @@ +26,5 @@
> > > +  ~RtspDelegateChild();
> > > +
> > > +  void AddIPDLReference();
> > > +  void ReleaseIPDLReference();
> > > +
> > 
> > I don't see SendDelegate being overridden in this class - is it definitely
> > ok to use the generated version from PRtspDelegate.ipdl?
> > 
> The only thing SendDelegate() does is to pass the URI, which is defined as
> "URIParams aURI", to the parent side.
> I think it is enough to use the generated version. Or is there anything I
> should concern?

I don't think there's anything you should be concerned about - I wanted to make sure that not overriding was your intention.

Can you add a comment in the class definition to clarify your choice please? This will help future readers.

Comment 11

[Ethan Tseng [:ethan]]

Attachment: Bug 884702: Delegate RTSP request using broadcast system message.

Hi Steve,
I fixed the codes according to your comments.
BTW, I am still new to Mozilla. Your advice is quite invaluable to me, thanks a lot! :)

Comment 13

[Ethan Tseng [:ethan]]

Attachment: Bug 884702: Delegate RTSP request using broadcast system message.

Resolved build error in the last patch.

Comment 14

[Ethan Tseng [:ethan]]

Attachment: Bug 884702: Delegate RTSP request using broadcast system message.

Minor change: set the "title" property the same as the url.

Comment 15

[Ethan Tseng [:ethan]]

Comment on attachment 830052 [details] [diff] [review]
Bug 884702: Delegate RTSP request using broadcast system message.

Hi Ms2ger,
We added something within nsDocShell::DoURILoad() to handle RTSP url scheme. Steve suggested we get someone familiar with nsDocShell to review these changes. Could you help to review this patch?

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 830052 [details] [diff] [review]
Bug 884702: Delegate RTSP request using broadcast system message.

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

I don't think I'm the person you're looking for :)

Comment 17

[Ethan Tseng [:ethan]]

(In reply to :Ms2ger from comment #16)
> Comment on attachment 830052 [details] [diff] [review]
> Bug 884702: Delegate RTSP request using broadcast system message.
> 
> Review of attachment 830052 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't think I'm the person you're looking for :)

Hi Ms2ger, thank you for helping transfer to the appropriate reviewer. :)

Comment 18

[Boris Zbarsky [:bzbarsky]]

Why do we want to do this in the docshell as opposed to the unknown protocol handler?

Comment 19

[Boris Zbarsky [:bzbarsky]]

In particular, normally the unknown protocol handler is what handles handing off URIs to helper apps and whatnot.

Comment 20

[Ethan Tseng [:ethan]]

Boris, sorry I am not so familiar with all of these. What is the unknown protocol handler? Is it related the external helper app service?

Comment 21

[Boris Zbarsky [:bzbarsky]]

Ethan, it's the code in uriloader/exthandler/nsExternalProtocolHandler.cpp plus the code in nsExternalHelperAppService::LoadURI.

Note that the latter also does some security checks and whatnot; presumably your code would go after those but before the attempt to look for an OS-level handler for the URI, right?  Though note the check for a content process up front in the method, which might complicate things.

Comment 22

[Ethan Tseng [:ethan]]

Boris, thanks for your information. Let me take some time to study them and see if we can hand off RTSP loading there.

BTW, the reason why we chose to do this in nsDocShell::DoURILoad() in the first place is because RTSP channel is not the real protocol handler. It is a dummy channel used to aid MediaResource creation in HTMLMediaElement. The actual network control and data flows are managed by RtspController object. So we thought we could break URI loading once the RTSP channel is created, just like the case of "mailto".

Comment 23

[Ethan Tseng [:ethan]]

I think the point is, we should not handle particular protocol in nsDocShell, right? I will try to do this in uriloader/exthandler.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Right, exactly.  I'm 99.9% sure that mailto is handled in the code I mentioned in comment 21, for example.

Comment 25

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 830052 [details] [diff] [review]
Bug 884702: Delegate RTSP request using broadcast system message.

Ethan, I'll review the patch once you have had time to figure out the unknown protocol handler.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 830052 [details] [diff] [review]
Bug 884702: Delegate RTSP request using broadcast system message.

Unsetting review request for now.

Comment 29

[Ethan Tseng [:ethan]]

Attachment: bug-884702-fix.patch

Hi Boris, I added the handling for RTSP scheme in the parent side of nsExternalHelperAppService::DoContent(). Could you please review it for me?

Comment 30

[Boris Zbarsky [:bzbarsky]]

Hmm.  So is rtsp not an unknown protocol, in the sense that we load the data ourselves?  Is it guaranteed to come with an unhandleable MIME type that we then hand off to DoContent?

Comment 31

[Boris Zbarsky [:bzbarsky]]

And in particular, with this patch what happens to the data we're receiving from necko?

Comment 32

[Ethan Tseng [:ethan]]

(In reply to Boris Zbarsky [:bz] from comment #30)
> Hmm.  So is rtsp not an unknown protocol, in the sense that we load the data
> ourselves?  Is it guaranteed to come with an unhandleable MIME type that we
> then hand off to DoContent?
I think the answer is yes, in Firefox OS. Rtsp protocol is specially handled and the MIME type is always a fake type "RTSP" specified by RTSP channel.

Comment 33

[Ethan Tseng [:ethan]]

(In reply to Boris Zbarsky [:bz] from comment #31)
> And in particular, with this patch what happens to the data we're receiving
> from necko?
Sorry I didn't aware of this. So far I just know that, for rtsp protocol, the actual control and data flows are managed by RtspController object and owned by RtspMediaResource. I will try to figure out this issue.

Comment 34

[Boris Zbarsky [:bzbarsky]]

OK.  I mean, this patch both kicks off the load to the new codepath _and_ does the normal exthandler stuff on whatever data comes in, which seems a bit odd.  I would like to understand why it's doing that.

Comment 35

[Ethan Tseng [:ethan]]

Attachment: bug-884702-fix.patch

Comment 36

[Ethan Tseng [:ethan]]

Attachment: bug-884702-fix.patch

Set rtsp as non-exposed protocol.
pref("network.protocol-handler.expose.rtsp", false);

Then handle rtsp protocol in nsExternalHelperAppService::LoadURI().
With this dispatch, we did not handle the case for <iframe>, such as:
<iframe src=rtsp://whatever>

Comment 37

[Ethan Tseng [:ethan]]

Comment on attachment 8334414 [details] [diff] [review]
bug-884702-fix.patch

Boris, could you please review my updated patch?

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8334414 [details] [diff] [review]
bug-884702-fix.patch

Thanks.  I like this code location a lot more.  ;)

>+NS_IMETHODIMP nsExternalHelperAppService::launchVideoAppForRtsp(nsIURI* aURI)

s/NS_IMETHODIMP/nsresult/, since this wasn't declared NS_IMETHOD.

But also, just make this return void, since the caller ignores the return value anyway.

>+  nsAutoString msgType(NS_LITERAL_STRING("rtsp-open-video"));

Please use NS_NAMED_LITERAL_STRING instead.

>+  NS_ENSURE_TRUE(isRTSP, false);

That return value is not an nsresult; I'n a little surprised this compiled.  But, again, the caller already ensures this.  So just skip this check, or turn it into an assert.

>+  NS_ENSURE_TRUE(msgObj, false);

Again, not an nsresult.

Worse, this can leave a pending exception on cx.  You need to JS_ClearPendingException in all your failure cases here.  Might be worth creating a stack helper for that or something.  Alternately, create a WebIDL dictionary to do the conversion for you so you don't have to hand-code it all...

>+    NS_ENSURE_TRUE(urlStr, false);

As above.

>+    jsVal = STRING_TO_JSVAL(urlStr);

  jsVal.setString(urlStr);

>+    NS_ENSURE_TRUE(rv, false);

As above.  This occurs in several places.

>+    JSString *typeStr = JS_NewStringCopyN(cx, "video/rtsp", strlen("video/rtsp"));

NS_NAMED_LITERAL_CSTRING would keep you from typing out the string twice, I'd think.

>+    NS_ENSURE_TRUE(typeStr, false);

The usual.

>+    jsVal = STRING_TO_JSVAL(typeStr);

setString().

>+  systemMessenger->BroadcastMessage(msgType, OBJECT_TO_JSVAL(msgObj),

This is an unsafe reference hazard.  You want to do something more like:

  jsVal.setObject(*msgObj);

and pass jsVal to BroadcastMessage.

> +  // Handle for rtsp protocol.

Drop the "for", please.

r=me with those issues fixed, but I want to see the updated patch before it gets checked in, please.

Comment 39

[Ethan Tseng [:ethan]]

(In reply to Boris Zbarsky [:bz] from comment #38)
> Comment on attachment 8334414 [details] [diff] [review]
> bug-884702-fix.patch
> r=me with those issues fixed, but I want to see the updated patch before it
> gets checked in, please.
Boris, thanks for your review!
Sure. I will update a patch by your suggestions. Thanks. :)

Comment 40

[Ethan Tseng [:ethan]]

Attachment: bug-884702-fix.patch

Fixed the issues from the reviewer's comments.

Comment 41

[Ethan Tseng [:ethan]]

Attachment: bug-884702-fix.patch

Fix naming.

Comment 42

[Ethan Tseng [:ethan]]

(In reply to Boris Zbarsky [:bz] from comment #38)
> Comment on attachment 8334414 [details] [diff] [review]
> bug-884702-fix.patch
> Worse, this can leave a pending exception on cx.  You need to
> JS_ClearPendingException in all your failure cases here.  Might be worth
> creating a stack helper for that or something.  Alternately, create a WebIDL
> dictionary to do the conversion for you so you don't have to hand-code it
> all...

Hi Boris, could you please take a look at my updated patch?
I am not quite sure whether the stack helper I implemented is what you mentioned.

Comment 43

[Boris Zbarsky [:bzbarsky]]

That looks good, but maybe call the helper AutoClearPendingException?

Comment 44

[Ethan Tseng [:ethan]]

Attachment: bug-884702-fix.patch

Refine a function name.

Comment 45

[Ethan Tseng [:ethan]]

(In reply to Boris Zbarsky [:bz] from comment #43)
> That looks good, but maybe call the helper AutoClearPendingException?
Yeah, I was thinking of a similar name like this.
Thanks for your advice! I've renamed it.

Comment 46

[Ethan Tseng [:ethan]]

Attachment: bug-884702-fix.patch

Refresh comment.

Comment 47

[Ethan Tseng [:ethan]]

The result of try server:
https://tbpl.mozilla.org/?tree=Try&rev=39556a02beb3

Comment 48

[Ryan VanderMeulen [:RyanVM]]

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

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4c959d937a76

Comment 50

[Ethan Tseng [:ethan]]

(In reply to Steve Workman [:sworkman] from comment #25)
> Comment on attachment 830052 [details] [diff] [review]
> Ethan, I'll review the patch once you have had time to figure out the
> unknown protocol handler.

Hi Steve, in the final solution with external helper, we don't need to establish IPC ourselves.
However, I really appreciated you helped to review my middle work and thanks for your advices. I learned a lot, thanks. :)

Comment 51

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Ethan Tseng [:ethan] from comment #50)
> I really appreciated you helped to review my middle work and thanks
> for your advices. I learned a lot, thanks. :)

You're very welcome. Glad it helped.