Closed Bug 578096 Opened 14 years ago Closed 14 years ago

Uploading a file via drag-and-drop and XMLHttpRequest.send(File) locks the file

Categories

(Core :: General, defect)

Product:
Core
Component:
General
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla2.0b7
Tracking Flags:
Tracking Status
blocking1.9.2 --- needed
status1.9.2 --- .14-fixed
status1.9.1 --- unaffected

People

(Reporter: lfrenkel, Assigned: khuey)

References

Details

(Keywords: testcase, verified1.9.2, Whiteboard: [should be fixed by 583863])

Attachments

(3 files, 2 obsolete files)

reporters's testcase
421 bytes, text/html
Details
Test for checkin.
2.25 KB, patch
Details | Diff | Splinter Review
XHR locks file longer than necessary.
1.48 KB, patch
sicking
: review+
christian
: approval1.9.2.14+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US) AppleWebKit/533.4 (KHTML, like Gecko) Chrome/5.0.375.99 Safari/533.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.6) Gecko/20100625 Firefox/3.6.6

Drag-and-drop in conjunction with XMLHttpRequest.send(File) locks the uploaded file. This method works, but after the operation, the uploaded file can't be moved with Windows Explorer. Attempting to move the file results in the following error message: "The action can't be completed because the file is open in Firefox." The file stays locked until Firefox is restarted.

Reproducible: Always

Steps to Reproduce:
1. Upload a file via drag-and-drop and XMLHttpRequest.send(File) 
2. Try moving the file to a different folder with Windows Explorer
3. Observe the error message ""The action can't be completed because the file is open in Firefox"
Actual Results:  
Error message

Expected Results:  
No error message
See the comments in bug 183689 - it's often caused by the LiveHTTPHeaders or
Firebug extensions.

see also bug 459384
Severity: critical → normal
Component: Developer Tools → General
QA Contact: developer.tools → general
Blocks: 574221
As far as understand this report is about the new features introduced in Firefox 3.6, so the older bugs aren't necessarily relevant.

lfrenkel: do you have a testcase or could you create one please?
Keywords: testcase-wanted
Product: Firefox → Core
QA Contact: general → general
Ah, yes.  This is a bug in the patch for bug 491201.  Compare the code in XHR file upload:

2311       // Feed local file input stream into our upload channel
2312       return NS_NewLocalFileInputStream(aResult, internalFile);

to the code in form submission file upload:

503     // Get input stream
504     rv = NS_NewLocalFileInputStream(getter_AddRefs(fileStream),
505                                     aFile, -1, -1,
506                                     nsIFileInputStream::CLOSE_ON_EOF |
507                                     nsIFileInputStream::REOPEN_ON_REWIND);

XHR needs to do the same, plus probably make sure to explicitly close the stream if the POST doesn't end up actually getting sent (e.g. if the preflight fails or whatever).
Blocks: 491201
Status: UNCONFIRMED → NEW
blocking2.0: --- → ?
Ever confirmed: true
Keywords: testcase-wanted
Jonas, can you take this?  That part about handling the error cases seems to be most up your alley....
Though... when the stream object is destroyed, it should close itself.  So if we're really not releasing the file, that suggests that something is leaking.  So I really would like to see a testcase here...
Keywords: testcase-wanted
This problem isn't limited to drag-and-drop. It also happens with <input type="file"> and XMLHttpRequest.send(File). Something like this should do it, where blah.html is can handle the upload.

<html>
	<head>
		<script>
			function handleChange() {
				var file = document.getElementById('file');
				var xhr = new XMLHttpRequest();
				xhr.open('POST', 'blah.html', true);
				xhr.send(file.files[0]);
			}
		</script>
	</head>
	<body>
		<input type="file" id="file" onchange="handleChange()">
	</body>
</html>
I can't seem to reproduce the bug with the attached testcase on trunk.  The file only stays locked until the GC which finalizes the XHR object (and thence its mChannel and thence the file stream).

Reporter, do you see the problem with the attached testcase?
I also can't reproduce with the attached testcase on 1.9.2 branch...
Say the XMLHttpRequest object is not GCed, shouldn't the file still be closed after it's sent or failed to be sent?
That's what comment 3 is about, yes.  I'm just concerned about why the reporter is seeing them not GCed, in addition to the bug we need to fix here.
Ah, I assumed "The file stays locked until Firefox is restarted." from comment 0 applied to a more complex situation than the testcase (like the Gmail case), where something might be holding on to the XHR object.

lfrenkel, do you have a testcase for the situation you described in comment 0, where nothing is supposed to hold the XMLHttpRequest object alive? (Or is it the same testcase from comment 6?) It might be worth filing a separate bug on that.
Not going to block on this without a reproducible testcase. Please renominate if/when a testcase is available.
blocking2.0: ? → ---
The attached testcase can be used to reproduce the problem, as currently described in summary, although not the "The file stays locked until Firefox is restarted" part of comment 0.

If there is a case when "The file stays locked until Firefox is restarted", it should be filed separately with a testcase.

