1329298 - SendBeacon and XHR should really share the "extract" algorithm

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

https://fetch.spec.whatwg.org/#concept-bodyinit-extract 

Right now SendBeacon has its own (buggy for blobs with empty type, for example) version.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: xhr.patch

I merged XHR, fetch and sendBeacon

Comment 2

[Andrea Marchesini [:baku]]

Comment on attachment 8825811 [details] [diff] [review]
xhr.patch

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

::: dom/base/Navigator.cpp
@@ +1225,5 @@
>  }
>  
>  bool
>  Navigator::SendBeacon(const nsAString& aUrl,
> +                      const Nullable<ArrayBufferOrArrayBufferViewOrBlobOrFormDataOrUSVStringOrURLSearchParams> aData,

I forgot a '&' in this line.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: xhr.patch

Some fixes to make WPT happy.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 1 - Unify body extraction in Fetch/Beacon/XHR

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 2 - use nsIXHRSendable instead Blob/FormData/URLSearchParams

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8828287 [details] [diff] [review]
part 2 - use nsIXHRSendable instead Blob/FormData/URLSearchParams

You should update the documentation about BodyExtractor that it doesn't deal with 
Blob, FormData nor URLSearchParams, but just nsIXHRSendable (and other stuff mentioned in the comment).
Perhaps mention there that Blob, FormData and URLSearchParams all implement nsIXHRSendable.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8828285 [details] [diff] [review]
part 1 - Unify body extraction in Fetch/Beacon/XHR

This is hard to review when there are all sorts of changes in a large patch.

So, sendBeacon starts to accept USVString, but XHR still uses DOMString
We need tests for the new types sendBeacon starts to accept

>+  nsCOMPtr<nsIInputStream> in;
>+  nsAutoCString mimeType;
>+  nsAutoCString charset;
>+  uint64_t length;
Please assign some default value to length


>+BodyExtractor<nsIDocument>::GetAsStream(nsIInputStream** aResult,
>+                                        uint64_t* aContentLength,
>+                                        nsACString& aContentType,
>+                                        nsACString& aCharset) const
Can you please rename aContentType to aContentTypeWithCharset since that is what its new meaning seems to have.
In this ::GetAsStream implementation and elsewhere.

>+class BodyExtractorBase
>+{
>+public:
>+  virtual nsresult GetAsStream(nsIInputStream** aResult,
>+                               uint64_t* aContentLength,
>+                               nsACString& aContentType,
>+                               nsACString& aCharset) const
>+  {
>+    NS_ASSERTION(false, "BodyExtractorBase should not be used directly.");
>+    return NS_ERROR_FAILURE;
>+  }
couldn't you make this method abstract. = 0;


> URLSearchParams::GetSendInfo(nsIInputStream** aBody, uint64_t* aContentLength,
>                              nsACString& aContentType, nsACString& aCharset)
> {
>-  aContentType.AssignLiteral("application/x-www-form-urlencoded");
>+  aContentType.AssignLiteral("application/x-www-form-urlencoded;charset=UTF-8");
Since you change the meaning of nsIXHRSendable.getSendInfo, please rename aContentType argument here and elsewhere, also in .idl

>-XMLHttpRequestMainThread::RequestBody<const nsAString>::GetAsStream(
>-   nsIInputStream** aResult, uint64_t* aContentLength,
>-   nsACString& aContentType, nsACString& aCharset) const
>-{
>-  aContentType.AssignLiteral("text/plain");
>-  aCharset.AssignLiteral("UTF-8");
>-
>-  nsAutoCString converted;
>-  if (!AppendUTF16toUTF8(*mBody, converted, fallible)) {
>-    return NS_ERROR_OUT_OF_MEMORY;
>-  }
Since XHR's webidl doesn't use USVString, but DOMString, doesn't the change from using
AppendUTF16toUTF8 to using encoder->Convert change at least what error is being thrown when invalid
UTF16 is passed as param? Worth to test and perhaps a separate patch to make XHR to use USVStrings too?
>     if (uploadStream) {
>       // If author set no Content-Type, use the default from GetAsStream().
>       mAuthorRequestHeaders.Get("content-type", uploadContentType);
>       if (uploadContentType.IsVoid()) {
>         uploadContentType = defaultContentType;
>-
>-        if (!charset.IsEmpty()) {
>-          // If we are providing the default content type, then we also need to
>-          // provide a charset declaration.
>-          uploadContentType.Append(NS_LITERAL_CSTRING(";charset="));
>-          uploadContentType.Append(charset);
>-        }
I think defaultContentType should be renamed to hint that it already contains charset

Comment 8

[Andrea Marchesini [:baku]]

Attachment: part 1 - Unify body extraction in Fetch/Beacon/XHR

Comment 9

[Andrea Marchesini [:baku]]

Can you take a look? Thanks

Comment 10

[Andrea Marchesini [:baku]]

Attachment: part 3 - Use application/octet-stream for arrayBuffer in sendBeacon

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8829779 [details] [diff] [review]
part 1 - Unify body extraction in Fetch/Beacon/XHR

Based on IRC, let's keep the special case in sendBeacon to use "application/octet-stream" for ArrayBuffer/ArrayBufferView to be compatible with blink, but then a separate patch to follow the spec?

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8829802 [details] [diff] [review]
part 3 - Use application/octet-stream for arrayBuffer in sendBeacon

ok, this way. We may want to back this out to follow the spec.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c914ce263f2
Part 1 - Unify body extraction in Fetch/Beacon/XHR, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/96061f5d5371
Part 2 - use nsIXHRSendable instead Blob/FormData/URLSearchParams, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e108eed36ea
Part 3 - Use application/octet-stream for arrayBuffer in sendBeacon, r=smaug

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Please file a bug to sort out application/octet-stream handling!

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

well, and to make our sendBeacon code to follow the spec better.

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2c914ce263f2
https://hg.mozilla.org/mozilla-central/rev/96061f5d5371
https://hg.mozilla.org/mozilla-central/rev/2e108eed36ea

Comment 17

[Anne (:annevk)]

Olli, I filed https://github.com/w3c/beacon/issues/40 and https://github.com/w3c/beacon/issues/41 as follow ups for the sendBeacon() folks. We should maybe also do our own investigation for some of this earlier. I'll leave that to you and Andrea.