1381465 - IPCBlobInputStream should preserve the non-blocking flag of the remote inputStream

Status: RESOLVED DUPLICATE of bug 1397627

(Core :: DOM: File, enhancement)

Description

[Andrea Marchesini [:baku]]

Currently IPCBlobInputStream is always non-blocking.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: blocking.patch

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8887026 [details] [diff] [review]
blocking.patch

Could you explain this bug. Do we have some actual issues without this patch? If so, do we need to fix this on all the branches? If not, why not?

Comment 3

[Andrea Marchesini [:baku]]

Comment on attachment 8887026 [details] [diff] [review]
blocking.patch

Think about a remote file: a DOM file selected from a FilePicker in the parent process and sent to the content process. Its inputStream is a nsFileStream, blocking for I/O.

When we use FileReader on the content process, we move that inputStream to the content process, moving the internal file descriptor. We must preserve the fact that this stream is blocking, otherwise FileReader will do I/O on the main-thread.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

And do we need the patch on branches? What is the actual bug this is fixing? I mean, is this fixing some issue a Firefox user can see today?
I'm puzzled why using File objects in child process isn't all broken without this.

Comment 5

[Andrea Marchesini [:baku]]

> And do we need the patch on branches? What is the actual bug this is fixing?
> I mean, is this fixing some issue a Firefox user can see today?

Everywhere IPCBlob is landed. I/O on main-thread when FileReader is used is bad, but it doesn't break firefox.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Could we still explain. What causes main thread I/O currently, and on which processes does that happen?

Comment 7

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Is there anything remaining blocking this from being landed? 
uplifting bug 1377589 to beta 55 depends on this according to bug 1377589 comment 16.

Comment 8

[Andrea Marchesini [:baku]]

I discussed this with smaug and overholt. I need to have necko able to work correctly with a nsIAsyncInputStream. Currently Necko uses ::Available() to know the size of the inputStream, mainly for the Content-Length header. This happens in several places. I have to fix that, and then I can land this patch.

Comment 9

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Andrea Marchesini [:baku] from comment #8)
> I discussed this with smaug and overholt. I need to have necko able to work
> correctly with a nsIAsyncInputStream. Currently Necko uses ::Available() to
> know the size of the inputStream, mainly for the Content-Length header. This
> happens in several places. I have to fix that, and then I can land this
> patch.

:baku, thanks for much for explaining again here that helped me a lot. :)

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Shouldn't this be a qf bug, even qf:p1. Comment 3 is very worrisome.

Comment 11

[Nika Layzell [:nika] (ni? for response)]

Attachment: Screenshot from 2017-08-29 15-36-20.png

We're definitely reading files on the main thread in the content process right now (probably due to this bug). It's showing up with a fairly high frequency in BHR.

I've attached a screenshot (as linking into hangs.html is unstable right now).

:asuth mentioned that the appearance of SlicedInputStream here might add another wrinkle.

Comment 12

[Andrea Marchesini [:baku]]

This bug is 'already' fixed, but the patch cannot land because necko doesn't support fully nsIAsyncInputStreams yet.
There are several places where necko uses Available() to retrieve the size of the stream and, of course, this doesn't work if the stream is async.

I have a couple of patches for fixing this issue and they are almost green except for a SW test in e10s.
I hoping to finish these patches for this week.

Comment 13

[Andrea Marchesini [:baku]]

I'll work on this in bug 1397627. Patches are ready.