1494176 - FF 62 fails to send some multipart forms

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[opennota]

Attachment: bug.txt.bz2

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:62.0) Gecko/20100101 Firefox/62.0
Build ID: 20180920131237

Steps to reproduce:

I enter a text into a textarea in a multipart form and click Submit.

The text file is attached (bug.txt.bz2).

The form is

<form method="POST" enctype="multipart/form-data" action="/">
<textarea name="text"></textarea>
<button type="submit">Submit</button>
</form>



Actual results:

Firefox 62.0.2 makes the POST request, but doesn't send any form data to the other side. The tab icon keeps oscillating. The Network tab in the Developer tools displays the POST request, but when I click it FF says there are no headers, no cookies, no parameters etc.


Expected results:

FF should sent the data. FF 61.0.2 does. FF 62.0.2 doesn't.

Also, it works only when I copy and paste the whole file into the form. I tried to minimize the test case, but unsuccessfully.

Comment 1

[opennota]

Attachment: A simple web server in Go which demonstrates the problem

I'm attaching the source code of a server which demonstrates the problem. It will listen on localhost:1234

Comment 2

[Matthias Versen [:Matti]]

I can confirm a regression in 2 steps

1) from "working" to doesn't seem to finish with the post
>018-09-26T05:38:58: DEBUG : Found commit message:
>Bug 1434553 - Implement nsIInputStreamLength and nsIAsyncInputStreamLength - part 9 - necko and docShell, r=mayhemer, r=smaug

2) from never finished the post request to an empty param section of the post request in the developer tools networking section
>2018-09-26T05:55:12: DEBUG : Found commit message:
>Bug 1464090 - Assertion in InputStreamLengthHelper::GetAsyncLength to avoid dealing with nsIAsyncInputStream, r=mayhemer

Comment 3

[Andrea Marchesini [:baku]]

On a debug build, we crash when copying and pasting the content of the file because of NS_OpenAnonymousTemporaryNsIFile() is used on the content process:

#0  0x00007fba6cc0c2f1 in NS_OpenAnonymousTemporaryNsIFile(nsIFile**) (aFile=0x7ffe706999d8) at /home/baku/Sources/m/tmp/src/xpcom/io/nsAnonymousTemporaryFile.cpp:87
#1  0x00007fba6cc0c686 in NS_OpenAnonymousTemporaryFile(PRFileDesc**) (aOutFileDesc=0x56129fb41188) at /home/baku/Sources/m/tmp/src/xpcom/io/nsAnonymousTemporaryFile.cpp:127
#2  0x00007fba716f86f6 in DataStruct::WriteCache(nsISupports*, unsigned int) (this=0x56129fb41178, aData=0x56129f3c5340, aDataLen=1209798) at /home/baku/Sources/m/tmp/src/widget/nsTransferable.cpp:133
#3  0x00007fba716f8609 in DataStruct::SetData(nsISupports*, unsigned int, bool) (this=0x56129fb41178, aData=0x56129f3c5340, aDataLen=1209798, aIsPrivateData=false) at /home/baku/Sources/m/tmp/src/widget/nsTransferable.cpp:79
#4  0x00007fba716f9890 in nsTransferable::SetTransferData(char const*, nsISupports*, unsigned int) (this=0x56129fb3fbd0, aFlavor=0x56129f1f199c "text/unicode", aData=0x56129f3c5340, aDataLen=1209798)
    at /home/baku/Sources/m/tmp/src/widget/nsTransferable.cpp:382
#5  0x00007fba716ddef8 in nsClipboardProxy::GetData(nsITransferable*, int) (this=0x56129ed114d0, aTransferable=0x56129fb3fbd0, aWhichClipboard=0) at /home/baku/Sources/m/tmp/src/widget/nsClipboardProxy.cpp:88
#6  0x00007fba71884cbf in mozilla::TextEditor::PasteAsAction(int) (this=0x56129f7b6dd0, aClipboardType=0) at /home/baku/Sources/m/tmp/src/editor/libeditor/TextEditorDataTransfer.cpp:330
#7  0x00007fba717a4f10 in mozilla::EditorEventListener::HandleMiddleClickPaste(mozilla::dom::MouseEvent*) (this=0x56129fa54cb0, aMouseEvent=0x56129f2aafc0)
    at /home/baku/Sources/m/tmp/src/editor/libeditor/EditorEventListener.cpp:722
