nsTemporaryFileInputStream isn't serializable; causes crash in mozilla::ipc::SerializeInputStream(nsIInputStream*, mozilla::ipc::InputStreamParams&, nsTArray<mozilla::ipc::FileDescriptor>&)

RESOLVED FIXED in Firefox 50

Status

()

RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: peterbe, Assigned: jdm)

Tracking

({crash})

40 Branch
mozilla52
Unspecified
Mac OS X
crash
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(e10s?, firefox49 wontfix, firefox-esr45 unaffected, firefox50 fixed, firefox51 fixed, firefox52 fixed)

Details

(Whiteboard: regression, crash signature, URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
FirefoxDeveloperEdition - 40.0a2 (2015-05-22)

This crash is happening every time I try to upload a video blob extracted from a getUserMedia stream under e10s. 

When switching off e10s, this does not happen. 

With e10s on, the crash does NOT happens if the file to be uploaded is less than 2Mb. 

The code is reproducible every time with this app:
https://github.com/peterbe/airmozilla/tree/bug-1167368-selfies/airmozilla/new/static/new/js

Setting it up might be non-trivial but I can at least reproduce it locally.
jdm: you duped bug 1009039 (with the same crash signature) over to bug 902271, but peterbe is seeing this in Aurora.

Comment 2

4 years ago
Looking at the link on the crash signature, it looks like there's other people, even on Linux and Windows, who crash with the same signature and the stacks at least matching in the top few frames. The total volume is pretty low, but the reports are there.
(Assignee)

Comment 3

4 years ago
This should be easy to diagnose if anybody can catch this in a debugger and figure out the concrete type of mStreams[index] at http://hg.mozilla.org/releases/mozilla-aurora/annotate/0555aa58d0d5/xpcom/io/nsMultiplexInputStream.cpp#l710 .
(Assignee)

Comment 4

3 years ago
frame #0: 0x0000000100e35184 XUL`mozilla::ipc::SerializeInputStream(aInputStream=0x00000001249c3b80, aParams=0x00007fff5fbf8c30, aFileDescriptors=0x00007fff5fbf8f50) + 196 at InputStreamUtils.cpp:49
   46  	  nsCOMPtr<nsIIPCSerializableInputStream> serializable =
   47  	    do_QueryInterface(aInputStream);
   48  	  if (!serializable) {
-> 49  	    MOZ_CRASH("Input stream is not serializable!");
   50  	  }
   51
   52  	  serializable->Serialize(aParams, aFileDescriptors);
(lldb) p aInputStream
(nsTemporaryFileInputStream *) $7 = 0x00000001249c3b80
(Assignee)

Comment 5

3 years ago
What's needed here is for nsTemporaryFileInputStream to implement the nsIIPCSerializableInputStream interface. It should be enough to implement the Serialize method using the PartialFileInputStream IPDL type (see nsPartialFileInputStream::Serialize and nsFileInputStream::Serialize from netwerk/base/nsFileStreams.cpp as models). Perhaps peterbe can help create an isolated testcase that doesn't require employee-only credentials?
Mentor: josh
Whiteboard: [lang=c++]
(Reporter)

Comment 6

3 years ago
Extracting out the code from that app would be a significant amount of work. 
However, anybody who is a vouched Mozillian will be able to sign in to air-dev.allizom.org and reproduce the crash.
(Reporter)

Updated

3 years ago
Blocks: 1175247
tracking-e10s: --- → ?

Updated

3 years ago
Assignee: nobody → mrbkap
tracking-e10s: ? → m8+
This actually got fixed by bug 1153499, which just landed on Aurora.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Comment 8

2 years ago
Blake, that was the wrong bug number, right? Do you remember what the right one was?
Flags: needinfo?(mrbkap)
(Assignee)

Comment 9

2 years ago
I just tried the upload feature on air.mozilla.org and this is definitely not fixed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I have no idea what I was thinking in comment 7 :( I wonder if I was confusing this bug with another bug.
Flags: needinfo?(mrbkap)

Updated

2 years ago
Duplicate of this bug: 1300175
blake - this usecase (large uploads in e10s) is common when using MediaRecorder.  Can we prioritize getting this fixed (and uplifted?)

Renominating for e10s
status-firefox49: --- → affected
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox-esr45: --- → unaffected
tracking-e10s: m8+ → ?
Flags: needinfo?(mrbkap)
(In reply to Josh Matthews [:jdm] from comment #5)
> Perhaps peterbe can help create
> an isolated testcase that doesn't require employee-only credentials?

It looks like this should be reproducible by using MediaRecorder (and setting the media.recorder.max_memory pref to something small if needed) and sending the Blob over a cross-process MessageManager.  The pieces of this could probably be scavenged from one the existing mediarecorder mochitests and any of the chrome-mochitests that uses a frame script.
Summary: crash in mozilla::ipc::SerializeInputStream(nsIInputStream*, mozilla::ipc::InputStreamParams&, nsTArray<mozilla::ipc::FileDescriptor>&) → nsTemporaryFileInputStream isn't serializable; causes crash in mozilla::ipc::SerializeInputStream(nsIInputStream*, mozilla::ipc::InputStreamParams&, nsTArray<mozilla::ipc::FileDescriptor>&)
(Assignee)

Comment 14

2 years ago
Currently giving this a shot, and it's harder than it first appeared. I'm using Air Mozilla's webcam upload feature for my manual testing right now.
Assignee: mrbkap → josh
Mentor: josh
Whiteboard: [lang=c++]
(Assignee)

Comment 15

2 years ago
I have a patch that makes this crash go away and exposes bug 1304910 on air.mozilla.org. I also have an automated test that demonstrates the crashing functionality working thanks to comment 13, which was super helpful.
(Assignee)

Comment 16

2 years ago
Created attachment 8794008 [details] [diff] [review]
Make nsTemporaryFileStream serializable

Andrea, if someone else understands the ownership model of temporary file streams better feel free to redirect. I chose you because they seem to be used specifically for blob-related code.
Attachment #8794008 - Flags: review?(amarchesini)
Comment on attachment 8794008 [details] [diff] [review]
Make nsTemporaryFileStream serializable

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

::: netwerk/base/nsTemporaryFileInputStream.cpp
@@ +182,5 @@
> +nsTemporaryFileInputStream::Serialize(InputStreamParams& aParams,
> +                                      FileDescriptorArray& aFileDescriptors)
> +{
> +  TemporaryFileInputStreamParams params;
> +

MutexAutoLock lock(mFileDescOwner->FileMutex());

@@ +183,5 @@
> +                                      FileDescriptorArray& aFileDescriptors)
> +{
> +  TemporaryFileInputStreamParams params;
> +
> +  if (mFileDescOwner->mFD) {

You can assert this.

nsTemporaryFileInputStream doesn't use mFD to know if the stream is closed or not.
What you really want to do is:

if (!mClosed) {
  ...
} else {
  ...
}

@@ +194,5 @@
> +    params.fileDescriptorIndex() = aFileDescriptors.Length() - 1;
> +
> +    Close();
> +  } else {
> +    NS_WARNING("This file has not been opened (or could not be opened). "

"The stream is already closed. Sending an invalid file descriptor to the other process!" or similar...

@@ +199,5 @@
> +               "Sending an invalid file descriptor to the other process!");
> +
> +    params.fileDescriptorIndex() = UINT32_MAX;
> +  }
> +  params.startPos() = mCurPos;

This must be mStartPos.

@@ +228,5 @@
> +      NS_WARNING("Failed to import file handle!");
> +      return false;
> +    }
> +    mFileDescOwner = new FileDescOwner(fileDesc);
> +  }

} else {
  mClosed = true;
|

@@ +239,5 @@
> +/*Maybe<uint64_t>
> +nsTemporaryFileInputStream::ExpectedStreamLength()
> +{
> +  return Nothing();
> +  }*/

remove this :)
Attachment #8794008 - Flags: review?(amarchesini) → review-

Updated

2 years ago
Blocks: 1304277
(Assignee)

Comment 18

2 years ago
(In reply to Andrea Marchesini [:baku] from comment #17) 
> @@ +199,5 @@
> > +               "Sending an invalid file descriptor to the other process!");
> > +
> > +    params.fileDescriptorIndex() = UINT32_MAX;
> > +  }
> > +  params.startPos() = mCurPos;
> 
> This must be mStartPos.

