1340921 - [e10s] Crash in mozilla::ipc::SerializeInputStream with ipc.processCount > 1

Status: RESOLVED FIXED

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

Description

[Kacper Michajłow [:kasper93]]

This bug was filed from the Socorro interface and is 
report bp-2097f8f8-ac58-459d-b42f-8fc582170219.
=============================================================

Open any stream on twitch.tv and try to use "Report Site Issue". Firefox will crash.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

I believe this is nightly only since it uses the "Report Site Issue" button in the nightly hamburger menu.

Comment 2

[Loic]

Yes, only Nightly is affected.

Comment 3

[Loic]

I think it crashes when webcompat.com is trying to take a screenshot of the Twitch's stream.

Comment 4

[Marcia Knous [:marcia]]

Similar report in Bug 1341353.

Comment 5

[Loic]

(In reply to Marcia Knous [:marcia - use ni] from comment #4)
> Similar report in Bug 1341353.

same issue, with ipc.processCount=1, it doesn't crash anymore. You can dupe one of them.

Comment 7

[Marcia Knous [:marcia]]

Adding the Mac specific crash to this bug.

Comment 8

[Andrea Marchesini [:baku]]

The issue here is this:

1. Multi-e10s
2. child-process-1 origin-A creates a memory blob (a lot of data is needed).
3. this blob is sent to the parent process as a BlobURL. Just because the memory blob is big, we create a IPC input stream from child to parent.
4. the parent broadcast the blob URL to child-process-2 origin-A (or anything that has access to that blobURL)
5. child-process-2 origin-A does blob::getInternalStream. We try to create a remote blob from the parent to child-process-2 but the parent process has a IPC inputStream from child-process-1 and this is a pipe stream, non serializable.
6. we crash.

Comment 9

[Marcia Knous [:marcia]]

Tracking 54+ for this crash.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: blobStream.patch

This patch is completed but I want to have bug 1274343 reviewed before asking for a review.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: part 1 - PParentToChildStream for PBlob

Comment 12

[Andrea Marchesini [:baku]]

Attachment: part 2 - Buffered stream for BlobURL - this is needed for many necko channels

Comment 13

[Andrea Marchesini [:baku]]

Attachment: part 3 - remove non used IPC inputStream protocols

Comment 14

[Andrea Marchesini [:baku]]

Attachment: part 4 - Use of AutoIPCStream when the stream is not serializable

Comment 16

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

[Tracking Requested - why for this release]:

Comment 17

[Nathan Froyd [:froydnj]]

It looks like bug 1274343 has been reviewed and landed, is this still on your radar, Andrea?

Comment 18

[Andrea Marchesini [:baku]]

Yes, I'm doing quite a lot of work in inputStream and IPC serialization. I'm basically doing the refactoring of PBlob using PChildToParentStream and PParentToChildStream. If there are bugs related to inputStream and IPC, NI me.

Comment 19

[Andrea Marchesini [:baku]]

Attachment: part 1 - Expose DataOwnerAdapter

Here the 'temporary' solution for having multi-10s and pblob without waiting for my refactoring. The idea is to send memory blob in chunks.
In order to do that, I want to use an existing class, owned by MemoryBlobImpl.
Note that I removed a comment because that doesn't apply anymore in the new File/Blob architecture.

Comment 20

[Andrea Marchesini [:baku]]

Attachment: part 2 - PMemoryStream

Here a new protocol where a stream can be sent in chunks.

Comment 21

[Andrea Marchesini [:baku]]

Attachment: part 3 - Use of PMemoryStream in PBlob

Here I use PMemoryStream protocol in PBlob. Note that is just Child to Parent.

Comment 22

[Andrea Marchesini [:baku]]

Attachment: part 4 - deleting the actor

Comment 23

[Andrea Marchesini [:baku]]

Attachment: part 5 - Memory stream must be cloneable

Comment 24

[Andrea Marchesini [:baku]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=42993be3c3fdcca004a7df5cd31c32d9fef6adcd&selectedJob=86973406
green on try.

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8851645 [details] [diff] [review]
part 2 - PMemoryStream

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

r=me with my questions answered.

::: dom/file/ipc/MemoryStreamParent.cpp
@@ +26,5 @@
> +    return IPC_FAIL_NO_REASON(this);
> +  }
> +
> +  static constexpr size_t elementSizeMultiplier =
> +    sizeof(aData[0]) / sizeof(char);

I'm pretty sure that sizeof(unsigned char) == sizeof(char) == 1 per the spec.

@@ +28,5 @@
> +
> +  static constexpr size_t elementSizeMultiplier =
> +    sizeof(aData[0]) / sizeof(char);
> +
> +  void* buffer = malloc(dataLength * elementSizeMultiplier);

Out of curiosity, is there any reason to prefer using malloc/free with DataOwner instead of new[]/delete[] (which would also avoid having to deal with the sizeof either of them? Maybe fodder for a followup bug?

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8851933 [details] [diff] [review]
part 4 - deleting the actor

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

::: dom/file/ipc/Blob.cpp
@@ +675,5 @@
> +
> +void
> +DeleteStreamMemoryFromBlobData(BlobData& aBlobData)
> +{
> +  if (aBlobData.type() == BlobData::TBlobDataStream) {

Any reason not to use a switch?

@@ +701,5 @@
> +
> +void
> +DeleteStreamMemory(AnyBlobConstructorParams& aParams)
> +{
> +  if (aParams.type() == AnyBlobConstructorParams::TFileBlobConstructorParams) {

Any reason not to use a switch?

Comment 27

[Andrea Marchesini [:baku]]

> I'm pretty sure that sizeof(unsigned char) == sizeof(char) == 1 per the spec.

Yeah. I added that because that was the previous code before my porting to IPCStream.

> Out of curiosity, is there any reason to prefer using malloc/free with
> DataOwner instead of new[]/delete[] (which would also avoid having to deal
> with the sizeof either of them? Maybe fodder for a followup bug?

No reasons. I'll do it as follow up.

Comment 28

[Andrea Marchesini [:baku]]

> Any reason not to use a switch?

Not really. There are multiple types here. I just care of 2 of them. Changing.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9a26f2c0e64
Introduce PMemoryStream for having PBlob and Multi-e10s happy - part 1 - Expose DataOwnerAdapter, r=mrbkap
https://hg.mozilla.org/integration/mozilla-inbound/rev/81292d0b435b
Introduce PMemoryStream for having PBlob and Multi-e10s happy - part 2 - PMemoryStream actor, r=mrbkap
https://hg.mozilla.org/integration/mozilla-inbound/rev/90e0ee883a3e
Introduce PMemoryStream for having PBlob and Multi-e10s happy - part 3 - PMemoryStream and PBlob, r=mrbkap
https://hg.mozilla.org/integration/mozilla-inbound/rev/791bff0c8196
Introduce PMemoryStream for having PBlob and Multi-e10s happy - part 4 - Delete of the PMemoryStream actor, r=mrbkap
https://hg.mozilla.org/integration/mozilla-inbound/rev/a58cea097599
Introduce PMemoryStream for having PBlob and Multi-e10s happy - part 5 - Make MemoryBlobImpl::DataOwner cloneable, r=mrbkap

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f9a26f2c0e64
https://hg.mozilla.org/mozilla-central/rev/81292d0b435b
https://hg.mozilla.org/mozilla-central/rev/90e0ee883a3e
https://hg.mozilla.org/mozilla-central/rev/791bff0c8196
https://hg.mozilla.org/mozilla-central/rev/a58cea097599

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Given that our intent is to run e10s-multi experiments on Fx54 when it goes to Beta, what do you think about backporting this fix to Aurora now?

Comment 32

[Andrea Marchesini [:baku]]

Comment on attachment 8851644 [details] [diff] [review]
part 1 - Expose DataOwnerAdapter

Approval Request Comment
[Feature/Bug causing the regression]: multi-e10s and PBlob
[User impact if declined]: in multi-10s PBlob can crash
[Is this code covered by automated tests?]: not specifically for multi-e10s but we have many tests for PBlob and streams
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: yes: BroadcastChannel of a big memory blob (more than 1mb). 
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low
[Why is the change risky/not risky?]: We stream the PBlob content in chunks. The code is not super complex but it introduces a new IPC actor.
[String changes made/needed]: none

Comment 33

[Gerry Chang [:gchang]]

Comment on attachment 8851644 [details] [diff] [review]
part 1 - Expose DataOwnerAdapter

Fix a crash. Aurora54+.

Comment 34

[Carsten Book [:Tomcat]]

needs rebasing for aurora

Comment 35

[Andrea Marchesini [:baku]]

Attachment: m-a 1

Comment 36

[Andrea Marchesini [:baku]]

Attachment: m-a 2

Comment 37

[Andrea Marchesini [:baku]]

Attachment: m-a 3

Comment 38

[Andrea Marchesini [:baku]]

Attachment: m-a 4

Comment 39

[Andrea Marchesini [:baku]]

Attachment: m-a 5

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e9e6146c820b
https://hg.mozilla.org/releases/mozilla-aurora/rev/05da998218c4
https://hg.mozilla.org/releases/mozilla-aurora/rev/8d1a61d60b99
https://hg.mozilla.org/releases/mozilla-aurora/rev/02a7a514a265
https://hg.mozilla.org/releases/mozilla-aurora/rev/d0b605e7f5c7