[Presentation WebAPI] support send(Blob|ArrayBuffer|ArrayBufferView)

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: junior, Assigned: kershaw)

Tracking

(Depends on 2 bugs)

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [ETA 9/2])

Attachments

(1 attachment, 5 obsolete attachments)

Currently we have PresentationConnection.send(DOMString)
In order to match spec, we need to support sending different types of data
Whiteboard: [ETA 9/2]
Assignee: nobody → kechang
First version.

Please take a look. Thanks.
Attachment #8786188 - Flags: feedback?(schien)
Depends on: 1299040
Comment on attachment 8786188 [details] [diff] [review]
Part1: Implement sending blob/arraybuffer/arraybufferview - v1

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

::: dom/presentation/PresentationDataChannelSessionTransport.js
@@ +309,5 @@
> +      array[i] = aData.charCodeAt(i);
> +    }
> +
> +    let blob = new this._window.Blob([array.buffer]);
> +    this._dataChannel.send(blob);

You can send ArrayBuffer directly without converting it to a Blob.

@@ +320,5 @@
> +                       createInstance(Ci.nsIBinaryInputStream);
> +    binaryStream.setInputStream(aStream);
> +    let bytes = binaryStream.readByteArray(aStream.available());
> +    let blob = new this._window.Blob([new Uint8Array(bytes).buffer]);
> +    this._dataChannel.send(blob);

This call only works for blocking input stream and |readByteArray| will block main thread in this situation, which is dangerous.
I'm thinking if we can replace |sendBinaryStream| with |sendBlob| so that we can simply passing blob object without additional data conversion. |nsIDOMBlob| and |PBlob| should be the type for xpidl and ipdl.

::: dom/presentation/interfaces/nsIPresentationListener.idl
@@ +32,5 @@
>     * Called when receive messages.
>     */
>    void notifyMessage(in DOMString sessionId,
> +                     in ACString data,
> +                     in boolean isbinary);

nit: isBinary

::: dom/presentation/ipc/PresentationIPCService.cpp
@@ +107,5 @@
> +  if (mSessionInfos.Get(aSessionId, getter_AddRefs(info))) {
> +    return info->SendBinaryMsg(aData);
> +  }
> +
> +  return NS_ERROR_NOT_AVAILABLE;

This is based on an assumption that DataChannelTransport the only transport that supports binary message and is constructed on content side.
My LocalTransport patch will change this assumption but I can modify it in my patch if your patch lands first.
Attachment #8786188 - Flags: feedback?(schien)
Thanks for the feedback.

This patch is fixed based on your previous comments. Please take a look again.
Attachment #8786188 - Attachment is obsolete: true
Attachment #8786669 - Flags: feedback?(schien)
Comment on attachment 8786669 [details] [diff] [review]
Implement sending blob/arraybuffer/arraybufferview - v2

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

