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

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Jorge Luis]

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"

Comment 1

[Loic]

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.

Comment 2

[Alice0775 White]

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

Comment 3

[Julian Reschke]

Firefox works as intended. Please contact the site to change their code not to special-case Firefox.

Comment 4

[Julian Reschke]

(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>

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

(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.

Comment 6

[Julian Reschke]

(In reply to [e-mail] [: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

[Alice0775 White]

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

Comment 8

[Julian Reschke]

(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.

Comment 9

[Julian Reschke]

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)

Comment 10

[:Gavin Sharp [email: [e-mail]]]

(In reply to [e-mail] [: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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

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.

Comment 12

[Julian Reschke]

(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.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> We already are.

Well, except for the "not release" part?

Comment 14

[Julian Reschke]

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

Indeed. :-)

Comment 15

[Alex Keybl [:akeybl]]

(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

Comment 16

[Alex Keybl [:akeybl]]

Forgot to find out "who wants this bug"

Comment 17

[Julian Reschke]

(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).

Comment 18

[Jason Duell]

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.

Comment 19

[Julian Reschke]

(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.

Comment 20

[Patrick McManus [:mcmanus]]

(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..)

Comment 21

[Julian Reschke]

Attachment: patch re-enabling RFC 2047 support (but not adjusting test cases yet)

Comment 22

[Patrick McManus [:mcmanus]]

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.)

Comment 23

[Julian Reschke]

(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.)

Comment 24

[Patrick McManus [:mcmanus]]

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).

Comment 25

[:Gavin Sharp [email: [e-mail]]]

(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.

Comment 26

[Julian Reschke]

(In reply to :Gavin Sharp (use [e-mail] 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).

Comment 27

[Julian Reschke]

Attachment: patch re-enabling RFC 2047 support

I haven't looked at how to make this depend on the branch yet.

Comment 28

[Jason Duell]

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.

Comment 29

[Jason Duell]

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.

Comment 30

[Jason Duell]

https://hg.mozilla.org/integration/mozilla-inbound/rev/908ee156f377

Comment 31

[Jason Duell]

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

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/908ee156f377

Comment 33

[Kohei Yoshino [:kohei]]

Updated the compat document.
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_22