1246375 - Cannot tweet with an attached photo.

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Mihai Barbat]

Since a while one cannot tweet a message with a photo attached from the gallery. The tweet is published, but the photo is not displayed. I tried with today's 20160204030239 OTA for the Flame and I have the issue. 

Someone else tried with a Sony device with a build from the 29-th of January and he also has the issue.

I also tried with my Sony running OTA 20151230201227 and I don't have the issue.

So sometime between 20151230201227 and 20160129 this problem appeared.

Comment 1

[Alexandre LISSY :gerard-majax]

I also get this from the Twitter app. Maybe we should try: from browser with faking user agent?

Comment 2

[Alexandre LISSY :gerard-majax]

I am not sure this is a Gecko or B2G issue itself, because all the other codepath of sharing usecase are working.

Comment 3

[Alexandre LISSY :gerard-majax]

I have been able to reproduce this with Firefox for Android nightlies.

Comment 4

[Alexandre LISSY :gerard-majax]

This is a Gecko regression: Firefox for Android stable (Gecko 44) is able to post a picture attached to a tweet. Firefox for Android nightly (Gecko 47) is not.

Comment 5

[Alexandre LISSY :gerard-majax]

Also reproduced with a Firefox Desktop (nightly) and connecting to Twitter's mobile website.

Comment 6

[Alexandre LISSY :gerard-majax]

mozregression gave:
> Was this inbound build good, bad, or broken? (type 'good', 'bad', 'skip', 'retry', 'back' or 'exit' and press Enter): bad
> 12:24.83 INFO: Narrowed inbound regression window from [4bdcdd43, c474f2b7] (3 revisions) to [4bdcdd43, 9fa27883] (2 revisions) (~1 steps left)
> 12:24.83 INFO: Oh noes, no (more) inbound revisions :(
> 12:24.83 INFO: Last good revision: 4bdcdd43b11d461b580e1cbe46b5eedd8513b733
> 12:24.83 INFO: First bad revision: 9fa27883caa1e0d5b4565b4ed40c653388e9aa19
> 12:24.83 INFO: Pushlog:
> https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4bdcdd43b11d461b580e1cbe46b5eedd8513b733&tochange=9fa27883caa1e0d5b4565b4ed40c653388e9aa19
> 
> 12:25.67 INFO: Looks like the following bug has the changes which introduced the regression:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1241171

Comment 7

[Alexandre LISSY :gerard-majax]

Bug 1247538 might be a duplicate

Comment 8

[Alexandre LISSY :gerard-majax]

Would this be a website-side issue?

Comment 9

[Sylvestre Ledru [:Sylvestre]]

[Tracking Requested - why for this release]:
As we uplifted bug 1241171 into aurora, this could also affect 46.
Liz, ni to make sure you see that :)

Comment 10

[Andrea Marchesini [:baku]]

We lately changed the FormData (xhr) spec in order to remove the 'blob' as filename.
So, I would say that it's an issue on the website-side. It's also true that we cannot break big websites.
annevk?

Comment 11

[ccd0]