mCurPos is initialized to mStartPos, and if anything has been read from this stream before serializing it using mStartPos would be incorrect.
(Assignee)

Comment 19

2 years ago
Created attachment 8794224 [details] [diff] [review]
Make nsTemporaryFileStream serializable
Attachment #8794224 - Flags: review?(amarchesini)
(Assignee)

Updated

2 years ago
Attachment #8794008 - Attachment is obsolete: true
Attachment #8794224 - Flags: review?(amarchesini) → review+
(Assignee)

Updated

2 years ago
Flags: needinfo?(mrbkap)
Keywords: checkin-needed
Comment on attachment 8794224 [details] [diff] [review]
Make nsTemporaryFileStream serializable

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

drive-by comment:

::: dom/ipc/tests/blob_verify.sjs
@@ +1,1 @@
> +const CC = Components.Constructor;

Do we need a copyright boilerplate here?
> Do we need a copyright boilerplate here?

Answering my own question - sjs files don't seem to have boilerplate.

Comment 22

2 years ago
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbf98b14f303
Make nsTemporaryFileStream serializable. r=baku
Keywords: checkin-needed

Comment 23

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cbf98b14f303
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago2 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
status-firefox49: affected → wontfix
(Assignee)

Comment 24

2 years ago
Comment on attachment 8794224 [details] [diff] [review]
Make nsTemporaryFileStream serializable

