1360887 - YouTube upload fails

Status: RESOLVED FIXED

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

Description

[Yani]

Attachment: youtubefail.png

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

Steps to reproduce:

Upload a video by both drop and drag and file select


Actual results:

Uploading 0% and nothing happens


Expected results:

YouTube sends message back on upload progress usually well within 30 seconds. 

Comparison with Chrome provided.
Checked in Safe Mode.
Error only started in last few days.

Comment 1

[Alice0775 White]

I can reproduce the problem on Windows10 Nightly55 32/64bit.


Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c35f0ad34397990adec0612015eb1a2a566434da&tochange=cf6065f64f9a8aef42144e7938c9c1aec92d8677


regressed by:
cf6065f64f9a	Andrea Marchesini — Bug 1359172 - RemoteInputStream must create a correct inputStream when used and sliced in a separate process, r=smaug
2629873b5fdc	Andrea Marchesini — Bug 1358115 - Use IPCBlob in DataTransfer, r=smaug
049673ad3893	Andrea Marchesini — Bug 1358113 - Use IPCBlob in File.createFromNsIFile/createFromFileName, r=smaug
2ef7ae7a605f	Andrea Marchesini — Bug 1358114 - Use IPCBlob in BlobURL, r=smaug
7d7576b29e2b	Andrea Marchesini — Bug 1358111 - Use IPCBlob in Entries API - part 2 - Entries API, r=smaug
b3cc0e03d9b7	Andrea Marchesini — Bug 1358111 - Use IPCBlob in Entries API - part 1 - GetFilesHelper, r=smaug
4de83aa3b817	Andrea Marchesini — Bug 1358109 - Use IPCBlob in PFilePicker, r=smaug


@:baku
Your patch seems to trigger the regression. Could you look this?

Comment 2

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

If fixing this takes time, can we back out problematic patches, like very soon?

Comment 4

[Andrea Marchesini [:baku]]

Attachment: sw.patch

Comment 5

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

Comment on attachment 8863723 [details] [diff] [review]
sw.patch

There is no explanation why this change is needed, and it isn't very obvious from the patch.

Why do we now need DoCopy to be more complicated?

Why can we remove that code from InitiateFetch?

We need tests here. (Landing r+'ed regression fix would be ok though, but I don't understand the change. Do we have other code in tree which requires similar changes as what is done to DoCopy? Please audit.)

Comment 6

[Andrea Marchesini [:baku]]

Attachment: sw2.patch

Comment 7

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

Comment on attachment 8863766 [details] [diff] [review]
sw2.patch

># HG changeset patch
># User Andrea Marchesini <[e-mail]>
># Parent  df523bbb60bceb8e9dd9c24d7aaf6ce2d3642851
>Bug 1360887 - nsBufferedInputStream must support nsIAsyncInputStream, r?smaug
This commit message says that buffered stream must support async stream.


> NS_INTERFACE_MAP_BEGIN(nsBufferedInputStream)
>-    NS_INTERFACE_MAP_ENTRY(nsIInputStream)
>+    NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsIInputStream, nsIBufferedInputStream)
>     NS_INTERFACE_MAP_ENTRY(nsIBufferedInputStream)
>     NS_INTERFACE_MAP_ENTRY(nsIStreamBufferAccess)
>     NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIIPCSerializableInputStream, IsIPCSerializable())
>+    NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIAsyncInputStream, IsAsyncInputStream())
But then this is conditional. 
I guess the commit message should say that buffered stream must be able to read from async stream?

>     nsresult rv = NS_OK;
>     while (count > 0) {
>         uint32_t amt = std::min(count, mFillPoint - mCursor);
>         if (amt > 0) {
>             uint32_t read = 0;
>-            rv = writer(this, closure, mBuffer + mCursor, *result, amt, &read);
>+            nsCOMPtr<nsIBufferedInputStream> bufferedInputStream = this;
>+            rv = writer(bufferedInputStream, closure, mBuffer + mCursor, *result, amt, &read);
Why this change? Is this kungfuDeathGrip type of thing? Then the right solution would be to ensure the callee 
is keeping 'this' alive.
Or is this about casting 'this' to nsIBufferedInputStream? nsCOMPtr shouldn't be needed for that.
static_cast works just fine and is faster.

>+NS_IMETHODIMP
>+nsBufferedInputStream::OnInputStreamReady(nsIAsyncInputStream* aStream)
>+{
>+    // We have been canceled in the meanwhile.
>+    if (!mAsyncWaitCallback) {
>+        return NS_OK;
>+    }
>+
>+    nsCOMPtr<nsIInputStreamCallback> callback;
>+    callback.swap(mAsyncWaitCallback);
Do we clear mAsyncWaitCallback also when Close is called? Pretty sure we should, since don't we otherwise leak easily.

Comment 8

[Andrea Marchesini [:baku]]

Attachment: sw2.patch

Comment 9

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

Comment on attachment 8863787 [details] [diff] [review]
sw2.patch

>         if (amt > 0) {
>             uint32_t read = 0;
>-            rv = writer(this, closure, mBuffer + mCursor, *result, amt, &read);
>+            nsCOMPtr<nsIBufferedInputStream> bufferedInputStream = this;
You don't use this variable at all


We must get some testcase for this bug to prevent regressions. All these regressions hint that we have way too few tests for all the
code using IPCBlob/PBlob.

But feel free to land the regression fix asap.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f59f4ccdd75
nsBufferedInputStream must be able to read from nsIAsyncInputStream, r=smaug

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2f59f4ccdd75