Closed Bug 1246375 Opened 7 years ago Closed 7 years ago

Cannot tweet with an attached photo.


(Core :: General, defect)

Gonk (Firefox OS)
Not set



Tracking Status
firefox46 + fixed
firefox47 + fixed


(Reporter: mihaibn, Assigned: baku)



(Keywords: foxfood, regression)


(1 file, 3 obsolete files)

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.
Keywords: foxfood
I also get this from the Twitter app. Maybe we should try: from browser with faking user agent?
Component: Gaia::Browser → General
I am not sure this is a Gecko or B2G issue itself, because all the other codepath of sharing usecase are working.
I have been able to reproduce this with Firefox for Android nightlies.
Product: Firefox OS → Core
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.
Also reproduced with a Firefox Desktop (nightly) and connecting to Twitter's mobile website.
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:
> 12:25.67 INFO: Looks like the following bug has the changes which introduced the regression:
Bug 1247538 might be a duplicate
Would this be a website-side issue?
Flags: needinfo?(amarchesini)
[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 :)
Flags: needinfo?(lhenry)
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.
Flags: needinfo?(amarchesini) → needinfo?(annevk)
(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:
> 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 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.
Tracking since this look like a regression from 46.
Flags: needinfo?(lhenry)
Assignee: nobody → amarchesini
Attached patch form.patch (obsolete) — Splinter Review
I still don't know if it's fully green on try.
Attachment #8718921 - Flags: review?(bugs)
Does the patch fix also bug 1247538?
Comment on attachment 8718921 [details] [diff] [review]

>+  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.
Attachment #8718921 - Flags: review?(bugs) → review-
Duplicate of this bug: 1248150
ccd0, I think you found an issue in HTML. Could you maybe mention that in 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.
Flags: needinfo?(annevk)
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.
Attached patch form1.patch (obsolete) — Splinter Review
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.
Attachment #8718921 - Attachment is obsolete: true
Attachment #8719785 - Flags: review?(bugs)
Attached patch form1.patch (obsolete) — Splinter Review
Attachment #8719785 - Attachment is obsolete: true
Attachment #8719785 - Flags: review?(bugs)
Attachment #8719909 - Flags: review?(bugs)
Comment on attachment 8719909 [details] [diff] [review]

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
Attachment #8719909 - Flags: review?(bugs) → review-
(I couldn't quite see from the patch what leads to the behavior those test changes tell about.)
Attached patch form1.patchSplinter Review
Same patch. the 2 tests were broken. Fixing what you suggested, now they work.
Attachment #8719909 - Attachment is obsolete: true
Attachment #8720155 - Flags: review?(bugs)
Attachment #8720155 - Flags: review?(bugs) → review+
Comment on attachment 8720155 [details] [diff] [review]

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
Attachment #8720155 - Flags: approval-mozilla-beta?
Attachment #8720155 - Flags: approval-mozilla-aurora?
Why beta? I thought the regression is Aurora/Nightly only.
Attachment #8720155 - Flags: approval-mozilla-beta?
So I gave a chance to this patch locally, guess what, ... It's working :)
Comment on attachment 8720155 [details] [diff] [review]

Manual testing, fixes a bad regression, includes test fixes too, let's take it in aurora.
Attachment #8720155 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.