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

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Peter Bengtsson [:peterbe]]

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.

Comment 1

[(not currently active) Ted Mielczarek]

jdm: you duped bug 1009039 (with the same crash signature) over to bug 902271, but peterbe is seeing this in Aurora.

Comment 2

[Robert Kaiser]

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.

Comment 3

[Josh Matthews [:jdm]]

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 .

Comment 4

[Josh Matthews [:jdm]]

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

Comment 5

[Josh Matthews [:jdm]]

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?

Comment 6

[Peter Bengtsson [:peterbe]]

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.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

This actually got fixed by bug 1153499, which just landed on Aurora.

Comment 8

[Josh Matthews [:jdm]]

Blake, that was the wrong bug number, right? Do you remember what the right one was?

Comment 9

[Josh Matthews [:jdm]]

I just tried the upload feature on air.mozilla.org and this is definitely not fixed.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

I have no idea what I was thinking in comment 7 :( I wonder if I was confusing this bug with another bug.

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

blake - this usecase (large uploads in e10s) is common when using MediaRecorder.  Can we prioritize getting this fixed (and uplifted?)

Renominating for e10s

Comment 13

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(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.

Comment 14

[Josh Matthews [:jdm]]

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.

Comment 15

[Josh Matthews [:jdm]]

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.

Comment 16

[Josh Matthews [:jdm]]

Attachment: 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.

Comment 17

[Andrea Marchesini [:baku]]

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 :)

Comment 18

[Josh Matthews [:jdm]]

(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.

Comment 19

[Josh Matthews [:jdm]]

Attachment: Make nsTemporaryFileStream serializable

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

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?

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

> Do we need a copyright boilerplate here?

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

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbf98b14f303
Make nsTemporaryFileStream serializable. r=baku

Comment 23

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/cbf98b14f303

Comment 24

[Josh Matthews [:jdm]]

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

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Josh, should we uplift this to Beta50 as well?

Comment 27

[Randell Jesup [:jesup] (needinfo me)]

(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).

Comment 28

[Gerry Chang [:gchang]]

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

Fix a crash related to e10s. Take it in 51 aurora.

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6492a2635633

Comment 30

[Josh Matthews [:jdm]]

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

Same reasoning as previous.

Comment 31

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

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

Comment 33

[Ryan VanderMeulen [:RyanVM]]

And a bustage fix for Beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/98f174fa9b903aca83ecfb596e13c4cadc6ac2f4