525736 - inputElement.files.length of (input type="file" multiple> is not restored correctly after session restore

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Alice0775 White]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2b2pre) Gecko/20091031 Firefox/3.5.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2b2pre) Gecko/20091031 Firefox/3.5.1

inputElement.files.length of (input type="file" multiple>  is not restored correctly after session restore.

This happens on branch build only.

Reproducible: Always

Steps to Reproduce:
1.Start Namoroka with new profile.
2.Make sure browser.startup.page;3
3.Open Test Case.
4.Select Multiple files(more than 2) by clicking "Brouse..." button.
5.Click "Click"button.
6.You can confirm number of selected files.
    Number of files = (number of you had selected).

7.Restart Namoroka with session restore.
8.Click "Click"button again.

Actual Results:  
Number of files = 1.


Expected Results:  
Number of files = (number of you had selected).


This problem does not occur in the following trunk build.
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.3a1pre) Gecko/20091031 Minefield/3.7a1pre ID:20091031042521

Comment 1

[Alice0775 White]

Attachment: Test Case

Comment 2

[Alice0775 White]

Attachment: Test Case

Comment 3

[Alice0775 White]

Please open the test case after a download.

Comment 4

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

I looked into this briefly. For some reason, |node.mozGetFileNameArray({})| is returning an object like {"0": filename1, "1": filename2} instead of [filename1, filename2]. Only on 1.9.2.

We can work around this pretty easily, but I'd like to know WHY that's happening.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh, wow, that's really bad.

cc'ing some people that might know why our array-returning code is broken on branch. Seems strange that we're not hitting this elsewhere.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I talked this over with jst and he said that we might actually be returning the same type of {0: "name1", 1: "name2"} objects on trunk too. It might not neccesarily be wrong to do so. Maybe.

One important question is does this object have a "length" property?

Would be great to know where we behave different between trunk and branch here.

Comment 7

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Adding some dumps right after this is set http://hg.mozilla.org/mozilla-central/file/3c230b561fa2/browser/components/sessionstore/src/nsSessionStore.js#l1571

dump(data[id].fileList.length);
dump(data[id].fileList);
dump(JSON.stringify(data[id].fileList));

has the following output (running the sessionstore test with multiple files):

mozilla-central:
2
/dev/null,/dev/stdin
["/dev/null","/dev/stdin"]

mozilla-1.9.2:
2
/dev/null,/dev/stdin
{"0":"/dev/null","1":"/dev/stdin"}

So it has a length, can be iterated over using Array.forEach, but when stringified comes out in that not so great form.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So, here's what we're thinking:

It's probably the JSON-serialization code that fails to deal with this object properly.

Possibly you can convert it to a "more real" javascript array by doing:

value.fileList = Array.prototype.concat(node.mozGetFileNameArray({}));

Comment 9

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #8)
> So, here's what we're thinking:
> 
> It's probably the JSON-serialization code that fails to deal with this object
> properly.
> 
> Possibly you can convert it to a "more real" javascript array by doing:
> 
> value.fileList = Array.prototype.concat(node.mozGetFileNameArray({}));

Yea that works. But JSON serialization is a pretty important thing... so we should probably fix that, right?

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It's very likely the serialization of arrays returned from XPConnect that's the culprit (why they are different from JS arrays I don't know). And it web pages aren't exposed to "XPConnect arrays".

And it seems like it's working on trunk so not sure it's worth back porting to the 1.9.2 branch.

We should definitely make sure there are tests for this so that it doesn't regress though. I'll make sure that happens.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #6)
> I talked this over with jst and he said that we might actually be returning the
> same type of {0: "name1", 1: "name2"} objects on trunk too. It might not
> neccesarily be wrong to do so. Maybe.

Looking at the code in xpcconvert.cpp, we're definitely creating a JS array object. However, do note bug 513428, which still needs to land on the 1.9.2 branch and could explain this bug.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ah, yes, that is probably the cause here.

Comment 13

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.1

Since it seems like the fix from JS land depends on other changes made post branching that never landed and we need to freeze, it seems best to just workaround it.

This is the same as Jonas's patch to sessionstore from bug 523771 with the exception that I used |[].concat(...)| as discussed above.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Not blocking, would take a reviewed and tested patch.

Comment 15

[Simon Bünzli]

Comment on attachment 413464 [details] [diff] [review]
Patch v0.1

I'd prefer |Array.slice(...)| to |[].concat(...)|.

And please also add a comment as to why this is needed at all (including this bug's number).

r+=me with these two fixed.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

bug 513428 just landed on the 1.9.2 branch, so this might be fixed. I don't know if you want to take the hack anyway.

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Paul talked to me, we're gonna take the full patch from bug 523771 for mozilla-1.9.2

Comment 18

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 413464 [details] [diff] [review]
Patch v0.1

This patch is now obsolete since we'll be taking Jonas's original patch from bug 523771.

Comment 19

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Landed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/1756a41d1fe3. This is the sessionstore part from Jonas's original patch in bug 523771 (which was reviewed & got approval there).

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Thanks a ton for your work here Paul!