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

Status: RESOLVED FIXED

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

Description

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

Patch in a bit.

Comment 1

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

Attachment: Kill nsIFileControlElement

Comment 2

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

Attachment: Refactor

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

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

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

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.

Comment 5

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

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

Comment 6

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

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

Comment 7

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

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 8

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

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.

Comment 9

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

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

Comment 10

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

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

Comment 11

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

Attachment: Kill nsIFileControlElement V2

This adds nsHTMLInputElement::FromContent and uses that to do the checking.  Carrying forward r+.

Comment 12

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

Attachment: Refactor V2

Comments about simplification and GetUrl addressed.

Comment 13

[Benjamin Smedberg]

Is this supposed to be a blocker? I'm not sure it's worth approving these changes now unless they are supposed to block.

Comment 14

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

We're going to need this for the fennec camera work.

Comment 15

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

Attachment: Patch V2.1

Missed the suggestion to implement nsIXHRSendable.

Comment 16

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

Attachment: Interdiff

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 17

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

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.

Comment 18

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

Attachment: Patch V2.2

I'd lost the bit about not using GetUrl directly in the FileReader.  That's in here now.

Comment 19

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

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.

Comment 20

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

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

Comment 21

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

Attachment: Interdiff V2.2 - V2.3

Still unresolved issues are naming the file and what to do about tests that set .value to a nonexistent file.

Comment 22

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

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

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

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.

Comment 24

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

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

Comment 26

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

Attachment: Kill nsIFileControlElement

Rebased to tip, carrying forward r+

Comment 27

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

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.

Comment 28

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

Attachment: Almost-done patch

This passes all tests except as noted in the previous comment.

Comment 29

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

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

Comment 30

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

Yeah, we can do that.  I'll work up another patch on top of the above.

Comment 31

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

Attachment: Add another method to let layout in the side door.

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.

Comment 32

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

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.

Comment 33

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

Attachment: Attempt to fix print preview

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.

Comment 34

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

So the problem ended up being that the first patch botches the deCOMtamination and asks the anonymous text control for the filename.

Comment 35

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

Attachment: : Refactor <input> implementation to deal with files that are not on the disk. a=blocking-fennec

Comment 36

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

Last attachment is as pushed.

http://hg.mozilla.org/mozilla-central/rev/af1365b24066

Comment 37

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

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.

Comment 38

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

Backed out due to leaks.

Comment 39

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

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

Comment 40

[[e-mail]]

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?

Comment 41

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

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

Comment 42

[[e-mail]]

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

Comment 43

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

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

Comment 44

[Boris Zbarsky [:bzbarsky]]

This almost certainly caused bug 594964.

Comment 45

[Eric Shepherd [:sheppy]]

Added the note as indicated in comment #37.