::: dom/presentation/PresentationTCPSessionTransport.cpp
@@ +419,5 @@
>  
>  NS_IMETHODIMP
> +PresentationTCPSessionTransport::SendBinaryMsg(const nsACString& aData)
> +{
> +  return NS_ERROR_NOT_IMPLEMENTED;

NS_ERROR_DOM_NOT_SUPPORTED_ERR would be better.

I'm also thinking if we should expose this exception to WebAPI. Right now, we'll throws OperationError if SendBinaryMsg return error cause.
Attachment #8786669 - Flags: feedback?(schien) → feedback+
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #4)
> Comment on attachment 8786669 [details] [diff] [review]
> Implement sending blob/arraybuffer/arraybufferview - v2
> 
> Review of attachment 8786669 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/presentation/PresentationTCPSessionTransport.cpp
> @@ +419,5 @@
> >  
> >  NS_IMETHODIMP
> > +PresentationTCPSessionTransport::SendBinaryMsg(const nsACString& aData)
> > +{
> > +  return NS_ERROR_NOT_IMPLEMENTED;
> 
> NS_ERROR_DOM_NOT_SUPPORTED_ERR would be better.
> 
> I'm also thinking if we should expose this exception to WebAPI. Right now,
> we'll throws OperationError if SendBinaryMsg return error cause.

Thanks for the review.

I think NS_ERROR_DOM_NOT_SUPPORTED_ERR would be more clear.
Summary:

Implement send blob/arraybuffer/arraybufferview when using the data channel
Attachment #8786669 - Attachment is obsolete: true
Attachment #8787146 - Flags: review?(bugs)
Comment on attachment 8787146 [details] [diff] [review]
Implement sending blob/arraybuffer/arraybufferview - v3

>+PresentationConnection::Send(Blob& aData,
>+                             ErrorResult& aRv)
>+{
>+  if (NS_WARN_IF(mState != PresentationConnectionState::Connected)) {
>+    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
>+    return;
>+  }
>+
>+  nsCOMPtr<nsIPresentationService> service =
>+    do_GetService(PRESENTATION_SERVICE_CONTRACTID);
>+  if(NS_WARN_IF(!service)) {
>     aRv.Throw(NS_ERROR_DOM_OPERATION_ERR);
>+    return;
>+  }
>+
>+  nsresult rv = service->SendSessionBlob(mId, mRole, &aData);
>+  if(NS_WARN_IF(NS_FAILED(rv))) {
>+    aRv.Throw(rv == NS_ERROR_DOM_NOT_SUPPORTED_ERR ?
>+                    NS_ERROR_DOM_NOT_SUPPORTED_ERR :
>+                    NS_ERROR_DOM_OPERATION_ERR);
>+  }
>+}
The spec doesn't mention that NS_ERROR_DOM_NOT_SUPPORTED_ERR or NS_ERROR_DOM_OPERATION_ERR should be thrown.
But there is "If the previous step encounters an unrecoverable error, then abruptly close the presentation connection presentationConnection with error as closeReason, and a closeMessage describing the error encountered."
Shouldn't we do that?


>+PresentationConnection::Send(const ArrayBuffer& aData,
>+                             ErrorResult& aRv)
>+{
>+  if (NS_WARN_IF(mState != PresentationConnectionState::Connected)) {
>+    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
>+    return;
>+  }
>+
>+  nsCOMPtr<nsIPresentationService> service =
>+    do_GetService(PRESENTATION_SERVICE_CONTRACTID);
>+  if(NS_WARN_IF(!service)) {
>+    aRv.Throw(NS_ERROR_DOM_OPERATION_ERR);
>+    return;
>+  }
>+
>+  aData.ComputeLengthAndData();
>+
>+  static_assert(sizeof(*aData.Data()) == 1, "byte-sized data required");
>+
>+  uint32_t length = aData.Length();
>+  char* data = reinterpret_cast<char*>(aData.Data());
>+  nsDependentCSubstring msgString(data, length);
>+
>+  nsresult rv = service->SendSessionBinaryMsg(mId, mRole, msgString);
>+  if(NS_WARN_IF(NS_FAILED(rv))) {
>+    aRv.Throw(rv == NS_ERROR_DOM_NOT_SUPPORTED_ERR ?
>+                    NS_ERROR_DOM_NOT_SUPPORTED_ERR :
>+                    NS_ERROR_DOM_OPERATION_ERR);
>+  }
>+}
same here


>+PresentationConnection::Send(const ArrayBufferView& aData,
>+                             ErrorResult& aRv)
>+{
>+  if (NS_WARN_IF(mState != PresentationConnectionState::Connected)) {
>+    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
>+    return;
>+  }
>+
>+  nsCOMPtr<nsIPresentationService> service =
>+    do_GetService(PRESENTATION_SERVICE_CONTRACTID);
>+  if(NS_WARN_IF(!service)) {
>+    aRv.Throw(NS_ERROR_DOM_OPERATION_ERR);
>+    return;
>+  }
>+
>+  aData.ComputeLengthAndData();
>+
>+  static_assert(sizeof(*aData.Data()) == 1, "byte-sized data required");
>+
>+  uint32_t length = aData.Length();
>+  char* data = reinterpret_cast<char*>(aData.Data());
>+  nsDependentCSubstring msgString(data, length);
>+
>+  nsresult rv = service->SendSessionBinaryMsg(mId, mRole, msgString);
>+  if(NS_WARN_IF(NS_FAILED(rv))) {
>+    aRv.Throw(rv == NS_ERROR_DOM_NOT_SUPPORTED_ERR ?
>+                    NS_ERROR_DOM_NOT_SUPPORTED_ERR :
>+                    NS_ERROR_DOM_OPERATION_ERR);
>   }
and here.
Attachment #8787146 - Flags: review?(bugs) → review-
Summary:
Close the connection if encountering an error when sending a message.
Attachment #8787146 - Attachment is obsolete: true
Attachment #8788119 - Flags: review?(bugs)
Comment on attachment 8788119 [details] [diff] [review]
Implement sending blob/arraybuffer/arraybufferview - v4

>   nsresult rv = service->SendSessionMessage(mId, mRole, aData);
>   if(NS_WARN_IF(NS_FAILED(rv))) {
>+    const uint32_t kMaxMessageLength = 256;
>+    nsAutoString data(Substring(aData, 0, kMaxMessageLength));
>+
>+    CloseConnectionWithErrorMsg(
>+      NS_LITERAL_STRING("Unable to send message: \"") + data +
>+      NS_LITERAL_STRING("\" for DOMString messages, where only the first ") +
>+      NS_LITERAL_STRING("256 characters of the failed message is shown."));
I wouldn't add the explanation that only 256 is shown.
Perhaps just
NS_LITERAL_STRING("Unable to send message: \"") + data + NS_LITERAL_STRING("\"");


>+    aRv.Throw(rv == NS_ERROR_DOM_NOT_SUPPORTED_ERR ?
>+                    NS_ERROR_DOM_NOT_SUPPORTED_ERR :
>+                    NS_ERROR_DOM_OPERATION_ERR);
The spec doesn't say that these exceptions should be thrown

>+  nsresult rv = service->SendSessionBlob(mId, mRole, &aData);
>+  if(NS_WARN_IF(NS_FAILED(rv))) {
>+    CloseConnectionWithErrorMsg(
>+      NS_LITERAL_STRING("Unable to send binary message for Blob message."));
>+    aRv.Throw(rv == NS_ERROR_DOM_NOT_SUPPORTED_ERR ?
>+                    NS_ERROR_DOM_NOT_SUPPORTED_ERR :
>+                    NS_ERROR_DOM_OPERATION_ERR);
>+  }
Same here. The spec just says that connection should be closed.


>+  nsresult rv = service->SendSessionBinaryMsg(mId, mRole, msgString);
>+  if(NS_WARN_IF(NS_FAILED(rv))) {
>+    CloseConnectionWithErrorMsg(
>+      NS_LITERAL_STRING("Unable to send binary message for ArrayBuffer message."));
>+    aRv.Throw(rv == NS_ERROR_DOM_NOT_SUPPORTED_ERR ?
>+                    NS_ERROR_DOM_NOT_SUPPORTED_ERR :
>+                    NS_ERROR_DOM_OPERATION_ERR);
and here.



>+  nsresult rv = service->SendSessionBinaryMsg(mId, mRole, msgString);
>+  if(NS_WARN_IF(NS_FAILED(rv))) {
>+    CloseConnectionWithErrorMsg(
>+      NS_LITERAL_STRING("Unable to send binary message for ArrayBufferView message."));
>+    aRv.Throw(rv == NS_ERROR_DOM_NOT_SUPPORTED_ERR ?
>+                    NS_ERROR_DOM_NOT_SUPPORTED_ERR :
>+                    NS_ERROR_DOM_OPERATION_ERR);
and here.



>+PresentationConnection::CloseConnectionWithErrorMsg(const nsAString& aMessage)
>+{
>+  nsCOMPtr<nsIPresentationService> service =
>+    do_GetService(PRESENTATION_SERVICE_CONTRACTID);
>+  if(NS_WARN_IF(!service)) {
>+    return;
>+  }
>+
>+  // Set |mState| to |PresentationConnectionState::Closed| here to avoid
>+  // calling |ProcessStateChanged|.
>+  mState = PresentationConnectionState::Closed;
>+
>+  NS_WARN_IF(NS_FAILED(
>+    service->CloseSession(mId,
>+                          mRole,
>+                          nsIPresentationService::CLOSED_REASON_ERROR)));

So per spec closing should happen async.
"Queue a task to run the following steps: " .. If the presentation connection state of presentationConnection is not closed, set it to closed. 

>+
>+  NS_WARN_IF(NS_FAILED(
>+    DispatchConnectionClosedEvent(PresentationConnectionClosedReason::Error,
>+                                  aMessage)));
>+}
Looks like DispatchConnectionClosedEvent is already doing it async, but dispatching the event and closing the connection should happen within the same task
Attachment #8788119 - Flags: review?(bugs) → review-
Thanks for the detailed review.

I should have read the spec more carefully.
Attachment #8788119 - Attachment is obsolete: true
Attachment #8788318 - Flags: review?(bugs)
Comment on attachment 8788318 [details] [diff] [review]
Implement sending blob/arraybuffer/arraybufferview - v5

>+PresentationConnection::AsyncCloseConnectionWithErrorMsg(const nsAString& aMessage)
>+{
>+  if (mState == PresentationConnectionState::Terminated) {
>+    return;
>+  }
>+
>+  nsString message = nsString(aMessage);
>+  nsCOMPtr<nsIRunnable> r =
>+    NS_NewRunnableFunction([this, message]() -> void {
Ok, so 'this' is addrefed/release automatically when lambda is create use here, right?
I think yes, given Bug 1153295 and other relevant bugs, but could you verify.


>+      // Set |mState| to |PresentationConnectionState::Closed| here to avoid
>+      // calling |ProcessStateChanged|.
>+      mState = PresentationConnectionState::Closed;
>+
>+      // Make sure dispatching the event and closing the connection are invoked
>+      // at the same time by setting |aDispatchNow| to true.
>+      NS_WARN_IF(NS_FAILED(
>+        DispatchConnectionClosedEvent(PresentationConnectionClosedReason::Error,
>+                                      message,
>+                                      true)));
>+
>+      nsCOMPtr<nsIPresentationService> service =
>+        do_GetService(PRESENTATION_SERVICE_CONTRACTID);
>+      if(NS_WARN_IF(!service)) {
>+        return;
>+      }
>+
>+      NS_WARN_IF(NS_FAILED(
>+        service->CloseSession(mId,
>+                              mRole,
>+                              nsIPresentationService::CLOSED_REASON_ERROR)));
Per spec dispatching the event should happen right after closing the session. So, move DispatchConnectionClosedEvent somewhere here.
This fixed, and the lambda usage verified, r+
Attachment #8788318 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #11)
> Comment on attachment 8788318 [details] [diff] [review]
> Implement sending blob/arraybuffer/arraybufferview - v5
> 
> >+PresentationConnection::AsyncCloseConnectionWithErrorMsg(const nsAString& aMessage)
> >+{
> >+  if (mState == PresentationConnectionState::Terminated) {
> >+    return;
> >+  }
> >+
> >+  nsString message = nsString(aMessage);
> >+  nsCOMPtr<nsIRunnable> r =
> >+    NS_NewRunnableFunction([this, message]() -> void {
> Ok, so 'this' is addrefed/release automatically when lambda is create use
> here, right?
> I think yes, given Bug 1153295 and other relevant bugs, but could you verify.

I also thought that addrefed/release should be invoked automatically. But, I found that I was wrong by doing some experiments.
|this| is actually captured by value. The only way to call addrefed/release is by declaring a new pointer like: RefPtr<PresentationConnection> self = this.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9cb90011a896
Support sending blob/arraybuffer for data channel. r=smaug
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9cb90011a896
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.