Closed Bug 1340921 Opened 3 years ago Closed 3 years ago

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

Categories

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

54 Branch
Unspecified
Windows 10
defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox51 --- unaffected
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 + fixed
firefox55 + fixed

People

(Reporter: kasper93, Assigned: baku)

References

Details

(Keywords: crash, regression, Whiteboard: [e10s-multi:+])

Crash Data

Attachments

(10 files, 5 obsolete files)

7.45 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
34.12 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
9.11 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
3.29 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
4.09 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
7.52 KB, patch
Details | Diff | Splinter Review
33.93 KB, patch
Details | Diff | Splinter Review
9.07 KB, patch
Details | Diff | Splinter Review
3.45 KB, patch
Details | Diff | Splinter Review
4.30 KB, patch
Details | Diff | Splinter Review
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.
Flags: needinfo?(amarchesini)
I believe this is nightly only since it uses the "Report Site Issue" button in the nightly hamburger menu.
Yes, only Nightly is affected.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: crash
Version: unspecified → 54 Branch
I think it crashes when webcompat.com is trying to take a screenshot of the Twitch's stream.
(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.
Duplicate of this bug: 1341353
Adding the Mac specific crash to this bug.
Crash Signature: [@ mozilla::ipc::SerializeInputStream] → [@ mozilla::ipc::SerializeInputStream] [@ <name omitted> | mozilla::dom::BlobParent::RecvPBlobStreamConstructor]
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
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.
Summary: Crash in mozilla::ipc::SerializeInputStream → [e10s] Crash in mozilla::ipc::SerializeInputStream with ipc.processCount > 1
Tracking 54+ for this crash.
Attached patch blobStream.patch (obsolete) — Splinter Review
This patch is completed but I want to have bug 1274343 reviewed before asking for a review.
Attachment #8841949 - Attachment is obsolete: true
Attachment #8843296 - Flags: review?(bugs)
Attachment #8843298 - Flags: review?(bugs)
Attachment #8843296 - Flags: review?(bugs)
Attachment #8843297 - Flags: review?(bugs)
Attachment #8843298 - Flags: review?(bugs)
Attachment #8843299 - Flags: review?(bugs)
Duplicate of this bug: 1348795
Whiteboard: [e10s-multi:?]
It looks like bug 1274343 has been reviewed and landed, is this still on your radar, Andrea?
Flags: needinfo?(amarchesini)
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.
Flags: needinfo?(amarchesini)
Whiteboard: [e10s-multi:?] → [e10s-multi:+]
Priority: -- → P1
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.
Attachment #8843296 - Attachment is obsolete: true
Attachment #8843297 - Attachment is obsolete: true
Attachment #8843298 - Attachment is obsolete: true
Attachment #8843299 - Attachment is obsolete: true
Attachment #8851644 - Flags: review?(mrbkap)
Here a new protocol where a stream can be sent in chunks.
Attachment #8851645 - Flags: review?(mrbkap)
Here I use PMemoryStream protocol in PBlob. Note that is just Child to Parent.
Attachment #8851646 - Flags: review?(mrbkap)
Attachment #8851644 - Attachment description: part 0 - Expose DataOwnerAdapter → part 1 - Expose DataOwnerAdapter
Attachment #8851933 - Flags: review?(mrbkap)
Attachment #8851644 - Flags: review?(mrbkap) → review+
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?
Attachment #8851645 - Flags: review?(mrbkap) → review+
Attachment #8851646 - Flags: review?(mrbkap) → review+
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?
Attachment #8851933 - Flags: review?(mrbkap) → review+
Attachment #8851992 - Flags: review?(mrbkap) → review+
> 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.
> Any reason not to use a switch?

Not really. There are multiple types here. I just care of 2 of them. Changing.
Pushed by amarchesini@mozilla.com:
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
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?
Flags: needinfo?(amarchesini)
Flags: needinfo?(amarchesini)
Attachment #8851645 - Flags: approval-mozilla-aurora?
Attachment #8851646 - Flags: approval-mozilla-aurora?
Attachment #8851933 - Flags: approval-mozilla-aurora?
Attachment #8851992 - Flags: approval-mozilla-aurora?
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
Attachment #8851644 - Flags: approval-mozilla-aurora?
Depends on: 1352168
Comment on attachment 8851644 [details] [diff] [review]
part 1 - Expose DataOwnerAdapter

Fix a crash. Aurora54+.
Attachment #8851644 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8851645 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8851646 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8851933 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8851992 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
needs rebasing for aurora
Flags: needinfo?(amarchesini)
Attached patch m-a 1Splinter Review
Flags: needinfo?(amarchesini)
Attached patch m-a 2Splinter Review
Attached patch m-a 3Splinter Review
Attached patch m-a 4Splinter Review
Attached patch m-a 5Splinter Review
Depends on: 1356580
You need to log in before you can comment on or make changes to this bug.