Closed Bug 739173 Opened 12 years ago Closed 9 years ago

Support FormData in workers

Categories

(Core :: DOM: Workers, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: sicking, Assigned: nsm)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 1 obsolete file)

That way people can use XHR to communicate with servers which expects multipart/form-data encoded requests. This is actually especially important on workers since the option of using hacks based on <form> and <iframe> isn't available in workers.

This might be somewhat tricky since we can't simply send the FormData object over to the main thread and pass it to XHR there, since FormData objects aren't immutable. So we have to encode on the worker thread and send the encoded result to the main thread, possibly in the form of a nsIInputStream.

We might want to wait until the new DOM-Bindings work is done (don't know bug# off the top of my head) since XHR is changing quite a bit there, and since that way we won't have to write a lot of new JS-API code to expose FormData objects in workers.
The new DOM-Bindings work has landed which should make it easier to fix this bug.
(Only FormData doesn't use the new bindings yet.)
It appears FormData.webidl exists, so maybe this is easy now?
Probably not "easy", but should be easier. One thing that we'd need to check is that nsFSMultipartFormData, and anything it calls, is threadsafe enough to call off the main thread.
Keywords: dev-doc-needed
Assignee: nobody → nsm.nikhil
Comment on attachment 8563722 [details] [diff] [review]
Support FormData in workers

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

r- for the File CTOR. Then, ask bent a quick review for the XML in workers.

::: dom/base/nsFormData.h
@@ +121,5 @@
>      SetNameFilePair(data, aName, aBlob, aFilename);
>      return NS_OK;
>    }
> +
> +  typedef void (*FormDataEntryCallback)(const nsString& aName, bool isFile,

return a boolean here and if the return value is false, you stop the for loop.

@@ +136,5 @@
> +  ForEach(FormDataEntryCallback aFunc, void* aClosure)
> +  {
> +    for (uint32_t i = 0; i < mFormData.Length(); ++i) {
> +      FormDataTuple& tuple = mFormData[i];
> +      aFunc(tuple.name, tuple.valueIsFile, tuple.stringValue, tuple.fileValue,

if (!aFunc( ... {
  break;
}

::: dom/webidl/File.webidl
@@ +10,5 @@
>               USVString fileName, optional FilePropertyBag options),
>  
>   // These constructors are just for chrome callers:
>   Constructor(Blob fileBits, optional ChromeFilePropertyBag options),
> + // Constructor(nsIFile fileBits, optional ChromeFilePropertyBag options),

why this I remember there is a test and some code using this.
Then... if you want to remove this, you should remove also the code in File.*

::: dom/webidl/XMLHttpRequest.webidl
@@ +147,5 @@
>  
>    readonly attribute boolean mozAnon;
>    readonly attribute boolean mozSystem;
>  };
> +

no extra lines

::: dom/workers/WorkerPrivate.cpp
@@ +444,5 @@
> +// See WriteFormData for serialization format.
> +void
> +ReadFormData(JSContext* aCx,
> +             JSStructuredCloneReader* aReader,
> +             bool aIsMainThread,

remove this

@@ +453,5 @@
> +  MOZ_ASSERT(aReader);
> +  MOZ_ASSERT(!aFormData);
> +
> +  nsCOMPtr<nsISupports> parent;
> +  if (aIsMainThread) {

you don't actually need this boolean, right?
You can do:

if (IsOnMainThread()) {
 ...
} else {
  ...
}

@@ +460,5 @@
> +    nsCOMPtr<nsIScriptGlobalObject> scriptGlobal =
> +      nsJSUtils::GetStaticScriptGlobal(JS::CurrentGlobalOrNull(aCx));
> +    parent = do_QueryInterface(scriptGlobal);
> +  } else {
> +    MOZ_ASSERT(!NS_IsMainThread());

remove this and user workerPrivate->AssertIsOnWorkerThread()

@@ +486,5 @@
> +
> +    if (isFile) {
> +      // Read out the tag since the blob reader isn't expecting it.
> +      MOZ_ALWAYS_TRUE(JS_ReadUint32Pair(aReader, &dummy, &dummy));
> +      nsRefPtr<File> blob = ReadBlobOrFileNoWrap(aCx, aReader, aIsMainThread);

MOZ_ASSERT(blob);

@@ +548,5 @@
> +  if (NS_WARN_IF(!JS_WriteUint32Pair(aWriter, DOMWORKER_SCTAG_FORMDATA, aFormData->Length()))) {
> +    return false;
> +  }
> +
> +  class Closure {

MOZ_STACK_CLASS

@@ +563,5 @@
> +
> +    static void
> +    Write(const nsString& aName, bool isFile, const nsString& aValue,
> +          nsIDOMBlob* aFile, void* aClosure)
> +    {

In cae you don't want to change the boolean return value, do a check here like this:

if (closure->mFailed) {
  return;
}

::: dom/workers/XMLHttpRequest.cpp
@@ +2229,5 @@
> +    mWorkerPrivate->IsChromeWorker() ?
> +    ChromeWorkerStructuredCloneCallbacks(false) :
> +    WorkerStructuredCloneCallbacks(false);
> +
> +  nsTArray<nsCOMPtr<nsISupports> > clonedObjects;

no extra space between > >
Attachment #8563722 - Flags: review?(amarchesini) → review-
Andrea, I had commented the ctor due to Bug 1127206 which is now fixed.
I have not changed the main thread bool since we need to pass it to the blob/file reader. If you still want it changed, let me know.

Ben, please review XHR Send() and the corresponding test (testSend())
Attachment #8567532 - Flags: review?(bent.mozilla)
Attachment #8567532 - Flags: review?(amarchesini)
Comment on attachment 8567532 [details] [diff] [review]
Support FormData in workers

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

::: dom/base/nsFormData.h
@@ +122,5 @@
>      SetNameFilePair(data, aName, aBlob, aFilename);
>      return NS_OK;
>    }
> +
> +  typedef bool (*FormDataEntryCallback)(const nsString& aName, bool isFile,

aIsFile

::: dom/html/test/formData_test.js
@@ +99,5 @@
> +function testSend(doneCb) {
> +    var xhr = new XMLHttpRequest();
> +    xhr.open("POST", "form_submit_server.sjs");
> +    xhr.onload = function () {
> +        var response = xhr.response;

indent 2 spaces

::: dom/workers/WorkerPrivate.cpp
@@ +483,5 @@
> +    nsAutoString name;
> +    MOZ_ALWAYS_TRUE(ReadString(aReader, name));
> +
> +    if (isFile) {
> +      // Read out the tag since the blob reader isn't expecting it.

Maybe you can change WriteBlobOrFile like this:

bool
WriteBlobOrFile(JSContext* aCx,
                JSStructuredCloneWriter* aWriter,
                FileImpl* aBlobOrFileImpl,
                nsTArray<nsCOMPtr<nsISupports>>& aClonedObjects
                bool aWriteTag = true)

if aWriteTag is true you write DOMWORKER_SCTAG_BLOB, otherwise you don't. This will reduce the buffer size, but I let bent decide about this.

@@ +598,5 @@
> +    }
> +  };
> +
> +  Closure closure(aCx, aWriter, aClonedObjects);
> +  aFormData->ForEach(Closure::Write, &closure);

If you make ForEach return true if all the callbacks return true, and false if at least 1 fails, you don't need mFailed.
Attachment #8567532 - Flags: review?(amarchesini) → review+
I've documented the updated FormData spec, and its accessibility in workers:

https://developer.mozilla.org/en-US/docs/Web/API/FormData
https://developer.mozilla.org/en-US/docs/Web/API/Worker/Functions_and_classes_available_to_workers#APIs_available_in_workers

We wouldn't normally document something that isn't yet available in Gecko with such a high priority, but it looks to be landing pretty soon, and it is all part of the ongoing work to get Fetch/Service Workers, etc. documented.

Can I get a tech review on these? Bear in mind that FormData has a subpage for each method too.
Switching back to dev-doc-needed. We can't update Fx XY for developers before it lands.
Comment on attachment 8567532 [details] [diff] [review]
Support FormData in workers

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

::: dom/workers/XMLHttpRequest.cpp
@@ +2209,5 @@
> +  mWorkerPrivate->AssertIsOnWorkerThread();
> +  JSContext* cx = mWorkerPrivate->GetJSContext();
> +
> +  if (mCanceled) {
> +    aRv.Throw(UNCATCHABLE_EXCEPTION);

This should be .ThrowUncatchableException() now.

@@ +2229,5 @@
> +    mWorkerPrivate->IsChromeWorker() ?
> +    ChromeWorkerStructuredCloneCallbacks(false) :
> +    WorkerStructuredCloneCallbacks(false);
> +
> +  nsTArray<nsCOMPtr<nsISupports> > clonedObjects;

>>
Attachment #8567532 - Flags: review?(bent.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/10b88a94ab27
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Available in workers details included in the main FormData interface page, and all its member pages:

https://developer.mozilla.org/en-US/docs/Web/API/FormData
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: