Closed Bug 583863 Opened 10 years ago Closed 10 years ago

Refactor HTML input implementation to work with "files" that aren't on the disk.

Categories

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

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b7
Tracking Status
fennec 2.0+ ---

People

(Reporter: khuey, Assigned: khuey)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(5 files, 8 obsolete files)

13.14 KB, patch
khuey
: review+
Details | Diff | Splinter Review
52.90 KB, patch
Details | Diff | Splinter Review
2.73 KB, patch
sicking
: review+
Details | Diff | Splinter Review
1.97 KB, patch
Details | Diff | Splinter Review
100.18 KB, patch
Details | Diff | Splinter Review
Attached patch Kill nsIFileControlElement (obsolete) — Splinter Review
Attachment #462190 - Flags: superreview?(jst)
Attachment #462190 - Flags: review?(jonas)
Attached patch Refactor (obsolete) — Splinter Review
This refactors the input element to store an array of nsIDOMFiles and provides an implementation of a "fake" DOMFile that has no backing nsIFile (and no name).

We could do a lot of different things here.  We could make nsDOMFakeFile just the File API's blob (the FakeFile basically is a file with no name and no backing nsIFile).  We also have a lot of different tests that rely on setting a file input element's value to some nonexistent file which this breaks (since we won't be able to create an nsIFile for it).
Attachment #462192 - Flags: superreview?(jst)
Attachment #462192 - Flags: review?(jonas)
(In reply to comment #1)
> Created attachment 462190 [details] [diff] [review]
> Kill nsIFileControlElement

Perhaps add nsHTMLInputElement::FromContent, like at <http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLOptionElement.h#57>.
Comment on attachment 462190 [details] [diff] [review]
Kill nsIFileControlElement

>@@ -275,24 +275,24 @@ nsFileControlFrame::CreateAnonymousConte
>   // Mark the element to be native anonymous before setting any attributes.
>   mTextContent->SetNativeAnonymous();
> 
>   mTextContent->SetAttr(kNameSpaceID_None, nsGkAtoms::type,
>                         NS_LITERAL_STRING("text"), PR_FALSE);
> 
>   nsCOMPtr<nsIDOMHTMLInputElement> textControl = do_QueryInterface(mTextContent);
>   if (textControl) {
>-    nsCOMPtr<nsIFileControlElement> fileControl = do_QueryInterface(mContent);
>-    if (fileControl) {
>-      // Initialize value when we create the content in case the value was set
>-      // before we got here
>-      nsAutoString value;
>-      fileControl->GetDisplayFileName(value);
>-      textControl->SetValue(value);
>-    }
>+    nsRefPtr<nsHTMLInputElement> fileControl =
>+      static_cast<nsHTMLInputElement*>(textControl.get());
>+
>+    // Initialize value when we create the content in case the value was set
>+    // before we got here
>+    nsAutoString value;
>+    fileControl->GetDisplayFileName(value);
>+    textControl->SetValue(value);

You probably want to keep *some* type of check that mContent is the right type here. Generally what we do is check that IsHTML() returns true and Tag() returns nsGkAtoms::input. This is what we do elsewhere before casting to specific element subclasses. You can then cast mContent directly. Should be safer and faster.

If you want, you can also remove the QI on mTextContent and cast that directly to nsHTMLInputElement since we just created it and thus know that it's of the right type.

>@@ -437,17 +437,19 @@ nsFileControlFrame::CaptureMouseListener
> 
>   NS_ASSERTION(mFrame, "We should have been unregistered");
>   if (!ShouldProcessMouseClick(aMouseEvent))
>     return NS_OK;
> 
>   // Get parent nsIDOMWindowInternal object.
>   nsIContent* content = mFrame->GetContent();
>   nsCOMPtr<nsIDOMNSHTMLInputElement> inputElem = do_QueryInterface(content);
>-  nsCOMPtr<nsIFileControlElement> fileControl = do_QueryInterface(content);
>+  nsRefPtr<nsHTMLInputElement> fileControl =
>+    static_cast<nsHTMLInputElement*>(inputElem.get());
>+
>   if (!content || !inputElem || !fileControl)
>     return NS_ERROR_FAILURE;

Same here, you should add a check to make sure that the cast is safe. Check content->IsHTML and content->Tag. After null-checking content of course.

>@@ -534,17 +536,19 @@ nsFileControlFrame::BrowseMouseListener:
> 
>   NS_ASSERTION(mFrame, "We should have been unregistered");
>   if (!ShouldProcessMouseClick(aMouseEvent))
>     return NS_OK;
> 
>   // Get parent nsIDOMWindowInternal object.
>   nsIContent* content = mFrame->GetContent();
>   nsCOMPtr<nsIDOMNSHTMLInputElement> inputElem = do_QueryInterface(content);
>-  nsCOMPtr<nsIFileControlElement> fileControl = do_QueryInterface(content);
>+  nsRefPtr<nsHTMLInputElement> fileControl =
>+    static_cast<nsHTMLInputElement*>(inputElem.get());
>+
>   if (!content || !inputElem || !fileControl)
>     return NS_ERROR_FAILURE;

And here

> nsFileControlFrame::GetFormProperty(nsIAtom* aName, nsAString& aValue) const
> {
>   aValue.Truncate();  // initialize out param
> 
>   if (nsGkAtoms::value == aName) {
>-    nsCOMPtr<nsIFileControlElement> fileControl =
>+    nsCOMPtr<nsIDOMHTMLInputElement> inputElem =
>       do_QueryInterface(mContent);
>+    nsRefPtr<nsHTMLInputElement> fileControl =
>+      static_cast<nsHTMLInputElement*>(inputElem.get());
>+

And preferably use IsHTML/Tag checks here too.


Hmm.. feel free to add a function like:

IsHTML(nsIAtom* aTag) {
  return IsHTML() && Tag() == aTag;
}

to nsIContent. You could also add a

static nsHTMLInputElement* FromContent(nsIContent* aContent) {
  return aContent && aContent->IsHTML(nsGkAtoms::input)) ?
    static_cast<nsHTMLInputElement*>(aContent) : nsnull;
}

to nsHTMLInputElement. We have similar functions in other element classes.

r=me with that fixed.

For what it's worth, I don't think this needs sr, but feel free to request one if you want.
Attachment #462190 - Flags: review?(jonas) → review+
Comment on attachment 462192 [details] [diff] [review]
Refactor

Will this work if you attempt to read from the fake-file twice at the same time? It doesn't appear to. I.e. if you do:

xhr1 = new XMLHttpRequest();
xhr1.open("POST", url1);
xhr1.send(myfile);
xhr2 = new XMLHttpRequest();
xhr2.open("POST", url2);
xhr2.send(myfile);

or

img1.src = myfile.url;
img2.src = myfile.url;

where myfile is a fake-file.


In nsXMLHttpRequest.cpp, instead of modifying the nsIDOMFile-specific code, make nsDOMFile and nsDOMFakeFile implement nsIXHRSendable and remove the file-specific code from nsXMLHttpRequest.cpp

minusing based on that it looks like multiple reads will fail. Please rerequest if I'm misunderestimating the code :)
Attachment #462192 - Flags: superreview?(jst)
Attachment #462192 - Flags: review?(jonas)
Attachment #462192 - Flags: review-
Attachment #462192 - Flags: review- → review?(jonas)
(In reply to comment #5)
> minusing based on that it looks like multiple reads will fail. Please rerequest
> if I'm misunderestimating the code :)

This passes the buck to the callback function.  That should probably be documented ;-)
That is, the callback receives an XPCOM object (the nsISupports*) and has to provide an input stream from that.  Thus the callback has to handle the "hard" part of this.
Comment on attachment 462192 [details] [diff] [review]
Refactor

I talked to Kyle and I now see how this is intended to work.

However since we, at least for now, only need memory backed files, we should just rename nsDOMFakeFile to nsDOMMemoryFile or some such, and put all the logic in that class. No need for callbacks and such yet.
Attachment #462192 - Flags: review?(jonas)
Comment on attachment 462192 [details] [diff] [review]
Refactor

in nsDOMFileReader, you should not use the GetURL method and load from that url. Calling GetURL has some side effects since we have to keep the file alive for the lifetime of the calling page.

One solution would be to extract a channel directly and call AsyncOpen on that. But I'm not sure if we have memory-backed channels :(
(In reply to comment #9)
> One solution would be to extract a channel directly and call AsyncOpen on that.
> But I'm not sure if we have memory-backed channels :(

After chatting with sicking, the better solution is to register a separate moz-filedata uri for internal use and release it when we're done.
Attached patch Kill nsIFileControlElement V2 (obsolete) — Splinter Review
This adds nsHTMLInputElement::FromContent and uses that to do the checking.  Carrying forward r+.
Attachment #462190 - Attachment is obsolete: true
Attachment #462874 - Flags: review+
Attachment #462874 - Flags: approval2.0?
Attached patch Refactor V2 (obsolete) — Splinter Review
Comments about simplification and GetUrl addressed.
Attachment #462192 - Attachment is obsolete: true
Attachment #462948 - Flags: review?(jonas)
Is this supposed to be a blocker? I'm not sure it's worth approving these changes now unless they are supposed to block.
We're going to need this for the fennec camera work.
tracking-fennec: --- → ?
Attached patch Patch V2.1 (obsolete) — Splinter Review
Missed the suggestion to implement nsIXHRSendable.
Attachment #462948 - Attachment is obsolete: true
Attachment #463232 - Flags: review?(jonas)
Attachment #462948 - Flags: review?(jonas)
Attachment #462874 - Flags: approval2.0? → approval2.0+
tracking-fennec: ? → 2.0+
Attached patch Interdiff (obsolete) — Splinter Review
I just copied and pasted the relevant hunks, getting the earlier version to apply again (or finding a rev it applied to) didn't seem worth it.
Comment on attachment 463232 [details] [diff] [review]
Patch V2.1

Actually, looking at this a bit more closely, I lost a piece somewhere along the way.
Attachment #463232 - Attachment is obsolete: true
Attachment #463232 - Flags: review?(jonas)
Attached patch Patch V2.2 (obsolete) — Splinter Review
I'd lost the bit about not using GetUrl directly in the FileReader.  That's in here now.
Attachment #464499 - Flags: review?(jonas)
Comment on attachment 464499 [details] [diff] [review]
Patch V2.2

We might want to give the camera "files" some reasonable name. Such as "generated.jpg" or some such.

> nsDOMFile::GetAsDataURL(nsAString &aResult)
> {
>   aResult.AssignLiteral("data:");
> 
>   nsresult rv;
>-  nsCOMPtr<nsIMIMEService> mimeService =
>-    do_GetService(NS_MIMESERVICE_CONTRACTID, &rv);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  if (!mContentType.Length()) {
>+    nsCOMPtr<nsIMIMEService> mimeService =
>+      do_GetService(NS_MIMESERVICE_CONTRACTID, &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);
> 
>-  nsCAutoString contentType;
>-  rv = mimeService->GetTypeFromFile(mFile, contentType);
>-  if (NS_SUCCEEDED(rv)) {
>-    AppendUTF8toUTF16(contentType, aResult);
>+    nsCAutoString contentType;
>+    rv = mimeService->GetTypeFromFile(mFile, contentType);
>+    if (NS_SUCCEEDED(rv)) {
>+      AppendUTF8toUTF16(contentType, aResult);
>+    } else {
>+      aResult.AppendLiteral("application/octet-stream");
>+    }
>   } else {
>-    aResult.AppendLiteral("application/octet-stream");
>+    aResult.Append(mContentType);
>   }

Might be worth assigning the resulting type to mContentType here. I.e. do something like:

if (mContentType.IsEmpty()) {
  ... assign to mContentType ...
}
aResult.Append(mContentType);

Or even better, can you call GetType(some_temporary_string); and have GetType cache in mContentType?

>+nsDOMFile::GetSendInfo(nsIInputStream** aBody,
>+                       nsACString& aContentType,
>+                       nsACString& aCharset)
>+{
>+  nsresult rv;
>+
>+  nsCOMPtr<nsIInputStream> stream;
>+  rv = this->GetInternalStream(getter_AddRefs(stream));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  rv = this->GetType(aContentType);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  rv = this->GuessCharset(stream, aCharset);
>+  NS_ENSURE_SUCCESS(rv, rv);

Don't set a charset at all. Better let the server do the guessing, especially since many times it'll be a binary file without charset at all.

>diff --git a/content/base/src/nsFileDataProtocolHandler.cpp b/content/base/src/nsFileDataProtocolHandler.cpp
>-nsFileDataProtocolHandler::AddFileDataEntry(nsACString& aUri, nsIFile* aFile,
>+nsFileDataProtocolHandler::AddFileDataEntry(nsACString& aUri,
>+					    nsIDOMFile* aFile,
>                                             nsIPrincipal* aPrincipal)

Fix tabs.

>diff --git a/content/base/src/nsFileDataProtocolHandler.h b/content/base/src/nsFileDataProtocolHandler.h
>@@ -57,7 +57,8 @@ public:
>   virtual ~nsFileDataProtocolHandler() {}
> 
>   // Methods for managing uri->file mapping
>-  static void AddFileDataEntry(nsACString& aUri, nsIFile* aFile,
>+  static void AddFileDataEntry(nsACString& aUri,
>+			       nsIDOMFile* aFile,

Fix tabs. (You might want to check the whole diff for tabs).

>diff --git a/content/base/test/test_bug345339.html b/content/base/test/test_bug345339.html
>--- a/content/base/test/test_bug345339.html
>+++ b/content/base/test/test_bug345339.html
>@@ -32,7 +32,7 @@ function afterLoad() {
>     iframeDoc.getElementById("hidden").value = "gecko";
> 
>     netscape.security.PrivilegeManager.enablePrivilege("UniversalFileRead");
>-    iframeDoc.getElementById("file").value = "dummyfile.txt";
>+//    iframeDoc.getElementById("file").value = "test_bug345339.html";
> 
>     /* Reload the page */
>     $("testframe").setAttribute("onload", "afterReload()");
>@@ -55,8 +55,8 @@ function afterReload() {
>     is(iframeDoc.getElementById("hidden").value, "gecko",
>        "hidden field value preserved");
>     netscape.security.PrivilegeManager.enablePrivilege("UniversalFileRead");
>-    is(iframeDoc.getElementById("file").value, "dummyfile.txt",
>-       "file field value preserved");
>+//    is(iframeDoc.getElementById("file").value, "test_bug345339.txt",
>+//       "file field value preserved");

Why these changes?


>diff --git a/content/html/content/src/nsHTMLInputElement.cpp b/content/html/content/src/nsHTMLInputElement.cpp
>--- a/content/html/content/src/nsHTMLInputElement.cpp
>+++ b/content/html/content/src/nsHTMLInputElement.cpp
>@@ -183,12 +183,14 @@ class nsHTMLInputElementState : public n
>       mValue = aValue;
>     }
> 
>-    const nsTArray<nsString>& GetFilenames() {
>-      return mFilenames;
>+    void GetFiles(nsCOMArray<nsIDOMFile> aFiles) {
>+      aFiles.Clear();
>+      aFiles.AppendObjects(mFiles);
>     }

Why not just return a reference to mFiles, like the old function does?

>@@ -697,12 +701,38 @@ nsHTMLInputElement::MozSetFileNameArray(
>     return NS_ERROR_DOM_SECURITY_ERR;
>   }
> 
>-  nsTArray<nsString> fileNames(aLength);
>+  nsCOMArray<nsIDOMFile> files;
>   for (PRUint32 i = 0; i < aLength; ++i) {
>-    fileNames.AppendElement(aFileNames[i]);
>+    nsCOMPtr<nsIFile> file;
>+    // XXXkhuey this should be an ASCII only case folding.
>+    if (StringBeginsWith(nsDependentString(aFileNames[i]),
>+                         NS_LITERAL_STRING("file:"),
>+                         nsCaseInsensitiveStringComparator())) {
>+      // Converts the URL string into the corresponding nsIFile if possible
>+      // A local file will be created if the URL string begins with file://
>+      NS_GetFileFromURLSpec(NS_ConvertUTF16toUTF8(aFileNames[i]),
>+                            getter_AddRefs(file));
>+    }
>+
>+    if (!file) {
>+      // this is no "file://", try as local file
>+      nsCOMPtr<nsILocalFile> localFile;
>+      NS_NewLocalFile(nsDependentString(aFileNames[i]),
>+                      PR_FALSE, getter_AddRefs(localFile));
>+      file = do_QueryInterface(localFile);
>+    }
>+
>+    if (file) {
>+      nsCOMPtr<nsIDOMFile> domFile =
>+        do_QueryObject(new nsDOMFile(file, GetOwnerDoc()));
>+      files.AppendObject(domFile);

No need for do_QueryObject here. nsCOMPtr<nsIDOMFile> domFile = new nsDOMFile... should work fine.

>@@ -724,7 +754,8 @@ nsHTMLInputElement::SetUserInput(const n
> 
>   if (mType == NS_FORM_INPUT_FILE)
>   {
>-    SetSingleFileName(aValue);
>+    return NS_ERROR_NOT_IMPLEMENTED;
>+    //SetSingleFile(aValue);
>   } else {
>     SetValueInternal(aValue, PR_TRUE, PR_TRUE);
>   }

This doesn't seem finished.


Looking very good in general. Please attach an interdiff for next iteration though. I ended up just reviewing the whole thing here since I wasn't sure if the interdiff corresponded to the latest patch version or not.
Attachment #464499 - Flags: review?(jonas) → review-
(In reply to comment #19)
> Comment on attachment 464499 [details] [diff] [review]
> Patch V2.2
> 
> We might want to give the camera "files" some reasonable name. Such as
> "generated.jpg" or some such.

Yeah, we probably do.

> >diff --git a/content/base/test/test_bug345339.html b/content/base/test/test_bug345339.html
> >--- a/content/base/test/test_bug345339.html
> >+++ b/content/base/test/test_bug345339.html
> >@@ -32,7 +32,7 @@ function afterLoad() {
> >     iframeDoc.getElementById("hidden").value = "gecko";
> > 
> >     netscape.security.PrivilegeManager.enablePrivilege("UniversalFileRead");
> >-    iframeDoc.getElementById("file").value = "dummyfile.txt";
> >+//    iframeDoc.getElementById("file").value = "test_bug345339.html";
> > 
> >     /* Reload the page */
> >     $("testframe").setAttribute("onload", "afterReload()");
> >@@ -55,8 +55,8 @@ function afterReload() {
> >     is(iframeDoc.getElementById("hidden").value, "gecko",
> >        "hidden field value preserved");
> >     netscape.security.PrivilegeManager.enablePrivilege("UniversalFileRead");
> >-    is(iframeDoc.getElementById("file").value, "dummyfile.txt",
> >-       "file field value preserved");
> >+//    is(iframeDoc.getElementById("file").value, "test_bug345339.txt",
> >+//       "file field value preserved");
> 
> Why these changes?

So the issue here is that in nsHTMLInputElement::MozSetFileNameArray (which is what everything else calls eventually) trying to create a nonexistent file will throw (because we're not going to be able to create any sort of DOMFile for it to store internally.  Idk what we want to do here (probably should just change the tests that do this to only do files that exist.)
Attached patch Interdiff V2.2 - V2.3 (obsolete) — Splinter Review
Still unresolved issues are naming the file and what to do about tests that set .value to a nonexistent file.
Attachment #464472 - Attachment is obsolete: true
Attachment #464815 - Flags: review?(jonas)
(In reply to comment #20)
> > Why these changes?
> 
> So the issue here is that in nsHTMLInputElement::MozSetFileNameArray (which is
> what everything else calls eventually) trying to create a nonexistent file will
> throw (because we're not going to be able to create any sort of DOMFile for it
> to store internally.  Idk what we want to do here (probably should just change
> the tests that do this to only do files that exist.)

Yeah, I think we should change the tests to only use existing files.

However I also think we shouldn't make MozSetFileNameArray or setting .value throw if there are non-existing filenames. Just ignore those names instead.
Comment on attachment 464815 [details] [diff] [review]
Interdiff V2.2 - V2.3

You're doing

nsCOMArray<nsIDOMFile> files = GetFiles();

a lot. However this copies the contents of the array, in at least many cases unnecessarily. Just do

nsCOMArray<nsIDOMFile>& files = GetFiles();

To be on the safe side you could even make GetFiles return a const nsCOMArray& and add |const| to callers accordingly.

r=me with that and fixing the tests as discussed in previous comment.
Attachment #464815 - Flags: review?(jonas) → review+
(In reply to comment #23)
> Comment on attachment 464815 [details] [diff] [review]
> Interdiff V2.2 - V2.3
> 
> You're doing
> 
> nsCOMArray<nsIDOMFile> files = GetFiles();
> 
> a lot. However this copies the contents of the array, in at least many cases
> unnecessarily. Just do
> 
> nsCOMArray<nsIDOMFile>& files = GetFiles();

Mmm good point.
Rebased to tip, carrying forward r+
Attachment #462874 - Attachment is obsolete: true
Attachment #468039 - Flags: review+
So an issue I ran into when fixing the last of the test failures is that with this patch the file names are no longer rendered.  This is because layout calls GetDisplayFilename which in turn calls GetMozFullPath on all of the DOMFiles.  GetMozFullPath returns nothing though because apparently layout doesn't have UniversalFileRead capability.

I'm not sure how to proceed here.
This passes all tests except as noted in the previous comment.
Attachment #464499 - Attachment is obsolete: true
Attachment #464815 - Attachment is obsolete: true
Can you cast the files to nsDOMFile and call some internal method there that doesn't perform the security check? We'd have to make sure never to stick anything that isn't an nsDOMFile into the array in nsHTMLInputElement.

In fact, maybe it'd be better to make nsHTMLInputElement hold an nsTArray<nsDOMFile> rather than nsCOMArray<nsIDOMFile>.
Yeah, we can do that.  I'll work up another patch on top of the above.
This adds another method to get the internal file name without security checks.  We could add this to the base object (nsDOMFile) rather than nsIDOMFile but that seems messier to me.
Attachment #469132 - Flags: review?(jonas)
Comment on attachment 469132 [details] [diff] [review]
Add another method to let layout in the side door.

I still think it would be cleaner to convert the internal code to use nsDOMFiles instead of putting [noscript] attributes on the interface. DeCOMtamination FTW!

But I'm totally fine with this too.
Attachment #469132 - Flags: review?(jonas) → review+
So, that fixes layout, but not print preview.  When the static doc gets created for print preview there are no files in it.  I tried this but it didn't work, when GetDisplayFileName gets called on the static document there is nothing there, which makes no sense to me.
So the problem ended up being that the first patch botches the deCOMtamination and asks the anonymous text control for the filename.
Last attachment is as pushed.

http://hg.mozilla.org/mozilla-central/rev/af1365b24066
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b6
We should document for extension authors that <input type="file"> will ignore anything of the form document.getElementById("random input type=file").value="file that doesn't exist").  This isn't relevant to webpages because we don't let them set the value to anything interesting anyways.
Keywords: dev-doc-needed
Backed out due to leaks.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The patch leaks when creation of a channel fails (the file doesn't exist).  Easy enough to fix, and glad that our test suite catches it.  Repushed: http://hg.mozilla.org/mozilla-central/rev/8d846fde08cb
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Blocks: 594687
Comment on attachment 472265 [details] [diff] [review]
: Refactor <input> implementation to deal with files that are not on the disk.  a=blocking-fennec

>+  var file = Components.classes["@mozilla.org/file/directory_service;1"]
>+               .getService(Components.interfaces.nsIProperties)
>+               .get("TmpD", Components.interfaces.nsILocalFile);
>+  file.append("346337_test1.file");
>+  file.createUnique(Components.interfaces.nsIFile.NORMAL_FILE_TYPE, 0666);
>+  filePath1 = file.path;
>+  file = Components.classes["@mozilla.org/file/directory_service;1"]
>+             .getService(Components.interfaces.nsIProperties)
>+             .get("TmpD", Components.interfaces.nsILocalFile);
>+  file.append("346337_test2.file");
>+  file.createUnique(Components.interfaces.nsIFile.NORMAL_FILE_TYPE, 0666);
You never clean these files up do you?
(In reply to comment #40)
> Comment on attachment 472265 [details] [diff] [review]
> : Refactor <input> implementation to deal with files that are not on the disk. 
> a=blocking-fennec
> 
> >+  var file = Components.classes["@mozilla.org/file/directory_service;1"]
> >+               .getService(Components.interfaces.nsIProperties)
> >+               .get("TmpD", Components.interfaces.nsILocalFile);
> >+  file.append("346337_test1.file");
> >+  file.createUnique(Components.interfaces.nsIFile.NORMAL_FILE_TYPE, 0666);
> >+  filePath1 = file.path;
> >+  file = Components.classes["@mozilla.org/file/directory_service;1"]
> >+             .getService(Components.interfaces.nsIProperties)
> >+             .get("TmpD", Components.interfaces.nsILocalFile);
> >+  file.append("346337_test2.file");
> >+  file.createUnique(Components.interfaces.nsIFile.NORMAL_FILE_TYPE, 0666);
> You never clean these files up do you?

No, doesn't look like it.  File a followup?
(In reply to comment #40)
>(From update of attachment 472265 [details] [diff] [review])
>>+  file.createUnique(Components.interfaces.nsIFile.NORMAL_FILE_TYPE, 0666);
>You never clean these files up do you?
In fact the tests don't seem to require the files to exist.
(In reply to comment #42)
> (In reply to comment #40)
> >(From update of attachment 472265 [details] [diff] [review])
> >>+  file.createUnique(Components.interfaces.nsIFile.NORMAL_FILE_TYPE, 0666);
> >You never clean these files up do you?
> In fact the tests don't seem to require the files to exist.

The DOM does.  You can't do document.getElementById("random input type=file element").value = "File that doesn't exist" anymore.  See comments 20 and 22.
This almost certainly caused bug 594964.
Depends on: 596162
Added the note as indicated in comment #37.
Depends on: 639378
Depends on: 672414
You need to log in before you can comment on or make changes to this bug.