922322 - readAsText method in FileReader neither fails or succeeds if called with invalid encoding parameter

Status: NEW

(Core :: DOM: Core & HTML, defect)

Description

[alexei.eleusis]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/29.0.1547.76 Safari/537.36

Steps to reproduce:

I am using a library that has this function:

		function getData(callback, onerror) {
			var reader = new FileReader();
			reader.onload = function(e) {
				callback(e.target.result);
			};
			reader.onerror = onerror;
			reader.readAsText(blob, encoding);
		}

For reasons I don't fully understand, sometimes the encoding parameter is undefined.


Actual results:

When the encoding parameter is undefined neither the load nor onerror callbacks are executed. I solved it with the code below, but I think FireFox can do better:

		function getData(callback, onerror) {
			var reader = new FileReader();
			reader.onload = function(e) {
				callback(e.target.result);
			};
			reader.onerror = onerror;
			if (encoding) {
				reader.readAsText(blob, encoding);
			} else {
				reader.readAsText(blob);
			}
		}



Expected results:

At least one of the two callbacks should have executed.

Comment 1

[Boris Zbarsky [:bzbarsky]]

So the IDL for FileReader has:

  void readAsText(Blob blob, optional DOMString label = "");

Per spec, this meant (until the recent WebIDL changes that bug 882541 tracks) that passing undefined as the second argument would be equivalent to passing "undefined".

Then we end up in nsDOMFileReader::GetAsText, there is no encoding for the label "undefined", and we return an error back to DoOnStopRequest.  This itself then returns the error on to FileIOObject::OnStopRequest, which does NS_ENSURE_SUCCESS(rv, rv).

That last looks wrong; I'd think we should instead set aStatus = rv if rv failed and fall through to firing an error event...  Kyle?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Looks like a regression from bug 657994.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Simple testcase

Comment 4

[Boris Zbarsky [:bzbarsky]]

I should note that in Chrome passing in an invalid encoding label seems to succeed with the read, which seems very broken, unless I'm missing something.

Comment 5

[Masatoshi Kimura [:emk]]

(In reply to Boris Zbarsky [:bz] from comment #1)
>   void readAsText(Blob blob, optional DOMString label = "");

Why "label" has a default value of empty string? It looks bogus. Empty string is not a valid encoding label. The spec has no default value.
http://dev.w3.org/2006/webapi/FileAPI/#FileReader-interface
Only to avoid .WasPassed() mess? It's unfortunate we tend to add bogus default values for the convenience of the implementation...

(In reply to Boris Zbarsky [:bz] from comment #4)
> I should note that in Chrome passing in an invalid encoding label seems to
> succeed with the read, which seems very broken, unless I'm missing something.

Actually readAsText() will never fail regardless of the "label" value, per spec. (That is, Chrome is correct.)
http://dev.w3.org/2006/webapi/FileAPI/#encoding-determination

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Why "label" has a default value of empty string?

Dunno.  I didn't write this code.

That said, per spec "not passed" is identical to passing an invalid encoding label, looks like, so using = "" is fine in theory.

You're right that the spec says to not error out on invalid encodings; I'm not sure why my reading suggested it did.  :(

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

So did we figure out what the correct behavior is here?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Yes.  Need to treat invalid encoding labels the same was as "", as far as I can tell.