User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.8) Gecko/20100722 Firefox/3.6.8 (.NET CLR 3.5.30729) Build Identifier: 3.6.8 In order to deploy RFC2231/5987 encoding, there may be cases where servers want to send both. The format allows this, and optimally UAs would ignore parameters they do not understand, and prefer "filename*" over "filename" when both are present and understood. Unfortunately, FF takes the first one it finds, not the "better" one. Reproducible: Always Steps to Reproduce: 1. See http://greenbytes.de/tech/tc2231/#attfnboth 2. Run the test http://greenbytes.de/tech/tc2231/attfnboth.asis Actual Results: Suggests "foo-ae.html" as filename. Expected Results: Should suggest "foo-ä.html" as filename. Of the UAs that support RFC 2231/5987 (FF/Opera/Konqueror), currently only Konqueror gets this right.

This patch makes the InternalGetParameter continue after finding a matching parameter, looking for an instance of a RFC 2231 encoded parameter.

Comment on attachment 472622 [details] [diff] [review] proposed patch Looks pretty good, but please fix the two issues below, and add the test file to the patch. +++ b/netwerk/mime/nsMIMEHeaderParamImpl.cpp Tue Sep 07 15:09:22 2010 +0100 *aResult = ToNewCString(tempStr); - NS_ENSURE_TRUE(*aResult, NS_ERROR_OUT_OF_MEMORY); You should keep that check - on OOM we do want to return failure. - NS_ASSERTION(!*aResult, "This is the 1st line. result buffer should be null."); + if (!*aResult) + { + // drop non-2231-encoded value + nsMemory::Free(*aResult); This doesn't look right - don't you want the if to be if (*aResult)? (at some point this entire file should be changed to use NS_Alloc, NS_Free etc instead of nsMemory::... but not in this patch)

This patch makes the InternalGetParameter continue after finding a matching parameter, looking for an instance of a RFC 2231/5987 encoded parameter.

(In reply to comment #0) > Of the UAs that support RFC 2231/5987 (FF/Opera/Konqueror), currently only > Konqueror gets this right. This has been fixed with the RC of Opera 10.63, so now FF is the only UA to get this wrong.

biesi, can you comment with a risk evaluation? Should we take it for 2.0 at this stage? The fix is straightforward enough, but I'm not sure exactly what its potential compatibility effects are (e.g. what its potential is for causing regressions due to different parsing of other parameters).

(In reply to comment #5) > This has been fixed with the RC of Opera 10.63, so now FF is the only UA to get > this wrong. ...and Opera 10.63 is now officially shipping.

(In reply to comment #6) > biesi, can you comment with a risk evaluation? Should we take it for 2.0 at > this stage? The fix is straightforward enough, but I'm not sure exactly what > its potential compatibility effects are (e.g. what its potential is for causing > regressions due to different parsing of other parameters). Clarifying: the patch doesn't really affect parsing, but the interpretation of the parsing results. Unless there's something wrong with it, it will only change the behavior for header field instances that contain *both* the standard form and the RFC2231/5987 form of a parameter. The only reason I can think of why this would occur in practice is in order to provide a fallback for recipients that do not understand the 2231/5987 encoding, in which case the proposed change will do exactly what the sender intended.

cc-ing bz in case he has an opinion on this. I agree that logically speaking, we should be fine if we always prefer "paramname*" over an initial "paramname". Of course that's no guarantee that it couldn't actually cause problems. I just ran this patch through tryserver (hmm, doesn't seem to be any way to link to a specific tbpl entry. meh). There's lots of orange, much of it clearly random, but some of I'm not clear about (doesn't seem obviously related to this patch, but tpbl isn't finding any bugzilla bugs for them). For example: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1287155695.1287156258.23748.gz

Generally speaking, taking this for 2.0 seems ok to me... but I can also see us just holding off until after branch at this point. We need to get the actual blocker list down. :(

This problem blocks deployment of "filename*" with "filename" as fallback. As long as it's not fixed, senders have no choice but doing UA sniffing, ot to wait until IE/Chrome/Safari are fixed (which need the fallback).

JST: how about we mark approved for 2.0, try to land this, and if there's nontrivial problems we mark post 2.0?

This seems risky enough that I don't want to take it for FF4 now.

(In reply to comment #13) > This seems risky enough that I don't want to take it for FF4 now. Trying to understand where you see the risk... The patch is expected to change the behavior if and only if a header field contains both "foo" and "foo*", in which case it will pick the latter. Do you think *that* change may have undesired consequences? Or do you fear that there are more behavioral changes? That would indeed be a bug, and I would appreciate some hints about what to look for.

Comment on attachment 472645 [details] [diff] [review] priorize "param*" (RFC 2231/5987) over "param" > Trying to understand where you see the risk... It's nothing specific, just that we're late in the release cycle, and have to make hard choices about not landing new stuff. I'd personally like to see this in 2.0/FF 4, but ultimately it's up the drivers. >diff -r e14130b81b5f netwerk/mime/nsMIMEHeaderParamImpl.cpp > // case B, C, and D >- else if (tokenEnd - tokenStart > paramLen && >+ if (tokenEnd - tokenStart > paramLen && I think you should keep the 'else if' here. The enclosing while loop will already ensure you'll check for the next parameter. Changing this to 'if' just means we'll do a useless check for cases B,C,D when we already know that the param under consideration is type A.

Also please get rid of comment above function: // The format of these header lines is // <token> [ ';' <token> '=' <token-or-quoted-string> ]* which is inaccurate. RFC 2231 comment later on suffices.

(In reply to comment #16) > Also please get rid of comment above function: > > // The format of these header lines is > // <token> [ ';' <token> '=' <token-or-quoted-string> ]* > > which is inaccurate. RFC 2231 comment later on suffices. Can't see what's not accurate here...

Fixed up patch: moving +r/+sr forward. Followup bug and patch in bug 610054. Julian/mnot: I'd appreciate it if either of you could look over the test cases her and in bug 610054 to see if they're all 2231/5987 compliant.

Julian is the master of the test cases. I'd urge Moz to consider getting this into FF4, because this bug is the one significant barrier to a good interop / i18n story for Content-Disposition.

This is NOT going into Fx4. See comment 13.

I'll drive this for our next release, which will be out 3 months after Firefox 4.

Yes, this should just land when the tree opens post-FF4. I doubt it's going to cause any trouble.

Tested <http://greenbytes.de/tech/tc2231/#attfnboth> with nightly build of 2011-03-25 and works fine. Thanks!

We have a mention of this on Firefox 5 for developers now; there is no documentation for content-disposition, which is a separate issue entirely; see bug 650562.

I just noticed that this caused an unplanned change in behavior for certain malformed headers, such as when there are multiple "filename" parameters. See http://greenbytes.de/tech/tc2231/#attwith2filenames (FF4 picks the first one, FF5aurora picks the second one) I do not believe this should cause problems in practice, as this is a test case for an edge case, not a header seen in practice. My suggestion is to leave the fix in, but to be prepared to tune it (should this be raised as separate issue?).

Yes, open a new bug if there's a refinement to be made to a patch that's already landed and RESOLVED FIXED.