jake Reporter
	Description • 8 years ago

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160823121617

Steps to reproduce:

Send file data in FormData through XMLHttpRequest in a Worker with Electrolysis enabled.


Actual results:

Firefox freezes/hangs. Can open new tab but cannot navigate, switching between open tabs show a spinner.

	Jim Mathies [:jimm]
	Comment 1 • 8 years ago

This sounds pretty serious, overholt, can you find someone to look at this?

	Josh Matthews [:jdm]
	Comment 2 • 8 years ago

Nothing specific to Service Workers here.

	Ben Kelly [:bkelly, not reviewing]
	Comment 3 • 8 years ago

Reproduced this locally in dev edition.  Andrea, you've been looking at XHR recently.  Do you have any idea what is happening here?

	Andrea Marchesini [:baku] Assignee
	Comment 4 • 8 years ago

I'll work on this tomorrow.

	Andrea Marchesini [:baku] Assignee
	Comment 5 • 8 years ago

I don't know who can review these 2 patches. Here the simple one:

on debug build we crash for 2 reasons: a non-sense assertion (see that we get the main-thread 2 lines above the assertion); and a non-cancellable runnable dispatched to workers.

	Andrea Marchesini [:baku] Assignee
	Comment 6 • 8 years ago

Here the complex part. I try to explain what is happening:

1. We are in a sync event loop because of a XHR in workers doing send(). (yeah... XHR in workers)
2. we want to send a blob. This blob is created by the parent process, so it's a remoteBlob.
3. in order to do the FormData, we have to retrieve the inputStream from the blob. Doing so, we arrive here: https://dxr.mozilla.org/mozilla-central/source/dom/ipc/Blob.cpp#2370

At this point we try to dispatch a runnable to the worker in order to create a  RemoteInputStream:

https://dxr.mozilla.org/mozilla-central/source/dom/ipc/Blob.cpp#2430

but this runnable will never be dispatched because of the sync event loop and we will be blocked here forever:

https://dxr.mozilla.org/mozilla-central/source/dom/ipc/Blob.cpp#2404

My solution is:

1. When a RemoteBlobImpl is created with a WorkerThread as target, I keep also the pointer of the current WorkerPrivate. And I add a WorkerHolder in order to be notified when the worker goes away.

2. There is a new DispatchToTarget method in RemoteBlobImpl. This method is called by GetStream. Here we check if we have a WorkerPrivate and in this case, we wrap the runnable into a WorkerControlRunnable.

3. A WorkerControlRunnable is executed also when a sync event loop is active. So we create the RemoteInputStream correctly. Note: the inputStream is then used in a different thread by FormData; here we are talking just about the creation of it.

4. The WorkerHolder is used to reset mWorkerPrivate when the worker goes away. And everything is protected by a mutex because DispatchToTarget can be called by any thread.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 7 • 8 years ago

Can we get some tests for this.
This is really bad issue.

	Andrea Marchesini [:baku] Assignee
	Comment 8 • 8 years ago

Here a test

	Ben Kelly [:bkelly, not reviewing]
	Comment 9 • 8 years ago

Comment on attachment 8789744 [details] [diff] [review]
part 2 - the complex part

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

