SendBeacon and XHR should really share the "extract" algorithm

RESOLVED FIXED in Firefox 54

Status

()

Core
DOM
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: bz, Assigned: baku)

Tracking

Trunk
mozilla54
Points:
---

Firefox Tracking Flags

(firefox53 affected, firefox54 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

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

Right now SendBeacon has its own (buggy for blobs with empty type, for example) version.
Flags: needinfo?(amarchesini)
(Assignee)

Updated

a year ago
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
(Assignee)

Comment 1

a year ago
Created attachment 8825811 [details] [diff] [review]
xhr.patch

I merged XHR, fetch and sendBeacon
Attachment #8825811 - Flags: review?(bugs)
(Assignee)

Comment 2

a year ago
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.
(Assignee)

Comment 3

a year ago
Created attachment 8828266 [details] [diff] [review]
xhr.patch

Some fixes to make WPT happy.
Attachment #8825811 - Attachment is obsolete: true
Attachment #8825811 - Flags: review?(bugs)
Attachment #8828266 - Flags: review?(bugs)
(Assignee)

Comment 4

a year ago
Created attachment 8828285 [details] [diff] [review]
part 1 - Unify body extraction in Fetch/Beacon/XHR
Attachment #8828266 - Attachment is obsolete: true
Attachment #8828266 - Flags: review?(bugs)
Attachment #8828285 - Flags: review?(bugs)
(Assignee)

Comment 5

a year ago
Created attachment 8828287 [details] [diff] [review]
part 2 - use nsIXHRSendable instead Blob/FormData/URLSearchParams
Attachment #8828287 - Flags: review?(bugs)
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.
Attachment #8828287 - Flags: review?(bugs) → review+
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
Attachment #8828285 - Flags: review?(bugs) → review-
(Assignee)

Comment 8

a year ago
Created attachment 8829779 [details] [diff] [review]
part 1 - Unify body extraction in Fetch/Beacon/XHR
Attachment #8828285 - Attachment is obsolete: true
(Assignee)

Comment 9

a year ago
Can you take a look? Thanks
Flags: needinfo?(bugs)
(Assignee)

Comment 10

a year ago
Created attachment 8829802 [details] [diff] [review]
part 3 - Use application/octet-stream for arrayBuffer in sendBeacon
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?
Attachment #8829779 - Flags: review+
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.
Attachment #8829802 - Flags: review+

Updated

a year ago
Flags: needinfo?(bugs)

Comment 13

a year ago
Pushed by amarchesini@mozilla.com:
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
Please file a bug to sort out application/octet-stream handling!
well, and to make our sendBeacon code to follow the spec better.

Comment 16

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2c914ce263f2
https://hg.mozilla.org/mozilla-central/rev/96061f5d5371
https://hg.mozilla.org/mozilla-central/rev/2e108eed36ea
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54

Comment 17

a year ago
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.
(Assignee)

Updated

11 months ago
Blocks: 1364925
You need to log in before you can comment on or make changes to this bug.