#8  0x00007fba717a4b98 in mozilla::EditorEventListener::MouseClick(mozilla::dom::MouseEvent*) (this=0x56129fa54cb0, aMouseEvent=0x56129f2aafc0) at /home/baku/Sources/m/tmp/src/editor/libeditor/EditorEventListener.cpp:674
#9  0x00007fba717a2c02 in mozilla::EditorEventListener::HandleEvent(mozilla::dom::Event*) (this=0x56129fa54cb0, aEvent=0x56129f2aafc0) at /home/baku/Sources/m/tmp/src/editor/libeditor/EditorEventListener.cpp:458
#10 0x00007fba7047b3fa in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*) (this=0x56129f6bb040, aListener=0x56129e7a4170, aDOMEvent=0x56129f2aafc0, aCurrentTarget=0x56129e882690) at /home/baku/Sources/m/tmp/src/dom/events/EventListenerManager.cpp:1106


Rob, it seems that you added this feature in DataTransfer. Can you take a look?

Comment 4

[Rob Wu [:robwu]]

Andrea, the crash is unrelated to the bug. The crash happens because you tried to copy&paste a large amount of data in the textarea, which causes Firefox to switch to a file-based cache. It is not a critical issue and already tracked here: bug 1482540.

If you want to fill the text area from the file content, consider using JavaScript, e.g.

<input type="file" onchange="var fr=new FileReader();fr.onloadend=function(){document.querySelector('textarea').value=fr.result;};fr.readAsBinaryString(this.files[0])">
<form method="POST" enctype="multipart/form-data" action="/">
<textarea name="text"></textarea>
<button type="submit">Submit</button>
</form>

Comment 5

[Andrea Marchesini [:baku]]

> Andrea, the crash is unrelated to the bug. The crash happens because you
> tried to copy&paste a large amount of data in the textarea, which causes
> Firefox to switch to a file-based cache. It is not a critical issue and
> already tracked here: bug 1482540.

Ok. I didn't know we already have that bug and that crash was blocking my testing.
I'll continue the testing using your suggested code.

Comment 6

[Andrea Marchesini [:baku]]

There are 2 issues here:

1. Without copy&paste, the upload is correctly done but devtools network panel shows no data.

