Closed Bug 1696386 Opened 4 years ago Closed 2 years ago

IPC input streams can't be rewinded

Categories

(Core :: DOM: Content Processes, defect, P2)

defect

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: valentin, Unassigned)

References

Details

Attachments

(1 obsolete file)

(In reply to Valentin Gosu [:valentin] (he/him) from bug 1676331 comment #35)

Summary of the issue in bug 1676331:

Content page is creating a blob larger that 256Kb in size and posting it.
In the parent process, we do an authRetry after the authentication completes so we try to rewind the stream and retry the request.
Due to the fact that the deserialized stream does not implement nsISeekableStream we fail to rewind the stream.
The channel will hang, because we attempt to read from the stream and there is no more data there.

See https://phabricator.services.mozilla.com/D107069 for a test case.

We should try to find a way to rewind the stream. Maybe we can add an API that allows us to rewind the stream asynchronously.

Nika has some thoughts on this, and would like to discuss with Valentin. Leaving NI for her as a reminder.

Flags: needinfo?(nika)

In bug 1681529 I added a helper type, SeekableStreamWrapper for handling this kind of situation where we need to ensure a stream which is received over IPC implements nsISeekableStream. You probably want to use this wrapper type here as well. You can see an example usage in the deserialization code for nsMIMEInputStream: https://searchfox.org/mozilla-central/rev/9bf82ef9c097ee6af0e34a1d21c073b2616cc438/netwerk/base/nsMIMEInputStream.cpp#420

It may also be possible to replace the current code for HttpBaseChannel::EnsureUploadStreamIsCloneable with this approach, though I am not 100% certain on that. https://searchfox.org/mozilla-central/rev/9bf82ef9c097ee6af0e34a1d21c073b2616cc438/netwerk/protocol/http/HttpBaseChannel.cpp#868

Flags: needinfo?(nika)
Assignee: nobody → valentin.gosu
Status: NEW → ASSIGNED
Severity: -- → S3
Priority: -- → P2

Hi Valentin, are you still planning to work on this? Thanks!

Flags: needinfo?(valentin.gosu)

I am not currently working on this.

Assignee: valentin.gosu → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(valentin.gosu)

I noticed that my changes in bug 1754004 actually seem to have partially fixed this issue, as the form is now submitting the first time. This is happening because I changed how streams are serialized somewhat in those patches so that nsBufferedInputStream now correctly only reports that it is seekable if the underlying stream is seekable, and the HTTP channel performs a somewhat complex manual check to convert streams sent to it over IPC into a standard form which satisfies the required properties of being seekable, cloneable, and synchronously available.

I found this issue as this change made the first navigation in browser_post_auth.js succeed, such that the check for it to fail the first time does not pass any longer (https://searchfox.org/mozilla-central/rev/f8db81665dc2833fff09dc7eef200539ac1fd351/netwerk/test/browser/browser_post_auth.js#69-75).

The severity field for this bug is set to S3. However, the following bug duplicate has higher severity:

:hsingh, could you consider increasing the severity of this bug to S2?

For more information, please visit auto_nag documentation.

Flags: needinfo?(hsingh)
Attachment #9207687 - Attachment is obsolete: true
Flags: needinfo?(hsingh) → needinfo?(jstutte)

Is this actually a Content Processes thing?

Flags: needinfo?(jstutte) → needinfo?(krosylight)

The code being in netwerk/base sounds like it should be a network thing...

Flags: needinfo?(krosylight)

Maybe Valentin knows.

Flags: needinfo?(valentin.gosu)

As nika pointed out, this should be fixed now. We even have a test for it.
Not sure if there are cases where we can't rewind, but the initial case I filed this bug for is now fixed by bug 1754004.

Status: NEW → RESOLVED
Closed: 2 years ago
Flags: needinfo?(valentin.gosu)
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: