1017765 - FileReader: Deprecate Necko channel from DOMFileReader; use StreamTransportService instead

Status: RESOLVED FIXED

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

Description

[Shihua Zheng]

Currently, we use necko (nsChannel) to implement FileReader API. We propose a change to use StreamTransportService. The benefit is that the support of FileReader on web worker could be easier.

Comment 1

[Shihua Zheng]

Attachment: Patch

Comment 2

[Shihua Zheng]

Link to try run: https://tbpl.mozilla.org/?tree=Try&rev=197cbfff857a

Comment 3

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

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

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

::: content/base/src/FileIOObject.cpp
@@ +148,3 @@
>  NS_IMETHODIMP
> +FileIOObject::OnInputStreamReady(nsIAsyncInputStream* aStream) {
> +  if (mReadyState != 1 || !IsCurrentStream(aStream)) {

This shouldn't be needed, so let's file a followup bug to make canceling a stream never call back after.

@@ +154,2 @@
>    nsresult rv;
> +  uint64_t aCount = 0;

No need to set aCount here, and just name it |count|.

@@ +154,3 @@
>    nsresult rv;
> +  uint64_t aCount = 0;
> +  rv = aStream->Available(&aCount);

How about:

  if (rv == NS_BASE_STREAM_CLOSED) {
    MOZ_ASSERT(!count);
    rv = NS_OK;
  }

@@ +154,5 @@
>    nsresult rv;
> +  uint64_t aCount = 0;
> +  rv = aStream->Available(&aCount);
> +  
> +  if (aCount) {

if (NS_SUCCEEDED(rv) && count)

@@ +155,5 @@
> +  uint64_t aCount = 0;
> +  rv = aStream->Available(&aCount);
> +  
> +  if (aCount) {
> +    nsresult rvr = DoReadData(aStream, aCount);

Now there's no need for |rvr|, just do:

  rv = DoReadData(aStream, aCount);

@@ +160,5 @@
> +    if (NS_FAILED(rvr)) rv = rvr;
> +  }
> +  
> +  // If stream is closed (either success or failure), conclude this transfer
> +  if (NS_FAILED(rv)) {

This should probably be |!count || NS_FAILED(rv)|

@@ +164,5 @@
> +  if (NS_FAILED(rv)) {
> +    return OnLoadEnd(rv);
> +  }
> +  
> +  DoAsyncWait(aStream);

You should check the return value from this, and call OnLoadEnd if it fails

@@ +181,5 @@
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +FileIOObject::OnLoadEnd(nsresult aStatus)

This should s/NS_IMETHODIMP/nsresult/

@@ +195,1 @@
>    NS_ENSURE_SUCCESS(rv, rv);

Let's file a followup for making sure we always dispatch an error event if DoOnLoadEnd fails.

@@ +195,4 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    // Set the status field as appropriate
> +  if (NS_FAILED(aStatus) && aStatus != NS_BASE_STREAM_CLOSED) {

The extra check shouldn't be necessary now.

@@ +206,5 @@
>  
>    return NS_OK;
>  }
>  
> +bool FileIOObject::DoAsyncWait(nsIAsyncInputStream* aStream)

Nit: return types are usually on their own line

@@ +210,5 @@
> +bool FileIOObject::DoAsyncWait(nsIAsyncInputStream* aStream)
> +{
> +  nsresult rv;
> +  nsCOMPtr<nsIThread> currentThread = do_GetCurrentThread();
> +  rv = aStream->AsyncWait(this, 0, 0, currentThread);

You can remove the extra nsCOMPtr and just call NS_GetCurrentThread():

  rv = aStream->AsyncWait(this, 0, 0, NS_GetCurrentThread());

::: content/base/src/FileIOObject.h
@@ +15,4 @@
>  
>  #include "mozilla/dom/DOMError.h"
>  
> +

Nit: Extra line, please remove.

@@ +70,5 @@
>    virtual void DoAbort(nsAString& aEvent) = 0;
> +  
> +  NS_IMETHOD DoReadData(nsIAsyncInputStream *aStream, uint64_t aCount) = 0;
> +  
> +  NS_IMETHOD OnLoadEnd(nsresult aStatus);

It doesn't look like this needs to be virtual, and private.

@@ +74,5 @@
> +  NS_IMETHOD OnLoadEnd(nsresult aStatus);
> +  
> +  NS_IMETHOD DoOnLoadEnd(nsresult aStatus, nsAString& aSuccessEvent,
> +                         nsAString& aTerminationEvent) = 0;
> +                         

Nit: Extra spaces here and elsewhere, please remove

@@ +77,5 @@
> +                         nsAString& aTerminationEvent) = 0;
> +                         
> +  virtual bool IsCurrentStream(nsIAsyncInputStream* aStream) = 0;
> +
> +  bool DoAsyncWait(nsIAsyncInputStream* aStream);

Let's make this return nsresult.

::: content/base/src/nsDOMFileReader.cpp
@@ +46,5 @@
>  #include "jsfriendapi.h"
>  
> +#include "mozilla/dom/WorkerPrivate.h"
> +#include "mozilla/dom/WorkerScope.h"
> +#include "File.h"

Nit: These belong to a different patch.

@@ +55,3 @@
>  using namespace mozilla;
>  using namespace mozilla::dom;
> +using namespace mozilla::dom::workers;

Nit: This belongs in a different patch.

@@ +107,5 @@
>  
>  //nsDOMFileReader constructors/initializers
>  
>  nsDOMFileReader::nsDOMFileReader()
> +  : mAsyncStream(nullptr), mFileData(nullptr),

Nit: nsCOMPtrs don't need to be initialized

@@ +331,2 @@
>  {
> +  

Nit: Extra space here.

@@ +339,5 @@
>    aSuccessEvent = NS_LITERAL_STRING(LOAD_STR);
>    aTerminationEvent = NS_LITERAL_STRING(LOADEND_STR);
>  
>    // Clear out the data if necessary
> +  if (NS_FAILED(aStatus) && aStatus != NS_BASE_STREAM_CLOSED) {

This extra check shouldn't be needed now.

@@ +444,5 @@
>    mDataFormat = aDataFormat;
>    CopyUTF16toUTF8(aCharset, mCharset);
>  
> +  nsresult rv;
> +  nsCOMPtr<nsIStreamTransportService> sts = 

Nit: trailing space

@@ +447,5 @@
> +  nsresult rv;
> +  nsCOMPtr<nsIStreamTransportService> sts = 
> +    do_GetService(kStreamTransportServiceCID, &rv);
> +  if (NS_FAILED(rv)) {
> +    aRv.Throw(rv); return;

Nit: Only one statement per line, please do for all below.

@@ +457,5 @@
> +    aRv.Throw(rv); return;
> +  }
> +
> +  nsCOMPtr<nsITransport> transport;
> +  rv = sts->CreateInputTransport(stream, 0, -1, true, getter_AddRefs(transport));

Let's do:

  rv =  sts->CreateInputTransport(stream,
                                  /* aStartOffset */ 0,
                                  /* aReadLimit */ -1,
                                  /* aCloseWhenDone */ true,
                                  getter_AddRefs(transport));

@@ +463,5 @@
> +    aRv.Throw(rv); return;
> +  }
> +
> +  nsCOMPtr<nsIInputStream> wrapper;
> +  rv = transport->OpenInputStream(0, 0, 0, getter_AddRefs(wrapper));

Same here!

@@ +467,5 @@
> +  rv = transport->OpenInputStream(0, 0, 0, getter_AddRefs(wrapper));
> +  if (NS_FAILED(rv)) {
> +    aRv.Throw(rv); return;
> +  }
> +  

Nit: spaces

@@ +468,5 @@
> +  if (NS_FAILED(rv)) {
> +    aRv.Throw(rv); return;
> +  }
> +  
> +  mAsyncStream = do_QueryInterface(wrapper, &rv);

This one doesn't need to be error checked or use rv. You can MOZ_ASSERT that |mAsyncStream| is non-null.

Also, before setting, you can assert that |mAsyncStream| is null.

@@ +477,5 @@
>    mTotal = mozilla::dom::kUnknownSize;
>    mFile->GetSize(&mTotal);
>  
> +  if (!DoAsyncWait(mAsyncStream)) {
> +    aRv.Throw(rv); return;

Here |rv| will be a success code.

::: content/base/src/nsDOMFileReader.h
@@ +16,5 @@
>  #include "nsTArray.h"              
>  #include "nsIJSNativeInitializer.h"
>  #include "prtime.h"                
>  #include "nsITimer.h"              
> +#include "nsIAsyncInputStream.h"

You can just forward declare this.

@@ +27,4 @@
>  
>  #include "FileIOObject.h"
>  
> +

Nit: extra line here.

@@ +50,5 @@
>  
>    // FileIOObject overrides
>    virtual void DoAbort(nsAString& aEvent) MOZ_OVERRIDE;
> +  
> +  NS_IMETHOD DoReadData(nsIAsyncInputStream *aStream, uint64_t aCount) MOZ_OVERRIDE;

s/NS_IMETHOD/nsresult/

@@ +53,5 @@
> +  
> +  NS_IMETHOD DoReadData(nsIAsyncInputStream *aStream, uint64_t aCount) MOZ_OVERRIDE;
> +  
> +  NS_IMETHOD DoOnLoadEnd(nsresult aStatus, nsAString& aSuccessEvent,
> +                         nsAString& aTerminationEvent) MOZ_OVERRIDE;

Same here.

@@ +173,5 @@
>      mFileData = nullptr;
>      mDataLen = 0;
>    }
>  
> +  nsCOMPtr<nsIAsyncInputStream> mAsyncStream;

Let's move this to the the FileIOObject class. Then you can get rid of the IsCurrentStream() virtual function also.

@@ +183,5 @@
>  
>    eDataFormat mDataFormat;
>  
>    nsString mResult;
> +  

Nit: This looks like extra whitespace.

@@ +188,1 @@
>    nsCOMPtr<nsIPrincipal> mPrincipal;

As you noticed it looks like you can get rid of this now!

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

We'll need to revert these changes and move them into the patch for worker bindings.

Comment 4

[Shihua Zheng]

Attachment: Patch v2

Comment 5

[Shihua Zheng]

Attachment: Patch v2

Comment 6

[Shihua Zheng]

Attachment: Patch v3

Comment 7

[Shihua Zheng]

Attachment: Patch v4

Link to try run: https://tbpl.mozilla.org/?tree=Try&rev=0cb2230749dc

Comment 8

[Shihua Zheng]

Attachment: Patch v4

Comment 9

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

Comment on attachment 8432796 [details] [diff] [review]
Patch v4

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

This looks really good! Mostly cosmetic stuff to fix below, but a few problems also:

Oh, please revert as many whitespace-only changes as you can from this patch. We can do a followup patch that just does whitespace later.

::: content/base/src/FileIOObject.cpp
@@ +148,2 @@
>  NS_IMETHODIMP
> +FileIOObject::OnInputStreamReady(nsIAsyncInputStream* aStream) {

Nit: Please put this { on its own line.

@@ +153,4 @@
>  
> +  nsresult rv;
> +  uint64_t aCount;
> +  rv = aStream->Available(&aCount);

Nit: Move the definition in line with the assignment (e.g. |nsresult rv = aStream->Available(...);|

@@ +163,5 @@
> +    rv = DoAsyncWait(aStream);
> +  }
> +
> +  if (!aCount || NS_FAILED(rv)) {
> +    if (rv == NS_BASE_STREAM_CLOSED) rv = NS_OK;

Nit: if statements should always be braced and each statement should be on its own line.

@@ +213,5 @@
> +FileIOObject::DoAsyncWait(nsIAsyncInputStream* aStream)
> +{
> +  nsresult rv;
> +  rv = aStream->AsyncWait(this, 0, 0, NS_GetCurrentThread());
> +  return rv;

Nit: The stack variable is not used here.

::: content/base/src/FileIOObject.h
@@ +74,5 @@
> +
> +  NS_IMETHOD DoOnLoadEnd(nsresult aStatus, nsAString& aSuccessEvent,
> +                         nsAString& aTerminationEvent) = 0;
> +
> +  nsresult DoAsyncWait(nsIAsyncInputStream* aStream);

Nit: It would be nice to put all the pure virtual methods together and then have all the non-virtuals together below. That way it is easier for other developers to read and see what methods they need to implement in subclasses.

::: content/base/src/nsDOMFileReader.cpp
@@ +239,5 @@
> +
> +  if (mAsyncStream) {
> +    mAsyncStream->Close();
> +    nsCOMPtr<nsIAsyncInputStream> stream;
> +    mAsyncStream.swap(stream);

The swap-to-stack-nsCOMPtr pattern is only really useful if you're doing other things and always need to remember to free something. Here it's much simpler to simply set mAsyncStream to null.

@@ +323,5 @@
>    return rv;
>  }
>  
> +NS_IMETHODIMP
> +nsDOMFileReader::DoReadData(nsIAsyncInputStream* aStream, uint64_t aCount) {

Nit: { on its own line.

@@ +326,5 @@
> +NS_IMETHODIMP
> +nsDOMFileReader::DoReadData(nsIAsyncInputStream* aStream, uint64_t aCount) {
> +  MOZ_ASSERT(aStream);
> +
> +  if (mDataFormat == FILE_AS_BINARY) {

Here you should be able to assert that |mResult.Length() == mDataLen| like the old code used to do with |aOffset|.

@@ +331,5 @@
> +    //Continuously update our binary string as data comes in
> +    uint32_t oldLen = mResult.Length();
> +    if (uint64_t(oldLen) + aCount > UINT32_MAX)
> +      return NS_ERROR_OUT_OF_MEMORY;
> +    char16_t *buf = nullptr;

Nit: It looks like you removed a bunch of whitespace that broke this code up into easier-to-read chunks. Please add them back.

@@ +336,5 @@
> +    mResult.GetMutableData(&buf, oldLen + aCount, fallible_t());
> +    NS_ENSURE_TRUE(buf, NS_ERROR_OUT_OF_MEMORY);
> +    uint32_t bytesRead = 0;
> +    aStream->ReadSegments(ReadFuncBinaryString, buf + oldLen, aCount,
> +                         &bytesRead);

Nit: Your indentation is off by one here.

@@ +357,5 @@
> +
> +    uint32_t bytesRead = 0;
> +    aStream->Read(mFileData + mDataLen, aCount, &bytesRead);
> +    NS_ASSERTION(bytesRead == aCount, "failed to read data");
> +

Nit: Please remove the extra line here.

@@ +368,4 @@
>  // Helper methods
>  
>  void
>  nsDOMFileReader::ReadFileContent(JSContext* aCx,

I don't see anywhere where mDataLen is being reset here... What happens when these objects are reused?

@@ +393,4 @@
>  
> +  nsCOMPtr<nsIStreamTransportService> sts =
> +    do_GetService(kStreamTransportServiceCID, &rv);
> +  if (NS_FAILED(rv)) {

Let's use |if (NS_WARN_IF(NS_FAILED(rv))) {| here and in the other places you've modified below.

@@ +420,5 @@
> +  rv = transport->OpenInputStream(/* aFlags */ 0,
> +                                  /* aSegmentSize */ 0,
> +                                  /* aSegmentCount */ 0,
> +                                  getter_AddRefs(wrapper));
> +/*  if (NS_FAILED(rv)) {

Why is this commented out?

@@ +434,5 @@
>    mFile->GetSize(&mTotal);
>  
> +  rv = DoAsyncWait(mAsyncStream);
> +  if (NS_FAILED(rv)) {
> +    return;

This needs to call |aRv.Throw(rv)| also.

::: content/base/src/nsDOMFileReader.h
@@ +26,4 @@
>  
>  #include "FileIOObject.h"
>  
> +

Nit: Please remove this extra line.

@@ +49,5 @@
>  
>    // FileIOObject overrides
>    virtual void DoAbort(nsAString& aEvent) MOZ_OVERRIDE;
> +
> +  NS_IMETHOD DoReadData(nsIAsyncInputStream *aStream, uint64_t aCount) MOZ_OVERRIDE;

s/NS_IMETHOD/virtual nsresult/

Here and below.

Comment 10

[Shihua Zheng]

Attachment: Patch v5

Q:Here you should be able to assert that |mResult.Length() == mDataLen| like the old code used to do with |aOffset|.

No, we do not have aOffset now. What we have is aCount, which means the bytes available. aOffset means the bytes read. They are inherently different.

Q:I don't see anywhere where mDataLen is being reset here... What happens when these objects are reused?

It is reset in the function FreeFileData. In the history, it lives in the header file unfortunately.

Other questions are solved.

Comment 11

[Shihua Zheng]

Attachment: Patch v5

Ok, I misunderstood your point in the assertion. The assertion is added.

Comment 12

[Shihua Zheng]

Attachment: Patch v6

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

The try run looks okay. BTW, OS X 10.6 seems to be down for a while.

Comment 13

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

(In reply to Shihua Zheng from comment #10)
> Q:Here you should be able to assert that |mResult.Length() == mDataLen| like
> the old code used to do with |aOffset|.
> 
> No, we do not have aOffset now. What we have is aCount, which means the
> bytes available. aOffset means the bytes read. They are inherently different.

Hm, I think maybe I didn't explain clearly.

The old code used to assert |mResult.Length() == aOffset|. You've replaced |aOffset| everywhere with |mDataLen| since that is the same as what |aOffset| used to mean.

So you should now be able to assert that |mResult.Length() == mDataLen|, right?

Comment 14

[Shihua Zheng]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #13)
> (In reply to Shihua Zheng from comment #10)
> > Q:Here you should be able to assert that |mResult.Length() == mDataLen| like
> > the old code used to do with |aOffset|.
> > 
> > No, we do not have aOffset now. What we have is aCount, which means the
> > bytes available. aOffset means the bytes read. They are inherently different.
> 
> Hm, I think maybe I didn't explain clearly.
> 
> The old code used to assert |mResult.Length() == aOffset|. You've replaced
> |aOffset| everywhere with |mDataLen| since that is the same as what
> |aOffset| used to mean.
> 
> So you should now be able to assert that |mResult.Length() == mDataLen|,
> right?

Yes, I've added it (Line 332).

Comment 15

[Shihua Zheng]

Attachment: Patch v7

Comment 16

[Shihua Zheng]

Attachment: Patch v7

Comment 17

[Shihua Zheng]

https://tbpl.mozilla.org/?tree=Try&rev=8c6b5d5bdc0c

Comment 18

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

Comment on attachment 8437314 [details] [diff] [review]
Patch v7

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

This looks great! r+ with these things fixed:

::: content/base/src/FileIOObject.cpp
@@ +213,5 @@
>  
> +nsresult
> +FileIOObject::DoAsyncWait(nsIAsyncInputStream* aStream)
> +{
> +  return aStream->AsyncWait(this, 0, 0, NS_GetCurrentThread());

Nit: Let's add /* */ comments around those 0 args so that the reader knows what they mean.

::: content/base/src/FileIOObject.h
@@ +67,5 @@
>  protected:
>    // Implemented by the derived class to do whatever it needs to do for abort
>    virtual void DoAbort(nsAString& aEvent) = 0;
> +
> +  virtual nsresult DoReadData(nsIAsyncInputStream *aStream, uint64_t aCount) = 0;

Nit: Let's put the * on the left there.

@@ +70,5 @@
> +
> +  virtual nsresult DoReadData(nsIAsyncInputStream *aStream, uint64_t aCount) = 0;
> +
> +  virtual nsresult DoOnLoadEnd(nsresult aStatus, nsAString& aSuccessEvent,
> +                         nsAString& aTerminationEvent) = 0;

Nit: Your indentation is off here.

::: content/base/src/nsDOMFileReader.h
@@ +46,5 @@
>    // nsIInterfaceRequestor 
>    NS_DECL_NSIINTERFACEREQUESTOR
>  
>    // FileIOObject overrides
> +  void DoAbort(nsAString& aEvent) MOZ_OVERRIDE;

Nit: Please keep this marked as 'virtual', the other two below.

@@ +48,5 @@
>  
>    // FileIOObject overrides
> +  void DoAbort(nsAString& aEvent) MOZ_OVERRIDE;
> +
> +  nsresult DoReadData(nsIAsyncInputStream *aStream, uint64_t aCount) MOZ_OVERRIDE;

Nit: Let's put the * on the left there.

@@ +51,5 @@
> +
> +  nsresult DoReadData(nsIAsyncInputStream *aStream, uint64_t aCount) MOZ_OVERRIDE;
> +
> +  nsresult DoOnLoadEnd(nsresult aStatus, nsAString& aSuccessEvent,
> +                         nsAString& aTerminationEvent) MOZ_OVERRIDE;

Nit: Your indentation is off here.

Comment 19

[Shihua Zheng]

Attachment: Patch v7

Style cleared, need checkin?

Comment 20

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/69e8adfd9257

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/69e8adfd9257