(In reply to Andrea Marchesini (:baku) from comment #10)
> We lately changed the FormData (xhr) spec in order to remove the 'blob' as
> filename.
> So, I would say that it's an issue on the website-side. It's also true that
> we cannot break big websites.
> annevk?

Regarding that spec change, which was motivated by:
https://github.com/whatwg/xhr/issues/48
> a HTMLInputElement type=file, if not set, contains a blob without name.

Are you sure that's what it should do? A quick fix would be to revert the "blob" filename spec change and have the entry for an unset file input be a File object with an empty filename, empty body, and type "application/octet-stream".

I tried looking at https://html.spec.whatwg.org/multipage/forms.html#constructing-form-data-set to see what the spec says to do, but it seems to be miswritten at this point:

> Otherwise, if the field element is an input element whose type attribute is
> in the File Upload state, then for each file selected in the input element,
> append an entry to the form data set with the name as the name, the file
> (consisting of the name, the type, and the body) as the value, and type as
> the type. If there are no selected files, then append an entry to the form
> data set with the name as the name, the empty string as the value, and
> application/octet-stream as the type.

That says the type of the entry is "application/octet-stream", but it should be `type` ("the value of the type IDL attribute of field"), with "application/octet-stream" being the type of the value of the entry instead.

Comment 12

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Tracking since this look like a regression from 46.

Comment 13

[Andrea Marchesini [:baku]]

Attachment: form.patch

I still don't know if it's fully green on try.

Comment 14

[Olli Pettay [:smaug] [away July 4-8]]

Does the patch fix also bug 1247538?

Comment 15

[Olli Pettay [:smaug] [away July 4-8]]

Comment on attachment 8718921 [details] [diff] [review]
form.patch

>+  mPostDataChunk += NS_LITERAL_CSTRING("--") + mBoundary
>+                 + NS_LITERAL_CSTRING(CRLF);
>+  // XXX: name/filename parameter should be encoded per RFC 2231
>+  // RFC 2388 specifies that RFC 2047 be used, but I think it's not
>+  // consistent with the MIME standard.
>+  mPostDataChunk +=
>+         NS_LITERAL_CSTRING("Content-Disposition: form-data; name=\"")
>+       + nameStr + NS_LITERAL_CSTRING("\"; filename=\"\"" CRLF)
>+       + NS_LITERAL_CSTRING("Content-Type: application/octet-stream" CRLF CRLF);
Somewhat annoying that we have now this data chunk string construction in 3 different places. Easy to make mistakes.
But making some helper method for this could be done in a separate bug.


>+
>+  // CRLF after file
>+  mPostDataChunk.AppendLiteral(CRLF);
>+
and this too...


I think reusing AddNameBlobPair for this would make the code easier to maintain (and review).
Perhaps some internal method AddNameBlobOrNullPair
which then AddNameBlobPair and AddNameNullPair would call with different params.

...thinking if we should do that now or in a followup...
I guess now is better. It is a small change and leads to less code and less error prone code.

Comment 17

[Anne (:annevk)]

ccd0, I think you found an issue in HTML. Could you maybe mention that in https://github.com/whatwg/html/issues/476? Thank you.

I discussed this issue with Andrea. From that it seems there might be several issues:

* Getting a form data set: it's unclear what to do here with <input type=file> when the user has selected nothing. We used to do something nonsensical. An empty Blob might be okay, but if that becomes a File with "blob" as filename that would not match what we submitted to the server before (a File with "" as filename). So perhaps no files selected should simply result in a File object with a filename "".

* FormData append/set taking a Blob: we used to turn these into File objects with filename "blob". Now we just add Blob objects. This might be breaking Facebook. But see the last point.

* Serializing a form data set: Blob objects now end up on the server as files without filename, likely leading to existing code assuming no file was selected to begin with. So perhaps if we change this serialization to treat Blob objects as files with filename "blob" things will start working again.

Comment 18

[Olli Pettay [:smaug] [away July 4-8]]

But for now we need to get back to the old behavior. The not-web-compatible behavior is already in Aurora and need to be fixed, and we can't really test web-compatibilityness on Aurora.

Comment 19

[Andrea Marchesini [:baku]]

Attachment: form1.patch

With this patch we convert any blob in a file with 'blob' name if they are not file already or if the filename is not passed.

Comment 20

[Andrea Marchesini [:baku]]

Attachment: form1.patch

Comment 21

[Olli Pettay [:smaug] [away July 4-8]]

Comment on attachment 8719909 [details] [diff] [review]
form1.patch

Ok, so this is backing out large parts of bug 1162658.

>     is(response[0].headers['Content-Disposition'],
>-        'form-data; name="empty"; filename=""');
>+        'form-data; name="empty"; filename="blob"');
> 
>     is(response[1].headers['Content-Disposition'],
>         'form-data; name="explicit"; filename="explicit-file-name"');
> 
>     is(response[2].headers['Content-Disposition'],
>-        'form-data; name="explicit-empty"; filename=""');
>+        'form-data; name="explicit-empty"; filename="blob"');
This is different to behavior in beta, and passing empty string explicitly leading to empty file name makes sense.
And even per current spec (I think even the old spec) filename should be "" here.



>+++ b/dom/tests/mochitest/fetch/test_fetch_basic_http.js
...
>       is(response[3].headers['Content-Disposition'],
>-          'form-data; name="explicit-empty"; filename=""');
>+          'form-data; name="explicit-empty"; filename="blob"');
Same here

Comment 22

[Olli Pettay [:smaug] [away July 4-8]]

(I couldn't quite see from the patch what leads to the behavior those test changes tell about.)

Comment 23

[Andrea Marchesini [:baku]]

Attachment: form1.patch

Same patch. the 2 tests were broken. Fixing what you suggested, now they work.

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0715e0b9a316

Comment 25

[Andrea Marchesini [:baku]]

Comment on attachment 8720155 [details] [diff] [review]
form1.patch

Approval Request Comment
[Feature/regressing bug #]: bug 1241171 and bug 1162658
[User impact if declined]: broken websites
[Describe test coverage new/current, TreeHerder]: tests included
[Risks and why]: no big risks: we are reverting patches.
[String/UUID change made/needed]: none

Comment 26

[Olli Pettay [:smaug] [away July 4-8]]

Why beta? I thought the regression is Aurora/Nightly only.

Comment 27

[Alexandre LISSY :gerard-majax]

So I gave a chance to this patch locally, guess what, ... It's working :)

Comment 28

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8720155 [details] [diff] [review]
form1.patch

Manual testing, fixes a bad regression, includes test fixes too, let's take it in aurora.

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0715e0b9a316

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6d5cebf17a9e