661752 - Ensure that when <input>.files is available to a web page the file sizes are already populated

Status: NEW

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

Description

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

Attachment: Patch

Right now if a webpage access <input>.files[0].size we'll synchronously touch the disk on the main thread to figure out the size.  We can avoid this by pre-statting off the main thread and delaying onchange for files until those stats are done.

We create two new runnables here.  The first is the FileSizeGetter runnable that calls GetFileSize on the underlying nsIFile.  This is created from the constructor of the nsDOMFile and dispatched to the stream transport service's nsIEventTarget.  It takes an optional callback/closure which is used by:

The second is the FileOnChangeDispatcher runnable.  This is created by the AsyncOnClickHandler runnable and receives a callback from each FileSizeGetter runnable.  Once all callbacks are received it dispatches itself to the main thread and fires <input>.onchange (and sets <input.files>).

The tricky bits here are:

1. FileSizeGetter does not hold the nsIDOMFile alive, so the File object revokes the FileSizeGetter in its dtor if the getter hasn't run yet.
2. The FileOnChangeDispatcher has to remember if any of the FileSizeGetters were revoked.  The callback from FileSizeGetter must still be dispatched (otherwise the FileOnChangeDispatcher will leak) but the FileOnChangeDispatcher must not fire events at the page.
3. The count of waiting callbacks in FileOnChangeDispatcher can go negative if some FileSizeGetters complete while the AsyncOnClickHandler is still running.  We handle this OK since we only increase the count once, so we can't prematurely hit 0.
4. If we end up in File.size before the FileSizeGetter completes, we run the getter synchronously on the main thread.  This won't happen for <input>.file, but I couldn't convince myself that we won't end up in File.size early with one of the other ways that Files are created, so I added this.

Jonas told me to request review from bent since there are threads involved :-)

Comment 1

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

Attachment: Add nsRevocableEventPtr.swap()

This is totally a micro-optimization, but it's easy enough to do.

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 537070 [details] [diff] [review]
Patch

Discussed with Kyle over IRC, he's going to attempt to make this simpler (and lockless) by holding the nsIDOMFile alive while figuring out its size.

Comment 3

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

Attachment: Patch

Now with no locking.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 586045 [details] [diff] [review]
Patch

Review of attachment 586045 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/public/nsDOMFile.h
@@ +236,5 @@
> +  // off the main thread but on the same thread.
> +  nsresult PreStat(StatDoneCallback aCallback, void* aClosure);
> +
> +private:
> +

Nit: Nuke extra newline.

::: content/base/src/nsDOMFile.cpp
@@ +153,5 @@
> +  if (!gDOMFileIOThread) {
> +    gDOMFileIOThread = new LazyIdleThread(DEFAULT_THREAD_TIMEOUT_MS,
> +                                          LazyIdleThread::AutomaticShutdown);
> +
> +    ClearOnShutdown(&gDOMFileIOThread);

Hm, this is weird. The thing is ClearOnShutdown happens after threads are already shut down. I think we should figure out a better way to do this.

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +237,5 @@
>  
>  NS_IMPL_ISUPPORTS1(nsHTMLInputElementState, nsHTMLInputElementState)
>  NS_DEFINE_STATIC_IID_ACCESSOR(nsHTMLInputElementState, NS_INPUT_ELEMENT_STATE_IID)
>  
> +class FileOnChangeDispatcher : public nsRunnable {

Nit: { on its own line.

@@ +279,5 @@
> +
> +  NS_IMETHOD Run();
> +
> +protected:
> +  nsCOMArray<nsIDOMFile> mFiles;

Ew. Please replace with nsTArray

@@ +299,5 @@
> +  mFilesRemaining = aFiles.Length();
> +  NS_ASSERTION(mFilesRemaining, "Don't call me if you have no files!");
> +
> +  // stat our files in the background and dispatch when we're done.
> +  nsresult rv = NS_OK;

You're not really using this, remove and just do |if (NS_FAILED(file->PreStat(...)))| below.

@@ +308,5 @@
> +    if (NS_FAILED(rv)) {
> +      // NB: We may have already dispatched some runnables.  Some of them may
> +      // have even completed already.  Go ahead and dispatch synchronously,
> +      // noting that we can't dispatch twice because the number of runnables
> +      // we dispatched is less than mFilesRemaining.

This seems like something we should warn about.

@@ +332,5 @@
> +FileOnChangeDispatcher::Run()
> +{
> +  NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> +
> +  NS_RELEASE_THIS(); // We are held alive by our caller.

This doesn't seem safe. What if this is the last reference?

@@ +343,5 @@
> +  mInput->mFileOnChangeDispatcher = nsnull;
> +
> +  mInput->SetFiles(mFiles, true);
> +  nsContentUtils::DispatchTrustedEvent(mInput->OwnerDoc(),
> +                                       static_cast<nsIDOMHTMLInputElement*>(mInput.get()),

Is this cast needed? What happens if the cast doesn't equal the result you would get if you tried to QI to nsIDOMHTMLInputElement?

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 586045 [details] [diff] [review]
Patch

I think we discussed a different way to do this.