1317725 - [e10s] Crash in mozilla::ipc::MessageChannel::AssertWorkerThread when sending file data in FormData through XHR in a worker

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[cachius]

Attachment: fileworker.js

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.98 Safari/537.36

Steps to reproduce:

I use FormData inside a webworker to send files piece by piece via XHR.
The worker receives a File object via postMessage.
Then it creates a chunk using the File.slice() method,
appends it to a FormData instance and sends this via XHR.


Actual results:

Without e10s everything works fine.
With e10s enabled this suddenly doesn't work anymore, instead
Windows 7: the tab freezes, closable via 2(!) clicks on  its close button
MacOS: the whole browser freezes.


Expected results:

It should work with e10s enabled.

Comment 1

[cachius]

Attachment: Testcase

Comment 2

[Loic]

With e10s enabled.

Freezing should have been fixed by bug 1301094:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d3303b1ac645d0b9def8dabd3e6a5f09aebd274c&tochange=cdc7be84b21a4092e04f1665e5eba5b31e54b63f

But FF50+ are crashing now with the testcase joined to the bug report.

FF51:
https://crash-stats.mozilla.com/report/index/5361c977-5ef7-4630-bc22-ade652161116
FF53:
https://crash-stats.mozilla.com/report/index/35705a81-5fa0-4995-9670-fd3b82161116

Comment 3

[blinky]

Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1a641275e97e48f383f1ed65363109e94c475b9b&tochange=7fac4674fdfa5cd1d2acbd5018f36213be63b010

Regressed by: bug 989619

Comment 4

[Andrea Marchesini [:baku]]

Attachment: blob_formData.patch

Some comments here:

1. the crash itself is fixed by using EnsureBlobForBackgroundManager() in Read/WriteFormData.

2. but then there is a new crash: how we manage sliced remove blob cross threads. We don't set mActorTarget in remoteBlob when they are generated from a slice, this means that when ::Destroy() is called, we crash. This happens because:
https://dxr.mozilla.org/mozilla-central/source/dom/ipc/Blob.cpp#2311

This returns true, bacause mEventTarget is nullptr.
  if (EventTargetIsOnCurrentThread(mActorTarget)) {
    if (mActor) {

This crash, because we are not on the worker thread.
      mActor->AssertIsOnOwningThread();

Solution is to initialiaze mEventTarget also for sliced blobs.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/959c8ce0a649
Correct creation of Sliced Remote Blob actorss when used in FormData in workers, r=smaug

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/959c8ce0a649

Comment 7

[Marcia Knous [:marcia]]

Tracking 53- since this has been fixed.

Comment 8

[Gerry Chang [:gchang]]

Track 51- as the volume of the crash is low.

Hi :baku,
Is this worth uplifting to Beta51 and Aurora52?

Comment 9

[Andrea Marchesini [:baku]]

Comment on attachment 8811208 [details] [diff] [review]
blob_formData.patch

Approval Request Comment
[Feature/regressing bug #]: FormData + XHR + Worker
[User impact if declined]: FF crashes.
[Describe test coverage new/current, TreeHerder]: green on try. test included.
[Risks and why]: The patch is simple: we must create blob actors for each thread also when FormData is serialized. Somehow we forgot to do it for this cloned objected.
[String/UUID change made/needed]: none

Comment 10

[Gerry Chang [:gchang]]

Comment on attachment 8811208 [details] [diff] [review]
blob_formData.patch

Fix a crash. Beta51+ and Aurora52+. Should be in 51 beta 3.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5cac2e6569d0

Comment 12

[Carsten Book [:Tomcat]]

has problems to uplift 

grafting 376698:5cac2e6569d0 "Bug 1317725 -Correct creation of Sliced Remote Blob actorss when used in FormData in workers, r=smaug, a=gchang"
merging dom/base/StructuredCloneHolder.cpp
merging dom/ipc/Blob.cpp
merging dom/workers/test/mochitest.ini
merging dom/workers/test/script_bug1301094.js and dom/workers/test/script_createFile.js to dom/workers/test/script_createFile.js
warning: conflicts while merging dom/workers/test/mochitest.ini! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 13

[Andrea Marchesini [:baku]]

Attachment: m-b

Comment 14

[Gerry Chang [:gchang]]

Hi :Tomcat,
Per comment #13, can you help uplift the new patch to beta?

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/aaa739912343