Closed Bug 1329298 Opened 3 years ago Closed 3 years ago

SendBeacon and XHR should really share the "extract" algorithm

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox53 --- affected
firefox54 --- fixed

People

(Reporter: bzbarsky, Assigned: baku)

References

Details

Attachments

(3 files, 3 obsolete files)

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: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attached patch xhr.patch (obsolete) — Splinter Review
I merged XHR, fetch and sendBeacon
Attachment #8825811 - Flags: review?(bugs)
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.
Attached patch xhr.patch (obsolete) — Splinter Review
Some fixes to make WPT happy.
Attachment #8825811 - Attachment is obsolete: true
Attachment #8825811 - Flags: review?(bugs)
Attachment #8828266 - Flags: review?(bugs)
Attachment #8828266 - Attachment is obsolete: true
Attachment #8828266 - Flags: review?(bugs)
Attachment #8828285 - 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-
Can you take a look? Thanks
Flags: needinfo?(bugs)
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+
Flags: needinfo?(bugs)
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.
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.
Blocks: 1364925
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.