Until then we should definitely fix the issue with the testcase. Re-setting the blocking? flag, since it was requested for the issue in comment 3 (the current summary) originally.
blocking2.0: --- → ?
Keywords: testcase-wanted → testcase
Yeah, I think this is a must-fix, since it causes files to be locked for arbitrary (from the user's point of view) lengths of time.
I think the patch in bug 583863 fixes this.
Depends on: 583863
By making us share the code with form submission?  If so, awesome.
(In reply to comment #17)
> By making us share the code with form submission?  If so, awesome.

Yep! :-)
Whiteboard: [should be fixed by 583863]
I just pushed 583863.  If somebody could verify that this bug is fixed on or after http://hg.mozilla.org/mozilla-central/rev/af1365b24066 that would be awesome.
Make that http://hg.mozilla.org/mozilla-central/rev/8d846fde08cb.  Today's nightly or anything later should be fixed.
Marking this FIXED by Kyles patch in bug 583863. Would still be great to get this confirmed.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Yes, both this testcase and gmail's uploading (bug 574221) works fine with Mozilla/5.0 (Windows NT 6.0; rv:2.0b6pre) Gecko/20100909 Firefox/4.0b6pre, though I can reproduce on Firefox 3.6.
Target Milestone: --- → mozilla2.0b6
Version: unspecified → Trunk
This should be testable, right?  The issue is that the XHR is keeping the nsIFile alive (and thus the file on disk locked) until the XHR is destroyed even though the file is no longer needed after the send.  If my understanding is correct, I will write a test for this.
blocking2.0: ? → ---
Flags: in-testsuite?
Rereading the comments, it looks like if we keep the XHR alive we should be able to test this.
Assignee: nobody → khuey
> The issue is that the XHR is keeping the nsIFile alive

The nsFileInputStream, but yes.
Attached patch Test for checkin (obsolete) — — Splinter Review 

        Attachment #474042 -
        Attachment is obsolete: true

    
    

    
  
Seems that this is only a partial fix (tested on 4.0b6).
In the GMail test case, the file is locked after uploading, and is released only a few moments after it is done. The file may stay locked even if the tab is closed.
In comparison, if the uploaded in the regular way it is released immediately after being uploaded.

    
    

    
  
This fix didn't make it into 4.0b6.  It'll be in b7.

    
    

    
  
We're going to want to backport a fix to 1.9.2.  There are a lot of user complaints about this.

          status1.9.2:
          --- → ?

    
    

    
  
It seems like bug 583863 fixes it, which is a pretty major change. Why is it important to block release? Where are we seeing user complaints?

Are we able to get a less-risky patch for branch?
blocking1.9.2: --- → ?

          status1.9.2:
          ? → wanted

    
    

    
  
We're definitely going to want a much narrower patch on branch.

    
    

    
  
> Where are we seeing user complaints?

We're not.  gmail is, though, from Firefox 3.6 users dragging in attachments.

    
    

    
  
we're also seeing duplicate bugs filed, those should count as complaints.

          status1.9.1:
          --- → unaffected
blocking1.9.2: ? → needed

    
    

    
  
(In reply to comment #37)
> We're definitely going to want a much narrower patch on branch.

Do you think we'll have a less invasive patch for the next branch release (likely due mid Feb)?

    
    

    
  
Yeah, I'm not likely to get to touch this until after my semester finishes, but as long as code freeze for that release is in 2011 I should be able to make that.

    
    

    
  
Comment on attachment 502699 [details] [diff] [review]
XHR locks file longer than necessary.

Would nice to get this in by tomorrow's freeze.

        Attachment #502699 -
        Flags: review?(jonas)

    
    

    
  
This is too late for Firefox4, we should do it for the next release though.

    
    

    
  
Comment on attachment 502699 [details] [diff] [review]
XHR locks file longer than necessary.

Ah, this is for 3.6

        Attachment #502699 -
        Flags: review?(jonas) → review+

    
    

    
  
Comment on attachment 502699 [details] [diff] [review]
XHR locks file longer than necessary.

a=LegNeato for 1.9.2.14. As you know, code freeze for non-blockers such as this bug is tomorrow evening PST.

        Attachment #502699 -
        Flags: approval1.9.2.14+

    
    

    
  
I can't reproduce this with the attached testcase on 1.9.2.13. Since I see that others have had issues there as well, can someone give me actual steps to reproduce?

Also, Kyle, is there a reason why the automated test wasn't checked into 1.9.2 with the fix? Does it need a review?
Whiteboard: [should be fixed by 583863] → [should be fixed by 583863] [qa-examined-192]

    
    

    
  
I forgot about landing the mochitest, but now that I run the test on a 1.9.2 build from before this change I see that the test does not fail.

I'll poke at it a bit more as time permits.  In the meantime, hopefully we can verify this through the gmail drag and drop attacher (which seemed to be what most complaints were about) or something else.

    
    

    
  
Do you know the steps to reproduce with the gmail drag and drop attacher (I'm unfamiliar with it)?

    
    

    
  
Happily, it turns out that gmail does useragent sniffing and turns off this feature in nightlies so I had to override my UA string. :-)

That said, I can verify that the problem is present in 1.9.2.13 and fixed in the nightly 1.9.2.14pre build (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.14pre) Gecko/20110113 Namoroka/3.6.14pre ( .NET CLR 3.5.30729)).
Keywords: verified1.9.2
Whiteboard: [should be fixed by 583863] [qa-examined-192] → [should be fixed by 583863]

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: