The default bug view has changed. See this FAQ.

Revert to decoding RFC 2047-encoding until we have telemetry on usage

RESOLVED FIXED in Firefox 22

Status

Core Graveyard
File Handling
RESOLVED FIXED
4 years ago
9 months ago

People

(Reporter: Jorge Luis, Assigned: Julian Reschke)

Tracking

(Blocks: 2 bugs, {regression})

22 Branch
mozilla24
regression
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox21 unaffected, firefox22+ fixed, firefox23+ fixed, firefox24+ fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130521223249

Steps to reproduce:

Downloading a file from ZippyShare (e.g. http://www75.zippyshare.com/v/38540382/file.html), clicking on "save link as...", starting from Firefox 22b1 became in a base64 encoding filename instead of plain text. Firefox 21 works right. Using FlashGot/DownThemAll shows the right filename.




Actual results:

After download the file, its name is "=_UTF-8_B_VGl0YW5pdW0gQmFja3VwIFBybyB2Ni4wLjUgYXBrbWFuaWEuY29tLnJhcg==_=" (without quotes) instead of "Titanium Backup Pro v6.0.5 apkmania.com.rar". The server response is:

HTTP/1.1 200 OK
Server: Apache-Coyote/1.1
Content-Disposition: attachment; filename==?UTF-8?B?VGl0YW5pdW0gQmFja3VwIFBybyB2Ni4wLjUgYXBrbWFuaWEuY29tLnJhcg==?=
Content-Type: application/x-download;name=38540382.rar
Content-Length: 2866793
Date: Fri, 24 May 2013 02:31:14 GMT


Expected results:

The file name had to be "Titanium Backup Pro v6.0.5 apkmania.com.rar"
(Reporter)

Updated

4 years ago
Component: Untriaged → File Handling

Comment 1

4 years ago
Regression range:
m-c
good=2013-02-22
bad=2013-02-23
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=885cde564ff3&tochange=08a034e1d43a

Suspected bug: bug 828782 or bug 835719.
Status: UNCONFIRMED → NEW
status-firefox21: --- → unaffected
tracking-firefox22: --- → ?
tracking-firefox23: --- → ?
tracking-firefox24: --- → ?
Ever confirmed: true
Keywords: regression

Comment 2

4 years ago
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9df26f2a96fc&tochange=83535a72fb27

Triggered by:
83535a72fb27	Julian Reschke — Bug 601933: remove RFC 2047 encoding support for HTTP header field parameters. r=jduell

Updated

4 years ago
Blocks: 601933
(Assignee)

Comment 3

4 years ago
Firefox works as intended. Please contact the site to change their code not to special-case Firefox.
(Assignee)

Updated

4 years ago
OS: Windows 7 → All
Hardware: x86 → All
(Assignee)

Comment 4

4 years ago
(In reply to Julian Reschke from comment #3)
> Firefox works as intended. Please contact the site to change their code not
> to special-case Firefox.

-> <http://www.support.zippyshare.com/index.php?act=tickets&code=view&id=142164>
(In reply to Julian Reschke from comment #3)
> Firefox works as intended. Please contact the site to change their code not
> to special-case Firefox.

We haven't actually shipped this yet so let's talk about https://bugzilla.mozilla.org/show_bug.cgi?id=601933#c45 and how many sites this might be affecting - if this is going to become a larger evangelism issue to get sites understanding we are no longer doing out-of-spec decoding should we consider backing out the patch in bug 601933 and having a longer period of awareness raising and outreach?

Gavin, curious to know your thoughts on this.
Flags: needinfo?(gavin.sharp)
status-firefox22: --- → affected
status-firefox23: --- → affected
status-firefox24: --- → affected
tracking-firefox22: ? → +
tracking-firefox23: ? → +
tracking-firefox24: ? → +
(Assignee)

Comment 6

4 years ago
(In reply to lsblakk@mozilla.com [:lsblakk] from comment #5)
> (In reply to Julian Reschke from comment #3)
> > Firefox works as intended. Please contact the site to change their code not
> > to special-case Firefox.
> 
> We haven't actually shipped this yet so let's talk about
> https://bugzilla.mozilla.org/show_bug.cgi?id=601933#c45 and how many sites
> this might be affecting - if this is going to become a larger evangelism
> issue to get sites understanding we are no longer doing out-of-spec decoding
> should we consider backing out the patch in bug 601933 and having a longer
> period of awareness raising and outreach?

1) Unless we start to measure things it'll be hard to find out unless we try.

2) By any means do not back out the change; if we want to buy us time it's much better just to flip a switch in the source (the RFC 2047 decoding is still there for email and would need to be re-enabled, no reason to back out everything)

Comment 7

4 years ago
http://www18.zippyshare.com/v/16040066/file.html
also be affected. I confirmed same regression range.

Actual Results:
=_UTF-8_B_dW50aXRsZWQucmFy_=
Expected Results:
untitled.rar
(Assignee)

Comment 8

4 years ago
(In reply to Alice0775 White from comment #7)
> http://www18.zippyshare.com/v/16040066/file.html
> also be affected. I confirmed same regression range.
> 
> Actual Results:
> =_UTF-8_B_dW50aXRsZWQucmFy_=
> Expected Results:
> untitled.rar

Well, same server. Nothing new here.
(Assignee)

Comment 9

4 years ago
Got mail from the site admin that the problem has been fixed. I have verified that this is indeed the case.

(So I believe this bug can be closed)
(In reply to lsblakk@mozilla.com [:lsblakk] from comment #5)
> Gavin, curious to know your thoughts on this.

I have very little background on this, I don't have an intuitive sense for how likely this is to pose problems in practice once we ship. If we found one affected server in beta, it could very well blow up in release. Others (Julian, Bz, jduell) would be in a better place to comment on alternate approaches/mitigation strategies.
Flags: needinfo?(gavin.sharp)
The fundamental problem is that to evangelize we need to know who these people are.  Phoning home the url when we see this bogus encoding is not really an option we have, right?

We can try shipping this for a while on nightly+aurora but not release....

Or maybe even nightly+aurora+early betas but not release?

Short of the above options, we can either ship or never ship.
(Assignee)

Comment 12

4 years ago
(In reply to Boris Zbarsky (:bz) from comment #11)
> The fundamental problem is that to evangelize we need to know who these
> people are.  Phoning home the url when we see this bogus encoding is not
> really an option we have, right?
> 
> We can try shipping this for a while on nightly+aurora but not release....
> 
> Or maybe even nightly+aurora+early betas but not release?
> ...

We already are. So far we got one bug report; and that server is fixed. I'm pretty sure there are more out there, but I have no idea how many.

See related bug http://code.google.com/p/chromium/issues/detail?id=66694 -- I believe the Chromium people are planning to get statistics.
> We already are.

Well, except for the "not release" part?
(Assignee)

Comment 14

4 years ago
(In reply to Boris Zbarsky (:bz) from comment #13)
> > We already are.
> 
> Well, except for the "not release" part?

Indeed. :-)
(In reply to Boris Zbarsky (:bz) from comment #11)
> The fundamental problem is that to evangelize we need to know who these
> people are.  Phoning home the url when we see this bogus encoding is not
> really an option we have, right?
> 
> We can try shipping this for a while on nightly+aurora but not release....
> 
> Or maybe even nightly+aurora+early betas but not release?
> 
> Short of the above options, we can either ship or never ship.

I'd tend towards nightly+aurora if there's any chance that disabling upon beta->release could cause critical regressions. 

https://wiki.mozilla.org/Platform/Channel-specific_build_defines \o/ Gavin
Forgot to find out "who wants this bug"
Flags: needinfo?(julian.reschke)
Flags: needinfo?(jduell.mcbugs)
(Assignee)

Comment 17

4 years ago
(In reply to Alex Keybl [:akeybl] from comment #15)

> I'd tend towards nightly+aurora if there's any chance that disabling upon
> beta->release could cause critical regressions. 

I don't think there's any risk in disabling. It's a simple change in an "if" statement (if we keep the code we currently have in beta).
Flags: needinfo?(julian.reschke)
So it seems like we've got 2 options here:

1) Just release the code and let the apparently small number of sites that are broken figure it out and fix it.  The argument here is that having an ugly name for your default "save as" filename is not really a "critical regression" [1], and while this breaks a few eggs it's the fastest way to make the Internet a better omelet.

2) disable the change on nightly, and wait for the Chrome team to gather some stats (and/or get them ourselves), then make the call as to how bad this is.

I'm not sure who gets to make the call here. (if we consider this necko, where the code lives, it's :mcmanus.  One could make the argument it's up to the Firefox module owner).  I personally lean toward #1.  Maybe I'm being cavalier, but from my perspective it's too many engineering-hours to babysit this fix when we expect the impacted site count to be small, and the consequences minor even when it happens.

Julian: do we know what browsers right now are still supporting RFC 2047 for this?  It sounds like it's just us and Chrome.   Would you be willing to write the patch that changes the "if" so we've got it if we want to proceed with #2?  Thanks.

[1] For instance, until a year or so ago if it took too long to get a response for a "Save as" request, we used to give the file a default name based on the URI, and it was messy.  It happened fairly often and complaints were minimal.  This probably happened 10x+ more often than this issue will.
Assignee: nobody → julian.reschke
Flags: needinfo?(jduell.mcbugs)
(Assignee)

Comment 19

4 years ago
(In reply to Jason Duell (:jduell) from comment #18)
> So it seems like we've got 2 options here:
> 
> 1) Just release the code and let the apparently small number of sites that
> are broken figure it out and fix it.  The argument here is that having an
> ugly name for your default "save as" filename is not really a "critical
> regression" [1], and while this breaks a few eggs it's the fastest way to
> make the Internet a better omelet.

That would be great.

> 2) disable the change on nightly, and wait for the Chrome team to gather
> some stats (and/or get them ourselves), then make the call as to how bad
> this is.

That would help, but not entirely. Sites send different header fields based on the User-Agent, so we wouldn't be able to rely on Chrome's statistics. In particular, Chrome hasn't supported RFC 2231 (the "correct" way of doing things) from the beginning, while Firefox essentially has.

> I'm not sure who gets to make the call here. (if we consider this necko,
> where the code lives, it's :mcmanus.  One could make the argument it's up to
> the Firefox module owner).  I personally lean toward #1.  Maybe I'm being
> cavalier, but from my perspective it's too many engineering-hours to babysit
> this fix when we expect the impacted site count to be small, and the
> consequences minor even when it happens.
> 
> Julian: do we know what browsers right now are still supporting RFC 2047 for
> this?  It sounds like it's just us and Chrome.   Would you be willing to
> write the patch that changes the "if" so we've got it if we want to proceed
> with #2?  Thanks.

Yes, I can do that.

> [1] For instance, until a year or so ago if it took too long to get a
> response for a "Save as" request, we used to give the file a default name
> based on the URI, and it was messy.  It happened fairly often and complaints
> were minimal.  This probably happened 10x+ more often than this issue will.
(In reply to Jason Duell (:jduell) from comment #18)

> I'm not sure who gets to make the call here. (if we consider this necko,
> where the code lives, it's :mcmanus.  One could make the argument it's up to
> the Firefox module owner). 

I nominate lukas and gavin who are going to have a better feel for the tradeoffs involved. If I were to make the call I would want to hear a concise argument for the absorbing breakage (e.g. what does it enable, what problematic use does it eliminate, etc..)
(Assignee)

Comment 21

4 years ago
Created attachment 759152 [details] [diff] [review]
patch re-enabling RFC 2047 support (but not adjusting test cases yet)
Julian if you make the 2 behaviors preffable we have directives that can choose different prefs based on the channel.

(they can actually do conditional compiles now too - but I think that feature is to recent to support any kind of backport. not sure though.)
(Assignee)

Comment 23

4 years ago
(In reply to Patrick McManus [:mcmanus] from comment #22)
> Julian if you make the 2 behaviors preffable we have directives that can
> choose different prefs based on the channel.

Can you suggest a good sample code for something preffable that I could steal?

> (they can actually do conditional compiles now too - but I think that
> feature is to recent to support any kind of backport. not sure though.)
https://wiki.mozilla.org/Platform/Channel-specific_build_defines

i believe the definition there is accurate for FF24 (the various defines and the fact that they control c++ as well as pref files). I'm not up to speed on how accurate that is for FF23 and FF22. I'm sure that RELEASE_BUILD does apply to at least prefs on all those branches. (its the one that got the party started).
(In reply to Patrick McManus [:mcmanus] from comment #24)
> https://wiki.mozilla.org/Platform/Channel-specific_build_defines
> 
> i believe the definition there is accurate for FF24 (the various defines and
> the fact that they control c++ as well as pref files). I'm not up to speed
> on how accurate that is for FF23 and FF22.

It's accurate for Firefox 23 and later (I added some notes).

I'm open to considering option 1 given jduell's arguments in 18, but I haven't seen a great case made for the benefits we're getting in exchange for the compatibility cost. Bug 601933 comment 0 and bug 601933 comment 9 suggest some code size reduction (is this still true?) and adherence to standards, but traded off against user frustration those seem like relatively weak reasons.
(Assignee)

Comment 26

4 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #25)
> (In reply to Patrick McManus [:mcmanus] from comment #24)
> > https://wiki.mozilla.org/Platform/Channel-specific_build_defines
> > 
> > i believe the definition there is accurate for FF24 (the various defines and
> > the fact that they control c++ as well as pref files). I'm not up to speed
> > on how accurate that is for FF23 and FF22.
> 
> It's accurate for Firefox 23 and later (I added some notes).
> 
> I'm open to considering option 1 given jduell's arguments in 18, but I
> haven't seen a great case made for the benefits we're getting in exchange
> for the compatibility cost. Bug 601933 comment 0 and bug 601933 comment 9
> suggest some code size reduction (is this still true?) and adherence to
> standards, but traded off against user frustration those seem like
> relatively weak reasons.

Well, handling of I18N in Content-Disposition is a giant mess. Look at the complexity of the code and the heuristics applied.

A few years ago I started work on getting all browsers to implement what was specified (RFC 2231), and was successful in that nowadays, servers can send the same field value to every current browser.

What's left to do (and admittingly is of less importance) is to get rid of all the cruft; as long as it's there, servers are going to continue to use it (because it "works"), thus more code using UA-specific switches will continue to be created.

Also, as this code is used for other stuff (such as @type in HTML), the "support" for RFC 2047 bleeds into places where it really really does not belong.

Regarding the code reduction: we will be able to get rid of the RFC-2047-related code later on once it's not needed anymore by the mail client (see bug 858337).
(Assignee)

Comment 27

4 years ago
Created attachment 759726 [details] [diff] [review]
patch re-enabling RFC 2047 support

I haven't looked at how to make this depend on the branch yet.
Attachment #759152 - Attachment is obsolete: true
Attachment #759726 - Attachment is patch: true
On IRC :gavin :akeybl and I made the call to re-enable RFC 2047 decoding (i.e. disable the effect of bug 601933, thought we're not backing the actual code out) until we've got some telemetry that indicates how common it is in the wild.
Summary: File is saved with base64 name instead of UTF-8 → Revert to decoding RFC 2047-encoding until we have telemetry on usage
Comment on attachment 759726 [details] [diff] [review]
patch re-enabling RFC 2047 support

Review of attachment 759726 [details] [diff] [review]:
-----------------------------------------------------------------

Code to disable looks easy and safe.

We want this on aurora/beta/inbound since we're not going to re-enable this until we have data and I don't want to have to remember to re-disable it on beta at each release.
Attachment #759726 - Flags: review+
Attachment #759726 - Flags: approval-mozilla-beta?
Attachment #759726 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/integration/mozilla-inbound/rev/908ee156f377

Updated

4 years ago
Attachment #759726 - Flags: approval-mozilla-beta?
Attachment #759726 - Flags: approval-mozilla-beta+
Attachment #759726 - Flags: approval-mozilla-aurora?
Attachment #759726 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/b26003b34809
https://hg.mozilla.org/releases/mozilla-beta/rev/f4843c5acbdc

Filed bug 883447 - Add telemetry for how often RFC 2047 usage occurs in Firefox
status-firefox22: affected → fixed
status-firefox23: affected → fixed
Component: File Handling → File Handling
Product: Firefox → Core
Blocks: 883447
https://hg.mozilla.org/mozilla-central/rev/908ee156f377
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox24: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Updated the compat document.
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_22
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.