Closed Bug 1494176 Opened Last year Closed Last year

FF 62 fails to send some multipart forms

Categories

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

62 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla64
Tracking Status
firefox-esr60 --- unaffected
firefox62 --- wontfix
firefox63 --- wontfix
firefox64 --- verified

People

(Reporter: opennota, Assigned: baku)

References

Details

(Keywords: regression)

Attachments

(4 files, 1 obsolete file)

Attached file 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.
I'm attaching the source code of a server which demonstrates the problem. It will listen on localhost:1234
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
Status: UNCONFIRMED → NEW
Component: Untriaged → HTML: Form Submission
Depends on: 1434553, 1464090
Ever confirmed: true
Flags: needinfo?(amarchesini)
Keywords: regression
Product: Firefox → Core
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?
Flags: needinfo?(amarchesini) → needinfo?(rob)
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>
Flags: needinfo?(rob)
> 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.
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]"
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).
Flags: needinfo?(nfroyd)
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.
Attached patch pipe3.patch (obsolete) — Splinter Review
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?
Attachment #9012482 - Flags: feedback?(honzab.moz)
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
Attachment #9012482 - Flags: feedback?(honzab.moz) → feedback+
Not sure what the question is here; I don't think we should try to make pipes fully support seek/tell, fwiw.
Flags: needinfo?(nfroyd)
(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.
> 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?
Attached patch pipe3.patchSplinter Review
Assignee: nobody → amarchesini
Attachment #9012482 - Attachment is obsolete: true
Attachment #9013250 - Flags: review?(honzab.moz)
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.
Attachment #9013250 - Flags: review?(honzab.moz) → review+
Priority: -- → P2
This patch is needed otherwise we duplicate any mimeInputStream received from the child process.
Attachment #9014081 - Flags: review?(honzab.moz)
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.
Attachment #9014081 - Flags: review?(honzab.moz) → review+
Pushed by amarchesini@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/f2ff07352cbe
https://hg.mozilla.org/mozilla-central/rev/90592dc5c3a8
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Blocks: 1434553, 1464090
No longer depends on: 1434553, 1464090
Flags: qe-verify+
Depends on: 1496581
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Component: HTML: Form Submission → DOM: Core & HTML
Blocks: 1506562
You need to log in before you can comment on or make changes to this bug.