Approval Request Comment
[Feature/regressing bug #]: e10s
[User impact if declined]: Content process crashes on sites that make use of WebRTC recording features.
[Describe test coverage new/current, TreeHerder]: A new test covers this case.
[Risks and why]: Code changes are minimal, and only introduce new code to cover a case that was previously always crashing.
[String/UUID change made/needed]: None
Attachment #8794224 - Flags: approval-mozilla-aurora?

Updated

2 years ago
Duplicate of this bug: 1304277

Updated

2 years ago
No longer blocks: 1304277
Hi Josh, should we uplift this to Beta50 as well?
Flags: needinfo?(josh)
(In reply to Ritu Kothari (:ritu) from comment #26)
> Hi Josh, should we uplift this to Beta50 as well?

I'd love it if we can, since this was a regression in 49 (due to e10s).
Whiteboard: regression
Comment on attachment 8794224 [details] [diff] [review]
Make nsTemporaryFileStream serializable

Fix a crash related to e10s. Take it in 51 aurora.
Attachment #8794224 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 29

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/6492a2635633
status-firefox51: affected → fixed
(Assignee)

Comment 30

2 years ago
Comment on attachment 8794224 [details] [diff] [review]
Make nsTemporaryFileStream serializable

Same reasoning as previous.
Flags: needinfo?(josh)
Attachment #8794224 - Flags: approval-mozilla-beta?

Updated

2 years ago
Version: unspecified → 40 Branch
Comment on attachment 8794224 [details] [diff] [review]
Make nsTemporaryFileStream serializable

I don't see any instances of this crash on Beta50, but given that this fix has stabilized on Nightly and we are still at Beta4, I'll take it.
Attachment #8794224 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 32

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/daf0099fc523
status-firefox50: affected → fixed
Flags: in-testsuite+
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1308916
You need to log in before you can comment on or make changes to this bug.