::: dom/ipc/Blob.cpp
@@ +2220,5 @@
>  
> +    {
> +      MutexAutoLock lock(mMutex);
> +      mWorkerPrivate = nullptr;
> +      mWorkerHolder = nullptr;

Pretty sure its not safe to destroy a WorkerHolder of the worker thread.  You need a control runnable to dispose of this, no?

	Ben Kelly [:bkelly, not reviewing]
	Comment 10 • 8 years ago

Comment on attachment 8789744 [details] [diff] [review]
part 2 - the complex part

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

::: dom/ipc/Blob.cpp
@@ +2220,5 @@
>  
> +    {
> +      MutexAutoLock lock(mMutex);
> +      mWorkerPrivate = nullptr;
> +      mWorkerHolder = nullptr;

The mutex made me think this could happen on a different thread, but I did not read closely enough.  This is on the worker thread.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 11 • 8 years ago

Comment on attachment 8789821 [details] [diff] [review]
part 3 - test

unclear to me why to use somewhat deprecated onreadystatechange and not just onload with XHR. But either way should be fine.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 12 • 8 years ago

Comment on attachment 8789744 [details] [diff] [review]
part 2 - the complex part

>@@ -2111,16 +2157,29 @@ RemoteBlobImpl::CommonInit(BlobChild* aA
> {
>   MOZ_ASSERT(aActor);
>   aActor->AssertIsOnOwningThread();
>   MOZ_ASSERT(!mIsSlice);
> 
>   mActor = aActor;
>   mActorTarget = aActor->EventTarget();
> 
>+  if (!NS_IsMainThread()) {
>+    mWorkerPrivate = GetCurrentThreadWorkerPrivate();
>+    if (mWorkerPrivate) {
>+      mWorkerHolder = new RemoteBlobImpl::WorkerHolder(this);
>+      if (NS_WARN_IF(!mWorkerHolder->HoldWorker(mWorkerPrivate, Closing))) {
>+        // We don't care too much if the worker is already going away because no
>+        // sync-event-loop can be created at this point.
>+        mWorkerPrivate = nullptr;
>+        mWorkerHolder = nullptr;
>+      }
>+    }
>+  }
I don't understand the setup by just reading this code. What guarantees that RemoteBlobImpl is actually created in the worker thread and not in the main thread and then passed to worker?
http://searchfox.org/mozilla-central/rev/950e13cca1fda6507dc53c243295044e8eda4493/dom/base/StructuredCloneHolder.cpp#552-554 and similar is needed to be read to understand that.
So, could you add some comment about RemoteBlob handling


>+    {
>+      MutexAutoLock lock(mMutex);
>+      mWorkerPrivate = nullptr;
>+      mWorkerHolder = nullptr;
>+    }
This needs some comment or assertion would be even better to hint that we're in the right thread.
Right now one needs to look at the place where mWorkerHolder is set to non-null to figure out that this is right.


+
+  bool
+  WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate)
Missing override?


Do we have tests for the case when the same Blob is being uploaded simultaneously in main thread and a worker?


(tiny bit silly that WorkerHolder doesn't expose the WorkerPrivate it holds via any method. If it did do that, we wouldn't need mWorkerPrivate member variable here.)
(I'm pretty sure we have similar to RemoteBlobControlRunnable class somewhere, but couldn't find where)

	Pulsebot
	Comment 13 • 8 years ago

Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/19a758a7a5ae
part 1 - RemoteBlobImpl::CreateStreamHelper must be a CancelableRunnable, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/03c0969b435f
part 2 - RemoteInputStream must be created using a ControlWorkerRunnable, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdc7be84b21a
part 3 - Test for Blob+XHR+FormData+Workers, r=smaug

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 14 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/19a758a7a5ae
https://hg.mozilla.org/mozilla-central/rev/03c0969b435f
https://hg.mozilla.org/mozilla-central/rev/cdc7be84b21a

	Benjamin Smedberg
	Comment 15 • 8 years ago

Should this be uplifted to 50?

	Andrea Marchesini [:baku] Assignee
	Comment 16 • 8 years ago

Comment on attachment 8789742 [details] [diff] [review]
part 1 - simple fix first

Approval Request Comment
[Feature/regressing bug #]: XHR and RemoteImpl Blob in Workers.
[User impact if declined]: FF freezes when, in a worker, XHR.send() is called with a FormData containing a Blob.
[Describe test coverage new/current, TreeHerder]: test included.
[Risks and why]: These 3 patches are not small and easy.
[String/UUID change made/needed]: none

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 17 • 8 years ago

Hello Jake, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

	jake Reporter
	Comment 18 • 8 years ago

(In reply to Ritu Kothari (:ritu) from comment #17)
> Hello Jake, could you please verify this issue is fixed as expected on a
> latest Nightly build? Thanks!

Hi Ritu,

Yup, looks like it's fixed

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 19 • 8 years ago

Comment on attachment 8789742 [details] [diff] [review]
part 1 - simple fix first

Fix was verified on Nightly, Aurora50+

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 20 • 8 years ago

(In reply to jake from comment #18)

> 
> Hi Ritu,
> 
> Yup, looks like it's fixed

Great! Really appreciate your prompt reply.

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/3c8cb854bf01
https://hg.mozilla.org/releases/mozilla-aurora/rev/a3d30b90a3fb
https://hg.mozilla.org/releases/mozilla-aurora/rev/0a758b4f6289

	Ryan VanderMeulen [:RyanVM]
	Comment 22 • 8 years ago

Bustage follow-up (had to rebase around bug 1293690):
https://hg.mozilla.org/releases/mozilla-aurora/rev/2d4867f1d033