Andrew Sutherland [:asuth] (he/him) Reporter
	Description • 11 years ago

Gecko implements support for FileReaderSync in workers, but does not support FileReader.  I am unable to find an active bug for this, so I'm filing one since the spec seems to suggests/requires that we implement the async API as well.


Specifically, section 8.2, constructors (http://dev.w3.org/2006/webapi/FileAPI/#filereaderConstrctr) says: "In environments where the global object is represented by a Window or a WorkerGlobalScope object, the FileReader constructor must be available."

And section 9, Reading on threads (http://dev.w3.org/2006/webapi/FileAPI/#readingOnThreads) says: "Workers can avail of both the asynchronous API (the FileReader object) and the synchronous API (the FileReaderSync object)."

	Andrew Sutherland [:asuth] (he/him) Reporter
	Comment 1 • 11 years ago

Note: As a completely acceptable work-around, you can just use URL.createObjectURL and an async XMLHttpRequest from the worker.  It does make me wonder why we have FileReader at all (except for the leaking Blob URLs thing).

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 2 • 11 years ago

Jonas has a plan to deprecate FileReader, IIRC.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 3 • 11 years ago

See proposed API in http://lists.w3.org/Archives/Public/public-webapps/2013AprJun/0727.html

So yeah, I could see punting on this if the new proposal gets any momentum.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 4 • 10 years ago

Shihua is going to dig in here!

	Shihua Zheng
	Comment 5 • 10 years ago

Here I upload some tests for this bug. Suggestions are appreciated.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 7 • 10 years ago

Comment on attachment 8425717 [details] [diff] [review]
test patch

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

These tests look good!

::: dom/workers/test/fileReader_worker.js
@@ +11,5 @@
> +
> +  var rtnObj = new Object();
> +
> +  var textReader = new FileReader();
> +  textReader.onload = function(event) {

It would be good to set error handlers on all the FileReader objects that you create, then you could post a special error message back to the main thread with some details about the failure. As this is currently written errors will simply not be handled and the test will just time out instead. This makes debugging test failures more difficult.

@@ +59,5 @@
> +}
> +
> +function handleOnLoadEvent(name) {
> +  is(event.target.readyState, FileReader.DONE,
> +     "Should be readyState in test " + name);

Hrm, I don't see where the is() function is ever defined here...

::: dom/workers/test/test_fileReader.xul
@@ +76,5 @@
> +  function readFileData(fileData, expectedText, /** optional */ encoding) {
> +    var worker = new Worker("fileReader_worker.js");
> +
> +    worker.onmessage = function(event) {
> +      is(event.data.text, expectedText, "readAsText in worker returned incorrect result.");

The third parameter to the is() function should explain the condition that is being tested, so rather than "returned incorrect result" you actually want "returned correct result". You'll want to invert the others as well.

@@ +84,5 @@
> +      finish();
> +    };
> +
> +    worker.onerror = function(event) {
> +      ok(false, "Worker had an error: " + event.data);

This will be an ErrorEvent instance, it doesn't have a 'data' property.

	Shihua Zheng
	Comment 8 • 10 years ago

Referring to your last comment: I find these test codes widely existing in worker tests (e.g.: dom/workers/test/test_file.xul). Should I file a bug against it?

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 9 • 10 years ago

(In reply to Shihua Zheng from comment #8)
> Referring to your last comment: I find these test codes widely existing in
> worker tests (e.g.: dom/workers/test/test_file.xul). Should I file a bug
> against it?

I think you did this already, right?

	Shihua Zheng
	Comment 12 • 10 years ago

Missed 2 unit tests on previous one. They are added.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 13 • 10 years ago

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

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

Oh, the patch has changed. This probably applies to the new one as well.

::: content/base/src/FileIOObject.cpp
@@ +147,3 @@
>  {
>    if (mReadyState != 1 || aStream != mAsyncStream) {
> +    return NS_ERROR_FAILURE;

This isn't really an error is it? Can't this happen if you call abort and the previous callback was already scheduled?

@@ +246,5 @@
> +NS_IMPL_RELEASE(FileIOObject::FileReaderCallbackDelegate)
> +NS_INTERFACE_MAP_BEGIN(FileIOObject::FileReaderCallbackDelegate)
> +  NS_INTERFACE_MAP_ENTRY(nsIInputStreamCallback)
> +  NS_INTERFACE_MAP_ENTRY(nsISupports)
> +NS_INTERFACE_MAP_END

You should be able to use the simple NS_IMPL_ISUPPORTS macro rather than breaking it into several macros like this.

::: content/base/src/FileIOObject.h
@@ +93,5 @@
>    uint64_t mTransferred;
> +
> +  nsCOMPtr<nsIEventTarget> mTarget;
> +
> +protected:

It looks like you're already in a protected section here.

@@ +94,5 @@
> +
> +  nsCOMPtr<nsIEventTarget> mTarget;
> +
> +protected:
> +  class FileReaderCallbackDelegate : public nsIInputStreamCallback {

Nit: { on its own line.

@@ +102,5 @@
> +    NS_DECL_NSIINPUTSTREAMCALLBACK
> +
> +    NS_DECL_THREADSAFE_ISUPPORTS
> +
> +    void terminate();

Nit: Methods should start with capital letters.

::: dom/webidl/FileReader.webidl
@@ +13,5 @@
>  [Constructor]
>  interface FileReader : EventTarget {
>    // async read methods
>    [Throws]
> +  void readAsArrayBuffer(any blob);

Let's please file a followup bug to change these all back to 'Blob' once we have converted Blob to WebIDL.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 14 • 10 years ago

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

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

This looks like a really great start! It's not quite enough though.

The main problem is that the code as written here does not hold the worker alive while the file reader is running. Consider this case:

  // worker.js script
  var reader = new FileReader();
  reader.onload = function(event) {
    // do something else
  };
  reader.readAsString("http://foo.com/myFile");

If that's all that the script does then the worker will stop running JS, at which point the worker's "busy count" will drop to 0. When the worker is not busy it is subject to GC, and we might collect it before the file reader completes.

The way we fix this is by registering the file reader as a "worker feature". That ensures that the busy count never drops to 0.

There's also this problem:

  // Same worker.js script from above
  // ...
  reader = null;

In this case the FileReader object can be collected before the data has been loaded. Ordinarily Necko helps us out here by holding a reference to the thing receiving the data, but since your delegate uses a weak reference that won't happen now so you need to fix that too. Hopefully we can just get rid of the delegate entirely, see below.

::: content/base/src/FileIOObject.h
@@ +94,5 @@
> +
> +  nsCOMPtr<nsIEventTarget> mTarget;
> +
> +protected:
> +  class FileReaderCallbackDelegate : public nsIInputStreamCallback {

Hrm, I'd much prefer us to fix the input stream stuff to always call the callbacks on the right thread without the need for threadsafe refcounting. This should be pretty easy I think and we wouldn't need to have delegates like this.

::: content/base/src/nsDOMFileReader.cpp
@@ +123,5 @@
>  nsDOMFileReader::Constructor(const GlobalObject& aGlobal, ErrorResult& aRv)
>  {
>    nsRefPtr<nsDOMFileReader> fileReader = new nsDOMFileReader();
>  
> +  fileReader->mIsMainThread = NS_IsMainThread();

I think this should be moved into the nsDOMFileReader constructor. Setting mTarget and mCallback too. Basically anything that can't fail.

@@ +136,5 @@
> +
> +    fileReader->BindToOwner(owner);
> +  } else {
> +    JSContext* cx = aGlobal.GetContext();
> +    WorkerPrivate* workerPrivate = GetWorkerPrivateFromContext(cx);

Please assert that workerPrivate is non-null here.

@@ +137,5 @@
> +    fileReader->BindToOwner(owner);
> +  } else {
> +    JSContext* cx = aGlobal.GetContext();
> +    WorkerPrivate* workerPrivate = GetWorkerPrivateFromContext(cx);
> +    nsIGlobalObject *owner = workerPrivate->GlobalScope();

Nit: * on the left here, or just lose the stack var and move this to the BindToOwner call.

@@ +542,5 @@
>  
> +nsIDOMBlob*
> +nsDOMFileReader::convertToBlob(JSContext* cx,
> +                               JS::Handle<JS::Value> aBlob,
> +                               ErrorResult& aRv) {

Nit: { on its own line.

@@ +543,5 @@
> +nsIDOMBlob*
> +nsDOMFileReader::convertToBlob(JSContext* cx,
> +                               JS::Handle<JS::Value> aBlob,
> +                               ErrorResult& aRv) {
> +  const JS::Value& val = aBlob.get();

Is this needed for something? I think you should be able to simply pass 'aBlob' anywhere you currently use 'val'...

@@ +550,5 @@
> +    return nullptr;
> +  }
> +  if (mIsMainThread) {
> +    nsIDOMBlob* ret;
> +    nsRefPtr<nsIDOMBlob> retHolder;

This should use nsCOMPtr.

::: content/base/src/nsDOMFileReader.h
@@ +64,5 @@
>    virtual JSObject* WrapObject(JSContext* aCx) MOZ_OVERRIDE;
>  
> +private:
> +  nsIDOMBlob* convertToBlob(JSContext* cx, JS::Handle<JS::Value> aBlob, ErrorResult& aRv);
> +  bool mIsMainThread;

Nit: Please keep all your members in the same section, methods too.

@@ +84,5 @@
>      MOZ_ASSERT(aBlob);
>      ReadFileContent(nullptr, aBlob, aLabel, FILE_AS_TEXT, aRv);
>    }
>  
> +  void ReadAsDataURLTypeChecked(nsIDOMBlob* aBlob, ErrorResult& aRv)

These shouldn't be public any longer I don't think.

@@ +186,5 @@
>  
>    JS::Heap<JSObject*> mResultArrayBuffer;
> +
> +private:
> +  class FileReaderWorkerEventTarget MOZ_FINAL : public nsIEventTarget {

This (and the runnable class) can just live in the cpp entirely, right? No need for them to be declared in the header here.

	Shihua Zheng
	Comment 15 • 10 years ago

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #14)
> In this case the FileReader object can be collected before the data has been
> loaded. Ordinarily Necko helps us out here by holding a reference to the
> thing receiving the data, but since your delegate uses a weak reference that
> won't happen now so you need to fix that too. Hopefully we can just get rid
> of the delegate entirely, see below.
> 
> ::: content/base/src/FileIOObject.h
> @@ +94,5 @@
> > +
> > +  nsCOMPtr<nsIEventTarget> mTarget;
> > +
> > +protected:
> > +  class FileReaderCallbackDelegate : public nsIInputStreamCallback {
> 
> Hrm, I'd much prefer us to fix the input stream stuff to always call the
> callbacks on the right thread without the need for threadsafe refcounting.
> This should be pretty easy I think and we wouldn't need to have delegates
> like this.

Thread-safe refcounting is still needed, I believe, because the stream thread needs to access it. That is the reason I add this class to delegate the call.

Other problems are fixed.

	Shihua Zheng
	Comment 16 • 10 years ago

> There's also this problem:
> 
>   // Same worker.js script from above
>   // ...
>   reader = null;
> 
> In this case the FileReader object can be collected before the data has been
> loaded. Ordinarily Necko helps us out here by holding a reference to the
> thing receiving the data, but since your delegate uses a weak reference that
> won't happen now so you need to fix that too. Hopefully we can just get rid
> of the delegate entirely, see below.
> 

The problem actually happens. There are many test failures because of that. Today I took time debugging and found that this was the cause. I have fixed this problem.

	Shihua Zheng
	Comment 17 • 10 years ago

Added WorkerFeature. Fixed other problems.

	Shihua Zheng
	Comment 21 • 10 years ago

Try run: https://tbpl.mozilla.org/?tree=Try&rev=b72f8d0ef390

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 22 • 10 years ago

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

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

Hrm, it looks like the way this works now is that all the network stuff bounces through the main thread, and the timer will be called on the main thread too? We really shouldn't need that kind of thread gymnastics here...

	Shihua Zheng
	Comment 23 • 10 years ago

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #22)
> Comment on attachment 8450403 [details] [diff] [review]
> Patch
> 
> Review of attachment 8450403 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hrm, it looks like the way this works now is that all the network stuff
> bounces through the main thread, and the timer will be called on the main
> thread too? We really shouldn't need that kind of thread gymnastics here...

They all run on the current thread. The delegate object is used because the FileIOObject is not thread-safe.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 24 • 10 years ago

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

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

Ah, I read too quickly. I understand how this is working now. But it's very confusing because the two classes are split poorly.

At this point it seems like we should merge the two classes. Do you agree?

	Shihua Zheng
	Comment 25 • 10 years ago

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #24)
> Comment on attachment 8450403 [details] [diff] [review]
> Patch
> 
> Review of attachment 8450403 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ah, I read too quickly. I understand how this is working now. But it's very
> confusing because the two classes are split poorly.
> 
> At this point it seems like we should merge the two classes. Do you agree?

Yeah, we should. How about filing a bug?

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 26 • 10 years ago

Well, I think it should probably be a separate patch on this bug actually. I don't think we want to land them separately...

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 27 • 10 years ago

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

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

Ok, with these things fixed (and the classes merged) I think this should work.

::: content/base/src/FileIOObject.cpp
@@ +62,5 @@
>      mTimerIsActive(false),
>      mReadyState(0),
> +    mTotal(0), mTransferred(0),
> +    mBusyCount(0),
> +    mTarget(do_GetMainThread()),

This should probably be do_GetCurrentThread.

@@ +76,5 @@
>    if (mProgressNotifier) {
>      mProgressEventWasDelayed = false;
>      mTimerIsActive = true;
>      mProgressNotifier->Cancel();
> +    mProgressNotifier->SetTarget(mTarget);

I guess you could MOZ_ASSERT_IF(mWorkerPrivate, mTarget != NS_GetCurrentThread()) here to make sure this is correct.

@@ +158,2 @@
>  {
> +  if (mWorkerPrivate && --mBusyCount == 0) {

Please first assert |mBusyCount > 0| (assuming mWorkerPrivate is non-null).

@@ +201,5 @@
> +FileIOObject::Notify(JSContext* aCx, workers::Status aStatus)
> +{
> +  MOZ_ASSERT(mWorkerPrivate);
> +  mWorkerPrivate->AssertIsOnWorkerThread();
> +  MOZ_ASSERT(mWorkerPrivate->GetJSContext() == aCx);

This doesn't need to be asserted, it's basically a law :)

@@ +203,5 @@
> +  MOZ_ASSERT(mWorkerPrivate);
> +  mWorkerPrivate->AssertIsOnWorkerThread();
> +  MOZ_ASSERT(mWorkerPrivate->GetJSContext() == aCx);
> +
> +  if (aStatus >= workers::Canceling) {

|> Running| is probably better.

@@ +253,5 @@
> +  mCallback = new FileReaderCallbackDelegate(this);
> +  nsresult rv = aStream->AsyncWait(mCallback,
> +                                   /* aFlags*/ 0,
> +                                   /* aRequestedCount */ 0,
> +                                   mTarget);

I guess you could MOZ_ASSERT_IF(mWorkerPrivate, mTarget != NS_GetCurrentThread()) here to make sure this is correct.

@@ +298,5 @@
> +{
> +  if (!mFileIOObj) {
> +    return NS_ERROR_FAILURE;
> +  }
> +  nsresult ret = mFileIOObj->OnStreamCallback(aStream);

Nit: No need for the extra stack variable, just return the result directly.

::: content/base/src/nsDOMFileReader.cpp
@@ +160,5 @@
> +    JSContext* cx = aGlobal.Context();
> +    WorkerPrivate* workerPrivate = GetWorkerPrivateFromContext(cx);
> +    MOZ_ASSERT(workerPrivate);
> +    fileReader->mWorkerPrivate = workerPrivate;
> +    nsIGlobalObject* owner = workerPrivate->GlobalScope();

Please assert this.

@@ +162,5 @@
> +    MOZ_ASSERT(workerPrivate);
> +    fileReader->mWorkerPrivate = workerPrivate;
> +    nsIGlobalObject* owner = workerPrivate->GlobalScope();
> +    fileReader->BindToOwner(owner);
> +    fileReader->mTarget = new FileReaderWorkerEventTarget(workerPrivate->GetEventTarget());

You need to check that |workerPrivate->GetEventTarget()| succeeds, I think it can return null in some cases and you'd need to handle that.

@@ +561,5 @@
>    return NS_OK;
>  }
>  
> +nsIDOMBlob*
> +nsDOMFileReader::convertToBlob(JSContext* cx,

Please make sure we have a followup filed to remove this once the WebIDL bindings handle Blobs on workers just like the main thread.

::: content/base/src/nsDOMFileReader.h
@@ +92,5 @@
> +    MOZ_ASSERT(aBlob);
> +    ReadFileContent(nullptr, aBlob, EmptyString(), FILE_AS_BINARY, aRv);
> +  }
> +
> +  nsIDOMBlob* convertToBlob(JSContext* cx, JS::Handle<JS::Value> aBlob, ErrorResult& aRv);

Nit: Please capitalize that, "ConvertToBlob"

	Andrew Sutherland [:asuth] (he/him) Reporter
	Comment 28 • 10 years ago

For clarity, even though we're implementing the spec, is the idea still that we would prefer to direct people towards XHR and createObjectURL?  Context is the quotes below, and my interest is as a Firefox OS developer and Blob aficionado.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #2)
> Jonas has a plan to deprecate FileReader, IIRC.

(In reply to Jonas Sicking (:sicking) Vacation until July 24 from comment #3)
> See proposed API in
> http://lists.w3.org/Archives/Public/public-webapps/2013AprJun/0727.html
> 
> So yeah, I could see punting on this if the new proposal gets any momentum.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 29 • 10 years ago

sicking, asuth meant to ni? you :)

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 30 • 10 years ago

No, I would definitely encourage people to use FileReader when it's available. createObjectURL both carries the risk of leaks (if you forget to release) and our XHR implementation has a higher overhead (though that could in theory be fixed, though that's unlikely to happen anytime soon.)

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 32 • 10 years ago

Comment on attachment 8461705 [details] [diff] [review]
Patch - Merge FileIOObject into nsDOMFileReader

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

This looks great! Please fix these little issues and then I think we're good to go!

::: content/base/src/nsDOMFileReader.cpp
@@ +7,5 @@
>  
> +#include "mozilla/EventDispatcher.h"
> +#include "nsIDOMEvent.h"
> +#include "mozilla/dom/ProgressEvent.h"
> +#include "nsComponentManagerUtils.h"

Nit: Can you merge and alphabetize these includes with the others?

@@ +51,5 @@
> +
> +class FileReaderCallbackDelegate MOZ_FINAL : public nsIInputStreamCallback
> +{
> +public:
> +  FileReaderCallbackDelegate(nsRefPtr<nsDOMFileReader> aFileReader) : mFileReader(aFileReader) {}

Please make the argument type 'nsDOMFileReader*'. The way you have there constructs a temporary nsRefPtr which leads to a hidden AddRef/Release pair.

@@ +56,5 @@
> +
> +  NS_DECL_NSIINPUTSTREAMCALLBACK
> +  NS_DECL_THREADSAFE_ISUPPORTS
> +
> +protected:

Nit: private, it is not possible to have derived classes since this class is MOZ_FINAL.

@@ +57,5 @@
> +  NS_DECL_NSIINPUTSTREAMCALLBACK
> +  NS_DECL_THREADSAFE_ISUPPORTS
> +
> +protected:
> +  virtual ~FileReaderCallbackDelegate() {}

This does not need to be virtual.

@@ +66,4 @@
>  
>  class FileReaderWorkerEventTarget MOZ_FINAL : public nsIEventTarget {
>  public:
> +  FileReaderWorkerEventTarget(nsCOMPtr<nsIEventTarget> aEventTarget) : mEventTarget(aEventTarget)

Same here, please use 'nsIEventTarget*' as the arg type.

@@ +73,5 @@
>  
>    NS_DECL_NSIEVENTTARGET
>    NS_DECL_THREADSAFE_ISUPPORTS
>  
> +protected:

Nit: private

@@ +74,5 @@
>    NS_DECL_NSIEVENTTARGET
>    NS_DECL_THREADSAFE_ISUPPORTS
>  
> +protected:
> +  virtual ~FileReaderWorkerEventTarget() {}

This does not need to be virtual.

@@ +89,5 @@
>    NS_DECL_THREADSAFE_ISUPPORTS
>  
>    NS_FORWARD_NSIRUNNABLE(mRunnable->)
>  
> +protected:

Nit: private

@@ +90,5 @@
>  
>    NS_FORWARD_NSIRUNNABLE(mRunnable->)
>  
> +protected:
> +  virtual ~FileReaderWorkerRunnable() {}

This does not need to be virtual.

@@ +111,2 @@
>    tmp->mResultArrayBuffer = nullptr;
>    NS_IMPL_CYCLE_COLLECTION_UNLINK(mFile)

You also need to unlink mProgressNotifier and mError here.

@@ +576,5 @@
> +    MOZ_ASSERT_IF(mWorkerPrivate, mTarget != NS_GetCurrentThread());
> +    mProgressEventWasDelayed = false;
> +    mTimerIsActive = true;
> +    mProgressNotifier->Cancel();
> +    mProgressNotifier->SetTarget(mTarget);

Let's move this SetTarget call (and the assertion about the worker target) up to right after we create the timer. We don't want to keep setting the target every time we start the timer.

@@ +782,5 @@
> +  MOZ_ASSERT_IF(mWorkerPrivate, mTarget != NS_GetCurrentThread());
> +  if (mWorkerPrivate && mBusyCount++ == 0 &&
> +      !mWorkerPrivate->AddFeature(mWorkerPrivate->GetJSContext(),
> +                                  this)) {
> +    return NS_ERROR_FAILURE;

You should probably reduce mBusyCount before returning.

::: content/base/src/nsDOMFileReader.h
@@ +10,5 @@
> +#include "nsIFile.h"
> +#include "nsITimer.h"
> +#include "nsIAsyncInputStream.h"
> +#include "mozilla/dom/workers/bindings/WorkerFeature.h"
> +#include "mozilla/dom/DOMError.h"

Nit: Can you merge and alphabetize these includes with the others?

@@ +33,5 @@
>  #include "nsIEventTarget.h"
>  #include "nsIRunnable.h"
>  #include "nsISupports.h"
>  
> +#define NS_PROGRESS_EVENT_INTERVAL 50

Nit: This (and the other #defines below) should move to the cpp since they're not used in the header.

@@ +49,5 @@
> +namespace workers {
> +  class WorkerPrivate;
> +}
> +
> +extern const uint64_t kUnknownSize;

This doesn't need to be exposed as an extern here anymore, it can just live in the cpp in an anonymous namespace.

@@ +56,5 @@
> +}
> +
> +using namespace mozilla;
> +using namespace mozilla::dom;
> +using namespace mozilla::dom::workers;

We don't allow "using" statements in headers because.

@@ +60,5 @@
> +using namespace mozilla::dom::workers;
> +
> +class nsDOMFileReader : public DOMEventTargetHelper,
> +                        public nsITimerCallback,
> +                        public WorkerFeature,

Nit: Might want to move this last instead of the middle of the interface vtables.

@@ +98,2 @@
>  
> +  IMPL_EVENT_HANDLER(abort)

Let's keep these together with the others below ("load", "loadstart").

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 34 • 10 years ago

Comment on attachment 8465040 [details] [diff] [review]
Patch: Merger

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

It doesn't look like the changes I was talking about in nsDOMFileReader.h were made (see comment 32). Please make those here.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)
	Comment 36 • 9 years ago

szheng, are you still working on this?

	Shihua Zheng
	Comment 37 • 9 years ago

Hi, Nikhil. I have merged the recent changes since August 2013, but still struggling to get a try run.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)
	Comment 38 • 9 years ago

(In reply to szheng from comment #37)
> Hi, Nikhil. I have merged the recent changes since August 2013, but still
> struggling to get a try run.

Let me know if I can help in any way. What problems are you facing?

	Shihua Zheng
	Comment 39 • 9 years ago

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #38)
> (In reply to szheng from comment #37)
> > Hi, Nikhil. I have merged the recent changes since August 2013, but still
> > struggling to get a try run.
> 
> Let me know if I can help in any way. What problems are you facing?

It used to be okay, but the central repo's build is broken as of now so I have to wait. For this bug, I have 2 issues now:

1. Static analysis is unhappy with my implicit casting (e.g: merger patch nsDOMFileReader.cpp:67.
2. Chrome test test_fileconstructor_tempfile.xul fails to delete the temp file.

Need some time to fix these.

	Jakub Mareda
	Comment 40 • 9 years ago

Come on, it's been 2 year, you can't be serious. One of the primary purposes of adding WebWorkers was to allow operations upon big chunks of data without lagging the GUI thread. So how can we be missing FileReader?

	adria
	Comment 41 • 9 years ago

ServiceWorker is not allow to expose FileReaderSync (or in fact any synchronous functions).
Without WorkerGlobalScope.FileReader, this leaves ServiceWorker with no way to actually read a blob.

	(no longer active)
	Comment 42 • 9 years ago

This doesn't block shipping service workers.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 43 • 9 years ago

Andrea, would you be interested in helping finish this one up?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 45 • 9 years ago

Comment on attachment 8696958 [details] [diff] [review]
patch

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

Someone else should review the XPCOM changes

::: dom/base/FileReader.cpp
@@ +116,2 @@
>  {
> +  MOZ_ASSERT_IF(!NS_IsMainThread(), mWorkerPrivate);

Maybe
MOZ_ASSERT_IF(!NS_IsMainThread(), mWorkerPrivate && !aWindow);
MOZ_ASSERT_IF(NS_IsMainThread(), !mWorkerPrivate && aWindow);

?

@@ +604,5 @@
>    if (mReadyState != LOADING || aStream != mAsyncStream) {
>      return NS_OK;
>    }
>  
> +  // We use this class to decrease the busy counter at the end of this method.

Might want to comment on why you can't just decrease the counter here.

	Andrea Marchesini [:baku] Assignee
	Comment 46 • 9 years ago

Comment on attachment 8696958 [details] [diff] [review]
patch

Nathan, what me and Jonas would like you to review is the XPCOM changes.
The reason why I touched those runnables is because, in order to work in workers, they must be 'cancelable'.

	Nathan Froyd [:froydnj]
	Comment 47 • 9 years ago

Comment on attachment 8696958 [details] [diff] [review]
patch

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

Brief comments only, as I'm not in a position to do a deep dive on this currently.

::: dom/base/FileReader.cpp
@@ +302,5 @@
> +
> +  rv = mAsyncStream->AsyncWait(this,
> +                               /* aFlags*/ 0,
> +                               /* aRequestedCount */ 0,
> +                               mTarget);

So this AsyncWait...

@@ +520,5 @@
>    if (mProgressNotifier) {
>      mProgressEventWasDelayed = false;
>      mTimerIsActive = true;
>      mProgressNotifier->Cancel();
> +    mProgressNotifier->SetTarget(mTarget);

...and this SetTarget will wind up directing events from the stream waiting (reading?) and the timer, respectively, onto the worker thread, where they'll get picked up by the worker event loop?

::: xpcom/io/nsStreamUtils.cpp
@@ +94,5 @@
>      }
>      return NS_OK;
>    }
>  
> +  NS_IMETHOD Cancel() override

I think all these nsICancelableRunnable/Cancel implementation deserve some commentary on why we're doing them.

@@ +444,5 @@
>  
>      return NS_OK;
>    }
>  
> +  NS_IMETHOD Cancel() override

Especially this one.

	Andrea Marchesini [:baku] Assignee
	Comment 48 • 9 years ago

> ...and this SetTarget will wind up directing events from the stream waiting
> (reading?) and the timer, respectively, onto the worker thread, where
> they'll get picked up by the worker event loop?

Right. The real operation will be executed in the IO thread, but then nsICancelableRunnables will be dispatched to the mTarget thread (main-thread or workers). For workers, we support only nsICancelableRunnable because these can be canceled in case in worker is terminated.

I'll submit a new patch with better comments.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 50 • 9 years ago

I think one thing to watch out for is: Does the code which create these runnable rely on that they are called under all circumstances?

For example, do we allocate some resources, or addref some object, which is only freed/released when the runnable runs?

So for example, does the timer implementation code itself hold on to some resource and only release it once the timer fires? Obviously the code which created the timer (in this case the FileReader code) might hold on to some resource that need to be freed if the timer is cancelled, but the patch seems to handle that fine.

The question is if the timer implementation itself holds on to some resource which is only released once the runnable runs?

Similarly, does the AsyncInputStream code hold on to some resource related to the stream that is only released once the runnable runs and it can call the registered callback?

	Andrea Marchesini [:baku] Assignee
	Comment 51 • 9 years ago

(In reply to Jonas Sicking (:sicking) from comment #50)
> I think one thing to watch out for is: Does the code which create these
> runnable rely on that they are called under all circumstances?

Indeed, this is the point. And in case this code doesn't rely on that, we must change it because, exposing it to workers, we introduce the possibility that everything is terminated before calling Run() - calling Cancel() instead.

	Nathan Froyd [:froydnj]
	Comment 52 • 9 years ago

Comment on attachment 8697118 [details] [diff] [review]
patch

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

Sorry for the delay.  Jonas's comment 50 is well-taken.

Canceling the timer is safe, AFAICT, because the timer itself is removed from the global list of timers once the timer event is posted to a run loop somewhere.  And destroying the timer will release any references that it holds.

The stream events look essentially cancelable anyway, and the only references they hold are to smart pointers, so the normal destructor/Release process should ensure those resources are taken care of.

I have a hard time believing that some of these Cancel implementations are correct, or the Run implementations are correct now that things are cancelable.  Documentation for nsICancelableRunnable is a little thin, but my mental model is that Run() must always function correctly if Cancel() is called prior to it, and it doesn't look like that's true for nsTimerEvent or nsAStreamCopier.  (I can believe that Run() might not be called after Cancel() in the workers case (?), but I don't think we should limit ourselves to the worker case here; we should code to the interface, not our specific use.)

::: xpcom/io/nsStreamUtils.cpp
@@ +25,5 @@
>  
>  //-----------------------------------------------------------------------------
>  
> +// This is a nsICancelableRunnable because we can dispatch it to Workers and
> +// those can be shutted down at any time, and in these cases, Cancel() is called

Nit: "those can be shut down..."

@@ +26,5 @@
>  //-----------------------------------------------------------------------------
>  
> +// This is a nsICancelableRunnable because we can dispatch it to Workers and
> +// those can be shutted down at any time, and in these cases, Cancel() is called
> +// instead Run().

Nit: "instead of Run()"

Please apply the same changes to the comment elsewhere.

@@ +450,5 @@
>  
>      return NS_OK;
>    }
>  
> +  NS_IMETHOD Cancel() override

This method should be MOZ_MUST_OVERRIDE, as the behavior here can't be correct in general, right?  Please declare this as a pure MOZ_MUST_OVERRIDE virtual, and please add the appropriate overriding implementations to the subclasses as well.

@@ +452,5 @@
>    }
>  
> +  NS_IMETHOD Cancel() override
> +  {
> +    return NS_OK;

Don't we need to set some flags or release some resources so that Run() won't actually do anything?

::: xpcom/threads/TimerThread.cpp
@@ +134,5 @@
>  {
>  public:
> +  NS_IMETHOD Run() override;
> +
> +  NS_IMETHOD Cancel() override

Shouldn't we be checking |if (!mTimer)| in |Run()| for completeness and/or correctness?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 53 • 9 years ago

I'm not sure if Cancel() run's *instead of* Run() for a nsICancelableRunnable? Or if Run() is always called. We should definitely document.

I do know that the idea is to enable us to cancel a thread, and so what we want to accomplish is to spend minimal number of cycles in order to do cleanup.

	Andrea Marchesini [:baku] Assignee
	Comment 54 • 9 years ago

In workers, Run() is not executed after a Cancel():

http://mxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp#4462

I'll update the documentation of nsICancelableRunnable.

	Nathan Froyd [:froydnj]
	Comment 56 • 9 years ago

Comment on attachment 8698416 [details] [diff] [review]
patch

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

::: xpcom/threads/nsICancelableRunnable.idl
@@ +15,5 @@
>  {
>      /**
>       * Cancels a pending task.  If the task has already been executed this will
>       * be a no-op.  Calling this method twice is considered an error.
> +     * If cancel() is called, run() will not be called.

I have my doubts about being able to enforce this, but I guess since we only really use this for workers and other places can use nsCancelableRunnable...

	Pulsebot
	Comment 57 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/d74d1e794421

	Carsten Book [:Tomcat]
	Comment 58 • 9 years ago

sorry backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=18633028&repo=mozilla-inbound

	Pulsebot
	Comment 59 • 9 years ago

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72bcc6afaaf4

	Pulsebot
	Comment 60 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/84c8783b8518

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 61 • 9 years ago

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/4723e0d81721 for crashes on android and b2g:

https://treeherder.mozilla.org/logviewer.html#?job_id=18652815&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=18649878&repo=mozilla-inbound

	Pulsebot
	Comment 62 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/30839ee209e8

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 63 • 9 years ago

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/15880ddc54b9 for crashes in mochitests on Android and Mulet.

https://treeherder.mozilla.org/logviewer.html#?job_id=18713814&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=18713839&repo=mozilla-inbound

04:27:38     INFO -  233 INFO TEST-START | dom/base/test/test_fileapi.html
04:27:38     INFO -  INFO | automation.py | Application ran for: 0:07:08.434010
04:27:38     INFO -  INFO | zombiecheck | Reading PID log: /tmp/tmpSa9Zcapidlog
04:27:39     INFO -  /data/tombstones does not exist; tombstone check skipped
04:27:39     INFO -  mozcrash Downloading symbols from: https://queue.taskcluster.net/v1/task/xY26X4KeQCCJmHBRH83qsA/artifacts/public/build/fennec-46.0a1.en-US.android-arm.crashreporter-symbols.zip
04:27:48     INFO -  mozcrash Saved minidump as /builds/slave/test/build/blobber_upload_dir/62cbd589-0583-4c99-103fd0f0-036c56c5.dmp
04:27:48     INFO -  mozcrash Saved app info as /builds/slave/test/build/blobber_upload_dir/62cbd589-0583-4c99-103fd0f0-036c56c5.extra
04:27:48  WARNING -  PROCESS-CRASH | dom/base/test/test_fileapi.html | application crashed [@ 0xabaaa9a8]
04:27:48     INFO -  Crash dump filename: /tmp/tmptIdakf/62cbd589-0583-4c99-103fd0f0-036c56c5.dmp
04:27:48     INFO -  Operating system: Android
04:27:48     INFO -                    0.0.0 Linux 2.6.29-gea477bb #1 Wed Sep 26 11:04:45 PDT 2012 armv7l generic/sdk/generic:4.3.1/JLS36I/eng.gbrown.20150308.182649:eng/test-keys
04:27:48     INFO -  CPU: arm
04:27:48     INFO -       ARMv0
04:27:48     INFO -       0 CPUs
04:27:48     INFO -  Crash reason:  SIGSEGV
04:27:48     INFO -  Crash address: 0xabaaa9a8
04:27:48     INFO -  Process uptime: not available
04:27:48     INFO -  Thread 12 (crashed)
04:27:48     INFO -   0  0xabaaa9a8
04:27:48     INFO -       r0 = 0xabaaa9a8    r1 = 0x67e6e0b0    r2 = 0x00000002    r3 = 0xffffff88
04:27:48     INFO -       r4 = 0x67e651a0    r5 = 0xffffff88    r6 = 0x60080021    r7 = 0xffffff82
04:27:48     INFO -       r8 = 0x00000a04    r9 = 0x60eb40a8   r10 = 0x00000006   r12 = 0x5fc05550
04:27:48     INFO -       fp = 0x52bfe144    sp = 0x52bfe0e8    lr = 0x602154a4    pc = 0xabaaa9a8
04:27:48     INFO -      Found by: given as instruction pointer in context
04:27:48     INFO -   1  libxul.so!nsXPCConstructor::CallOrConstruct [XPCComponents.cpp:30839ee209e8 : 1948 + 0x9]
04:27:48     INFO -       sp = 0x52bfe138    pc = 0x59f86649
04:27:48     INFO -      Found by: stack scanning
04:27:48     INFO -  Thread 0
04:27:48     INFO -   0  libc.so + 0x1c3dc
04:27:48     INFO -       r0 = 0xfffffffc    r1 = 0xbefeb4a8    r2 = 0x00000010    r3 = 0xffffffff
04:27:48     INFO -       r4 = 0x2a040aa0    r5 = 0x00000000    r6 = 0x2a040ab4    r7 = 0x000000fc
04:27:48     INFO -       r8 = 0x2a040ae8    r9 = 0x00000014   r10 = 0x2a00d0a0   r12 = 0xbefeb4a8
04:27:48     INFO -       fp = 0xbefeb61c    sp = 0xbefeb488    lr = 0x400b1fb5    pc = 0x400433dc
04:27:48     INFO -      Found by: given as instruction pointer in context
04:27:48     INFO -   1  libwebcore.so + 0x234fff
04:27:48     INFO -       sp = 0xbefeb4b0    pc = 0x51010001
04:27:48     INFO -      Found by: stack scanning
04:27:48     INFO -   2  libc.so!dlfree [malloc.c : 4722 + 0x7]
04:27:48     INFO -       sp = 0xbefeb4c8    pc = 0x400379eb
04:27:48     INFO -      Found by: stack scanning
04:27:48     INFO -   3  libgui.so!android::ScopedTrace::~ScopedTrace() [trace.h : 159 + 0x3]
04:27:48     INFO -       r3 = 0x4006f000    r4 = 0x2a3e31b8    r5 = 0x2a3e31b8    r6 = 0x00000090
04:27:48     INFO -       r7 = 0x00000000    sp = 0xbefeb4e0    pc = 0x40572d73
04:27:48     INFO -      Found by: call frame info

	Carsten Book [:Tomcat]
	Comment 64 • 9 years ago

can we get a try run before we land this again ? This bounced now 3 times so it might be worth to check this changes before trying to commit again

	Andrea Marchesini [:baku] Assignee
	Comment 65 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=233da98d7517&selectedJob=14718066

	Pulsebot
	Comment 66 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/6848f6651cce

	Carsten Book [:Tomcat]
	Comment 67 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/6848f6651cce

	Jean-Yves Perrier [:teoli]
	Comment 68 • 8 years ago

Updated:
https://developer.mozilla.org/en-US/docs/Web/API/FileReader
https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Functions_and_classes_available_to_workers#APIs_available_in_workers
and
https://developer.mozilla.org/en-US/Firefox/Releases/46#Miscellaneous