2. the reload of the page doesn't seem to work. When I try to reload the POST request, I see this error on stderr: Handler function BrowsingContextTargetActor.prototype.reload's delayed body threw an exception: [Exception... "Component returned failure code: 0x80470002 (NS_BASE_STREAM_CLOSED) [nsIWebNavigation.reload]"

Comment 7

[Andrea Marchesini [:baku]]

Here what is going on here:

1. The child process wants to send more than 1mb of data via a POST request. Because of that we create a PChildToParent stream.

2. On the parent side we have a nsPipeInputStream which receives data from the child.

3. The upload is done correctly, but when devtools wants to show the network request, it does a tell() + seek(): https://searchfox.org/mozilla-central/rev/ce57be88b8aa2ad03ace1b9684cd6c361be5109f/devtools/shared/webconsole/network-helper.js#145-146

4. nsPipeInputStream doesn't support seek() at all. And also tell() fails, returning NS_BASE_STREAM_CLOSED: https://searchfox.org/mozilla-central/rev/ce57be88b8aa2ad03ace1b9684cd6c361be5109f/xpcom/io/nsPipe3.cpp#1529-1533

5. devtools is not able to read the inputStream content and the network panel is kept empty.

For a similar reason the reloading of the page doesn't work: seek() returns an error in nsPipeInputStream.
We should probably expose a correct implementation of ::Seek() and ::Tell() in nsPipeInputStream or clone it (but this would have a terrible performance impact).

Comment 8

[opennota]

With Javascript (I used `fr.readAsText(this.files[0], 'utf-8')` instead of `fr.readAsBinaryString(this.files[0])` because otherwise the inserted text is garbled) the data is sent, but the request is never finished anyway.

Comment 9

[Andrea Marchesini [:baku]]

Attachment: pipe3.patch

I'm not particularly happy about this approach because it seems a work-around for nsPipeInputStream not implementing ::Seek() correctly. Mayhemer, do you see any better approach?

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 9012482 [details] [diff] [review]
pipe3.patch

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

so, to describe the patch (please provide some next time, even when asking just for a feedback), this is trying to ensure that we duplicate all the data, so that devtools see the POST data and reload can re-POST, right?

we have to keep the data in memory already and the user has to be prepared that this will simply consume some memory and potentially ran out of it.  still, this is somewhat a corner case (the first POST works!) and we could easily say "data too large to show" in devtools (I don't think 1MB of data can be inspected easily anyway) and we have "document expired" error for reloads (would have to find out how to report it exactly in this case).

still, I don't oppose that much to doing it your way, I don't see a better solution myself.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +913,5 @@
>    // but the complexity does not seem worth it yet.  Just fail if
>    // this is called more than once simultaneously.
>    NS_ENSURE_FALSE(mUploadCloneableCallback, NS_ERROR_UNEXPECTED);
>  
> +  if (!mUploadStream) {

add a good comment why you do this

@@ +922,4 @@
>    // If the CloneUploadStream() will succeed, then synchronously invoke
>    // the callback to indicate we're already cloneable.
> +  nsCOMPtr<nsISeekableStream> seekable = do_QueryInterface(mUploadStream);
> +  if (NS_SUCCEEDED(seekable->Seek(nsISeekableStream::NS_SEEK_SET, 0)) &&

oh!  you really should check seekable not null, and again, please write a comment

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +722,5 @@
> +      [self]() {
> +        self->TryInvokeAsyncOpen(NS_OK);
> +      });
> +  ++mAsyncOpenBarrier;
> +  mChannel->EnsureUploadStreamIsCloneable(r);

again, add comments why you do this and what effect this has

Comment 11

[Nathan Froyd [:froydnj]]

Not sure what the question is here; I don't think we should try to make pipes fully support seek/tell, fwiw.

Comment 12

[Honza Bambas (:mayhemer)]

(In reply to Nathan Froyd [:froydnj] from comment #11)
> Not sure what the question is here; I don't think we should try to make
> pipes fully support seek/tell, fwiw.

Yes, pipes can't be seekable from the nature of their function.  Hence, we do clone the stream in the patch (read the pipe and copy the content to a very large buffer).  I'd personally rather let devtools and reload fail, than waste memory this way.

Maybe we could have a limit?  When over the limit, show something more useful to the use then.

Comment 13

[Andrea Marchesini [:baku]]

> Yes, pipes can't be seekable from the nature of their function.  Hence, we
> do clone the stream in the patch (read the pipe and copy the content to a
> very large buffer).  I'd personally rather let devtools and reload fail,
> than waste memory this way.


We still need to clone the stream in order to support the reload of the page, right?

Comment 14

[Andrea Marchesini [:baku]]

Attachment: pipe3.patch

Comment 15

[Honza Bambas (:mayhemer)]

Comment on attachment 9013250 [details] [diff] [review]
pipe3.patch

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

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +919,5 @@
> +    aCallback->Run();
> +    return NS_OK;
> +  }
> +
> +  // Some nsSeekableStreams do not implement ::Seek() correctly (see

should this rather state that ::Seek() may not be implemented at all?  pipes don't implement seek from their nature.   I think the reason our pipe even implements the seekable interface is to get Tell() - which doesn't seem correct too, but probably sufficiently serves the original purpose why it has been added.

Comment 16

[Andrea Marchesini [:baku]]

Attachment: part 2 - nsMIMEInputStream must be cloneable

This patch is needed otherwise we duplicate any mimeInputStream received from the child process.

Comment 17

[Honza Bambas (:mayhemer)]

Comment on attachment 9014081 [details] [diff] [review]
part 2 - nsMIMEInputStream must be cloneable

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

::: netwerk/base/nsMIMEInputStream.cpp
@@ +528,5 @@
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +        return rv;
> +    }
> +
> +    nsCOMPtr<nsIMIMEInputStream> mimeStream = new nsMIMEInputStream();

a copy ctor would nice, but definitely not an r+ blocker.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2ff07352cbe
nsHttpChannel must clone the upload stream if it doesn't implement ::Seek(), r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/90592dc5c3a8
nsMIMEInputStream must be cloneable, r=mayhemer

Comment 19

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/f2ff07352cbe
https://hg.mozilla.org/mozilla-central/rev/90592dc5c3a8

Comment 20

[Hani Yacoub, Desktop QA]

Build ID: 20181019005426
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:64.0) Gecko/20100101 Firefox/64.0

Verified as fixed on Firefox Nightly 64.0a1 on Ubuntu 16.04 x64, Windows 10 x 64 and on Mac OS X 10.13.

Comment 21

[opennota]

The bug is here again (FF 70.0.1). When sending 1200000 bytes of text no data comes through. With 1100000 bytes everything's fine.

Comment 22

[Rob Wu [:robwu]]

@opennota Could you use mozregression to find out the cause of the regression, and report a new bug? If you're able to, please link to this bug and the bug that caused the regression. If you cannot add that info to the new bug, just post a comment here and I'll link the bugs.

Comment 23

[opennota]

Neither mozregression-gui nor the CLI version works for me. The first one complains about missing Qt4 libraries (I have only Qt5), and the first nightly build the CLI version runs just exits in one or two seconds with the return code 134:

Was this nightly build good, bad, or broken? (type 'good', 'bad', 'skip', 'retry' or 'exit' and press Enter):
0:13.99 WARNING: Process exited with code 134
bad
0:59.96 ERROR: Build was expected to be good! The initial good/bad range seems incorrect.

Comment 24

[opennota]

Nevermind, looks like this time the bug is caused by an extension (NoScript).