1109751 - [Fetch] Implement Body.formData() method

Status: RESOLVED FIXED

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

Description

[Arkadiusz Michalski (Spirit)]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0
Build ID: 20141125180439

Steps to reproduce:

https://fetch.spec.whatwg.org/#dom-body-formdata

<script>

	var request1 = new Request("");

	alert(request1.formData) // undefined
	
</script>

Only this method is missing from Body interface.
https://fetch.spec.whatwg.org/#body

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Thanks for filing. This is mainly blocked on having FormData on workers, and since forms are not the highest priority for shipping the first version (since the other extraction operators can be used with custom parsing to handle forms), this is not on the radar of any paid staff working on this. I'd be happy to review a patch!

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Reviewboard does not deal well with multiple bugs in a set of commits yet. The patch is at https://reviewboard.mozilla.org/r/3275

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: FormData in Fetch API

Folded:
Bug 1109751 - FormData parsing for Body.formData()

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Request and Response constructors should set mime type

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8563726 [details] [diff] [review]
FormData in Fetch API

reverting bad bzexport.

Comment 6

[Andrea Marchesini [:baku]]

Comment on attachment 8563730 [details] [diff] [review]
Request and Response constructors should set mime type

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

::: dom/fetch/Request.cpp
@@ +34,5 @@
>    , mOwner(aOwner)
>    , mRequest(aRequest)
>  {
> +  ErrorResult result;
> +  SetMimeType(result);

same here.

::: dom/fetch/Response.cpp
@@ +38,5 @@
>    , mOwner(aGlobal)
>    , mInternalResponse(aInternalResponse)
>  {
> +  ErrorResult result;
> +  SetMimeType(result);

Write some warning msg if the result.Failed().
The fact is that, if this operation fails somehow, we don't have any information.
Can we move this in the constructor and return error if this method fails? Or in FetchBody template?

Comment 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Request and Response constructors should set mime type

On further investigation, a Get() for "content-type" cannot fail, so I've made SetMimeType() not accept an error.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Nikhil, note bug 1143857 comment 0.  Apparently this is causing problems on github in nightly.  Does this change the priority of things?

Comment 10

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Andrea said he should be able to review my patches to add FormData support before 39 branches to Aurora. I can split up this bug so that we support sending FormData, which is easy, but not formData(). I'll put up patches this week. That way we can get this compat issue fixed, and wait for a review on the more complicated form parsing in formData() (afaik that is holding up the review) a little later.

Comment 11

[Boris Zbarsky [:bzbarsky]]

We should probably just use bug 1143857 for the "sending formdata" bit....

Comment 12

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0563a2b3ac76

Comment 13

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Consume FormData support in Fetch API

Comment 14

[Andrea Marchesini [:baku]]

Comment on attachment 8579031 [details] [diff] [review]
Consume FormData support in Fetch API

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

lgtm. Sorry for the huge delay.

::: dom/fetch/Fetch.cpp
@@ +514,5 @@
>  }
> +
> +void
> +FillFormData(const nsString& aName, const nsString& aValue, void* aFormData)
> +{

MOZ_ASSERT(aFormData);

@@ +538,5 @@
> + * FIXME(nsm): Bug 1127552 - We should add telemetry to calls to formData() and
> + * friends to figure out if Fetch ends up copying big blobs to see if this is
> + * worth optimizing.
> + */
> +class MOZ_STACK_CLASS FormDataParser {

new line before {

@@ +562,5 @@
> +  // Reads over a boundary and sets start to the position after the end of the
> +  // boundary. Returns false if no boundary is found immediately.
> +  bool
> +  PushOverBoundary(const nsACString& boundaryString,
> +                   nsACString::const_iterator& start,

aStart and aBoundaryString

@@ +591,5 @@
> +  }
> +
> +  // Reads over a CRLF and positions start after it.
> +  bool
> +  PushOverLine(nsACString::const_iterator& start)

aStart

@@ +602,5 @@
> +    return false;
> +  }
> +
> +  bool
> +  FindCRLF(nsACString::const_iterator& start,

aStart

@@ +610,5 @@
> +    return FindInReadable(NS_LITERAL_CSTRING("\r\n"), start, end);
> +  }
> +
> +  bool
> +  ParseHeader(nsACString::const_iterator& start,

aStart

@@ +696,5 @@
> +  // part of the body. This will position the iterator at the beginning of the
> +  // boundary (after the CRLF).
> +  bool
> +  ParseBody(const nsACString& boundaryString,
> +            nsACString::const_iterator& start,

aBoundaryString and aStart

@@ +1518,5 @@
> +      // Take over ownership of the string.
> +      nsAutoCString data(reinterpret_cast<char*>(aResult), aResultLength);
> +      autoFree.Reset();
> +
> +      if (StringBeginsWith(mMimeType, NS_LITERAL_CSTRING("multipart/form-data"))) {

Why StringBeginsWith ? What about multipart/form-datafoobar ?

Comment 15

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7480aee7241

Comment 16

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Andrea Marchesini (:baku) from comment #14)
> 
> @@ +1518,5 @@
> > +      // Take over ownership of the string.
> > +      nsAutoCString data(reinterpret_cast<char*>(aResult), aResultLength);
> > +      autoFree.Reset();
> > +
> > +      if (StringBeginsWith(mMimeType, NS_LITERAL_CSTRING("multipart/form-data"))) {
> 
> Why StringBeginsWith ? What about multipart/form-datafoobar ?

Added fix and test for this in an update to the mime type patch. Fixed other issues in original patch.

Comment 17

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ad62df90381

Comment 18

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/add6d1bd2e52
https://hg.mozilla.org/integration/mozilla-inbound/rev/269ce19afa43

Comment 19

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 20

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

All three backed out for ASAN mochitest-3 permafails:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b850625399af
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/90c5d1cc12a8


https://treeherder.mozilla.org/logviewer.html#?job_id=8469263&repo=mozilla-inbound

Comment 21

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9a0bfd91e6a

Comment 22

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

seems like asan is happy after i explicitly adopted the string in the formdata case.

Comment 23

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2daaf1eadf6
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bbcde14b9bb

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e2daaf1eadf6
https://hg.mozilla.org/mozilla-central/rev/1bbcde14b9bb

Comment 25

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8579031 [details] [diff] [review]
Consume FormData support in Fetch API

Approval Request Comment
[Feature/regressing bug #]: dom-fetch-api
[User impact if declined]: We ship FormData support for sending data in Aurora. It would be good to uplift this so that extracting FormData is also supported
[Describe test coverage new/current, TreeHerder]: The patch adds tests in dom/tests/mochitest/fetch
[Risks and why]: Minimal. There aren't any compatibility issues since this is a new spec.
[String/UUID change made/needed]: None

Comment 26

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8565499 [details] [diff] [review]
Request and Response constructors should set mime type

Approval Request Comment
[Feature/regressing bug #]: please see comment 25. The same applies for this patch.
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

Nikhil, for the messages in your patches, where would they be exposed?  I'm not sure if they need localization or not. 

Axel, is this something that would trigger your localization tools/process?

Comment 28

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

The BAD_FORMDATA message is on the '.message' property of a javascript error.

Comment 29

[Axel Hecht [:Pike]]

It seems that that message isn't localized. No idea if that's a bug, though.

Comment 30

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8565499 [details] [diff] [review]
Request and Response constructors should set mime type

Approving for uplift to aurora since it sounds like we need to support this feature and this includes some test coverage.

Comment 31

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8579031 [details] [diff] [review]
Consume FormData support in Fetch API

Approving for uplift for aurora.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c3453f50cb49
https://hg.mozilla.org/releases/mozilla-aurora/rev/58b5c2f98c1c

Comment 33

[[e-mail]]

(In reply to Nikhil Marathe from comment #23)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/1bbcde14b9bb

Although I see no reference to it in this bug or any attachments, Mercurial blames the above changeset for the pattern NS_NAMED_LITERAL_CSTRING(..., NS_LITERAL_CSTRING("...")); which appears twice.

The preferred syntax is simply NS_NAMED_LITERAL_CSTRING(..., "...");

I think this only compiles because I forgot to delete the copy constructor in bug 514173.

I will therefore file a bug to do that and also fix this usage at the same time.