Closed
Bug 1246375
Opened 7 years ago
Closed 7 years ago
Cannot tweet with an attached photo.
Categories
(Core :: General, defect)
Tracking
()
RESOLVED
FIXED
mozilla47
People
(Reporter: mihaibn, Assigned: baku)
References
Details
(Keywords: foxfood, regression)
Attachments
(1 file, 3 obsolete files)
12.80 KB,
patch
|
smaug
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•7 years ago
|
||
I also get this from the Twitter app. Maybe we should try: from browser with faking user agent?
Component: Gaia::Browser → General
Comment 2•7 years ago
|
||
I am not sure this is a Gecko or B2G issue itself, because all the other codepath of sharing usecase are working.
Comment 3•7 years ago
|
||
I have been able to reproduce this with Firefox for Android nightlies.
Product: Firefox OS → Core
Comment 4•7 years ago
|
||
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•7 years ago
|
||
Also reproduced with a Firefox Desktop (nightly) and connecting to Twitter's mobile website.
Comment 6•7 years ago
|
||
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
Updated•7 years ago
|
Keywords: regression
Comment 7•7 years ago
|
||
Bug 1247538 might be a duplicate
Comment 9•7 years ago
|
||
[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 :)
Assignee | ||
Comment 10•7 years ago
|
||
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?
Flags: needinfo?(amarchesini) → needinfo?(annevk)
Comment 11•7 years ago
|
||
(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.
Tracking since this look like a regression from 46.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → amarchesini
Assignee | ||
Comment 13•7 years ago
|
||
I still don't know if it's fully green on try.
Attachment #8718921 -
Flags: review?(bugs)
Comment 14•7 years ago
|
||
Does the patch fix also bug 1247538?
Comment 15•7 years ago
|
||
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.
Attachment #8718921 -
Flags: review?(bugs) → review-
Comment 17•7 years ago
|
||
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.
Flags: needinfo?(annevk)
Comment 18•7 years ago
|
||
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.
Assignee | ||
Comment 19•7 years ago
|
||
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)
Assignee | ||
Comment 20•7 years ago
|
||
Attachment #8719785 -
Attachment is obsolete: true
Attachment #8719785 -
Flags: review?(bugs)
Attachment #8719909 -
Flags: review?(bugs)
Comment 21•7 years ago
|
||
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
Attachment #8719909 -
Flags: review?(bugs) → review-
Comment 22•7 years ago
|
||
(I couldn't quite see from the patch what leads to the behavior those test changes tell about.)
Assignee | ||
Comment 23•7 years ago
|
||
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)
Updated•7 years ago
|
Attachment #8720155 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 25•7 years ago
|
||
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
Attachment #8720155 -
Flags: approval-mozilla-beta?
Attachment #8720155 -
Flags: approval-mozilla-aurora?
Comment 26•7 years ago
|
||
Why beta? I thought the regression is Aurora/Nightly only.
Assignee | ||
Updated•7 years ago
|
Attachment #8720155 -
Flags: approval-mozilla-beta?
Comment 27•7 years ago
|
||
So I gave a chance to this patch locally, guess what, ... It's working :)
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.
Attachment #8720155 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 29•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0715e0b9a316
Status: NEW → RESOLVED
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.
Description
•