Bug 860731 - Part e: Additional cleanup and fixes; r=bent

dom/file/FileHandle.h

@@ -17,7 +17,6 @@
 #include "mozilla/dom/FileModeBinding.h"
 
 class nsIDOMFile;
-class nsIDOMLockedFile;
 class nsIFileStorage;
 class nsPIDOMWindow;
 

dom/file/FileRequest.h

@@ -12,8 +12,6 @@
 
 #include "DOMRequest.h"
 
-class nsIDOMLockedFile;
-
 namespace mozilla {
 class EventChainPreVisitor;
 } // namespace mozilla

dom/file/LockedFile.cpp

@@ -7,7 +7,6 @@
 #include "LockedFile.h"
 
 #include "AsyncHelper.h"
-#include "nsIDOMEvent.h"
 #include "FileHandle.h"
 #include "FileHelper.h"
 #include "FileRequest.h"
@@ -24,6 +23,7 @@
 #include "nsContentUtils.h"
 #include "nsError.h"
 #include "nsIAppShell.h"
+#include "nsIDOMEvent.h"
 #include "nsIDOMFile.h"
 #include "nsIFileStorage.h"
 #include "nsISeekableStream.h"
@@ -244,54 +244,6 @@ LockedFile::Create(FileHandle* aFileHandle,
   return lockedFile.forget();
 }
 
-/* static */ already_AddRefed<nsIInputStream>
-LockedFile::GetInputStream(const ArrayBuffer& aValue, uint64_t* aInputLength,
-                           ErrorResult& aRv)
-{
-  const char* data = reinterpret_cast<const char*>(aValue.Data());
-  uint32_t length = aValue.Length();
-
-  nsCOMPtr<nsIInputStream> stream;
-  aRv = NS_NewByteInputStream(getter_AddRefs(stream), data, length,
-                              NS_ASSIGNMENT_COPY);
-  if (aRv.Failed()) {
-    return nullptr;
-  }
-
-  *aInputLength = length;
-  return stream.forget();
-}
-
-/* static */ already_AddRefed<nsIInputStream>
-LockedFile::GetInputStream(nsIDOMBlob* aValue, uint64_t* aInputLength,
-                           ErrorResult& aRv)
-{
-  aRv = aValue->GetSize(aInputLength);
-  if (aRv.Failed()) {
-    return nullptr;
-  }
-
-  nsCOMPtr<nsIInputStream> stream;
-  aRv = aValue->GetInternalStream(getter_AddRefs(stream));
-  return stream.forget();
-}
-
-/* static */ already_AddRefed<nsIInputStream>
-LockedFile::GetInputStream(const nsAString& aValue, uint64_t* aInputLength,
-                           ErrorResult& aRv)
-{
-  NS_ConvertUTF16toUTF8 cstr(aValue);
-
-  nsCOMPtr<nsIInputStream> stream;
-  aRv = NS_NewCStringInputStream(getter_AddRefs(stream), cstr);
-  if (aRv.Failed()) {
-    return nullptr;
-  }
-
-  *aInputLength = cstr.Length();
-  return stream.forget();
-}
-
 LockedFile::LockedFile()
 : mReadyState(INITIAL),
   mMode(FileMode::Readonly),
@@ -437,14 +389,21 @@ LockedFile::IsOpen() const
   return false;
 }
 
+// virtual
+JSObject*
+LockedFile::WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope)
+{
+  return LockedFileBinding::Wrap(aCx, aScope, this);
+}
+
 already_AddRefed<FileRequest>
 LockedFile::GetMetadata(const DOMFileMetadataParameters& aParameters,
                         ErrorResult& aRv)
 {
   NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
 
-  if (!IsOpen()) {
-    aRv.Throw(NS_ERROR_DOM_FILEHANDLE_LOCKEDFILE_INACTIVE_ERR);
+  // Common state checking
+  if (!CheckState(aRv)) {
     return nullptr;
   }
 
@@ -465,7 +424,7 @@ LockedFile::GetMetadata(const DOMFileMetadataParameters& aParameters,
   nsRefPtr<MetadataHelper> helper =
     new MetadataHelper(this, fileRequest, params);
 
-  if (NS_FAILED(helper->Enqueue())) {
+  if (NS_WARN_IF(NS_FAILED(helper->Enqueue()))) {
     aRv.Throw(NS_ERROR_DOM_FILEHANDLE_UNKNOWN_ERR);
     return nullptr;
   }
@@ -473,47 +432,28 @@ LockedFile::GetMetadata(const DOMFileMetadataParameters& aParameters,
   return fileRequest.forget();
 }
 
-bool
-LockedFile::CheckStateAndArgumentsForRead(uint64_t aSize, ErrorResult& aRv)
-{
-  if (!IsOpen()) {
-    aRv.Throw(NS_ERROR_DOM_FILEHANDLE_LOCKEDFILE_INACTIVE_ERR);
-    return false;
-  }
-
-  if (mLocation == UINT64_MAX) {
-    aRv.Throw(NS_ERROR_DOM_FILEHANDLE_NOT_ALLOWED_ERR);
-    return false;
-  }
-
-  if (!aSize) {
-    aRv.ThrowTypeError(MSG_INVALID_READ_SIZE);
-    return false;
-  }
-
-  // Do nothing if the window is closed
-  if (!GetOwner()) {
-    return false;
-  }
-
-  return true;
-}
-
 already_AddRefed<FileRequest>
 LockedFile::ReadAsArrayBuffer(uint64_t aSize, ErrorResult& aRv)
 {
   NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
 
+  // State and argument checking for read
   if (!CheckStateAndArgumentsForRead(aSize, aRv)) {
     return nullptr;
   }
 
+  // Do nothing if the window is closed
+  if (!GetOwner()) {
+    return nullptr;
+  }
+
   nsRefPtr<FileRequest> fileRequest = GenerateFileRequest();
 
   nsRefPtr<ReadHelper> helper =
     new ReadHelper(this, fileRequest, mLocation, aSize);
 
-  if (NS_FAILED(helper->Init()) || NS_FAILED(helper->Enqueue())) {
+  if (NS_WARN_IF(NS_FAILED(helper->Init())) ||
+      NS_WARN_IF(NS_FAILED(helper->Enqueue()))) {
     aRv.Throw(NS_ERROR_DOM_FILEHANDLE_UNKNOWN_ERR);
     return nullptr;
   }
@@ -529,16 +469,23 @@ LockedFile::ReadAsText(uint64_t aSize, const nsAString& aEncoding,
 {
   NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
 
+  // State and argument checking for read
   if (!CheckStateAndArgumentsForRead(aSize, aRv)) {
     return nullptr;
   }
 
+  // Do nothing if the window is closed
+  if (!GetOwner()) {
+    return nullptr;
+  }
+
   nsRefPtr<FileRequest> fileRequest = GenerateFileRequest();
 
   nsRefPtr<ReadTextHelper> helper =
     new ReadTextHelper(this, fileRequest, mLocation, aSize, aEncoding);
 
-  if (NS_FAILED(helper->Init()) || NS_FAILED(helper->Enqueue())) {
+  if (NS_WARN_IF(NS_FAILED(helper->Init())) ||
+      NS_WARN_IF(NS_FAILED(helper->Enqueue()))) {
     aRv.Throw(NS_ERROR_DOM_FILEHANDLE_UNKNOWN_ERR);
     return nullptr;
   }
@@ -548,32 +495,17 @@ LockedFile::ReadAsText(uint64_t aSize, const nsAString& aEncoding,
   return fileRequest.forget();
 }
 
-bool
-LockedFile::CheckStateForWrite(ErrorResult& aRv)
-{
-  if (!IsOpen()) {
-    aRv.Throw(NS_ERROR_DOM_FILEHANDLE_LOCKEDFILE_INACTIVE_ERR);
-    return false;
-  }
-
-  if (mMode != FileMode::Readwrite) {
-    aRv.Throw(NS_ERROR_DOM_FILEHANDLE_READ_ONLY_ERR);
-    return false;
-  }
-
-  // Do nothing if the window is closed
-  if (!GetOwner()) {
-    return false;
-  }
-
-  return true;
-}
-
 already_AddRefed<FileRequest>
 LockedFile::Truncate(const Optional<uint64_t>& aSize, ErrorResult& aRv)
 {
   NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
 
+  // State checking for write
+  if (!CheckStateForWrite(aRv)) {
+    return nullptr;
+  }
+
+  // Getting location and additional state checking for truncate
   uint64_t location;
   if (aSize.WasPassed()) {
     // Just in case someone calls us from C++
@@ -587,7 +519,8 @@ LockedFile::Truncate(const Optional<uint64_t>& aSize, ErrorResult& aRv)
     location = mLocation;
   }
 
-  if (!CheckStateForWrite(aRv)) {
+  // Do nothing if the window is closed
+  if (!GetOwner()) {
     return nullptr;
   }
 
@@ -596,7 +529,7 @@ LockedFile::Truncate(const Optional<uint64_t>& aSize, ErrorResult& aRv)
   nsRefPtr<TruncateHelper> helper =
     new TruncateHelper(this, fileRequest, location);
 
-  if (NS_FAILED(helper->Enqueue())) {
+  if (NS_WARN_IF(NS_FAILED(helper->Enqueue()))) {
     aRv.Throw(NS_ERROR_DOM_FILEHANDLE_UNKNOWN_ERR);
     return nullptr;
   }
@@ -613,15 +546,21 @@ LockedFile::Flush(ErrorResult& aRv)
 {
   NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
 
+  // State checking for write
   if (!CheckStateForWrite(aRv)) {
     return nullptr;
   }
 
+  // Do nothing if the window is closed
+  if (!GetOwner()) {
+    return nullptr;
+  }
+
   nsRefPtr<FileRequest> fileRequest = GenerateFileRequest();
 
   nsRefPtr<FlushHelper> helper = new FlushHelper(this, fileRequest);
 
-  if (NS_FAILED(helper->Enqueue())) {
+  if (NS_WARN_IF(NS_FAILED(helper->Enqueue()))) {
     aRv.Throw(NS_ERROR_DOM_FILEHANDLE_UNKNOWN_ERR);
     return nullptr;
   }
@@ -634,6 +573,8 @@ LockedFile::Abort(ErrorResult& aRv)
 {
   NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
 
+  // This method is special enough for not using generic state checking methods.
+
   // We can't use IsOpen here since we need it to be possible to call Abort()
   // even from outside of transaction callbacks.
   if (mReadyState != LockedFile::INITIAL &&
@@ -682,8 +623,10 @@ LockedFile::OpenInputStream(bool aWholeFile, uint64_t aStart, uint64_t aLength,
   NS_ASSERTION(mRequestMode == PARALLEL,
                "Don't call me in other than parallel mode!");
 
-  if (!IsOpen()) {
-    return NS_ERROR_DOM_FILEHANDLE_LOCKEDFILE_INACTIVE_ERR;
+  // Common state checking
+  ErrorResult error;
+  if (!CheckState(error)) {
+    return error.ErrorCode();
   }
 
   // Do nothing if the window is closed
@@ -704,24 +647,69 @@ LockedFile::OpenInputStream(bool aWholeFile, uint64_t aStart, uint64_t aLength,
   return NS_OK;
 }
 
+bool
+LockedFile::CheckState(ErrorResult& aRv)
+{
+  if (!IsOpen()) {
+    aRv.Throw(NS_ERROR_DOM_FILEHANDLE_LOCKEDFILE_INACTIVE_ERR);
+    return false;
+  }
+
+  return true;
+}
+
+bool
+LockedFile::CheckStateAndArgumentsForRead(uint64_t aSize, ErrorResult& aRv)
+{
+  // Common state checking
+  if (!CheckState(aRv)) {
+    return false;
+  }
+
+  // Additional state checking for read
+  if (mLocation == UINT64_MAX) {
+    aRv.Throw(NS_ERROR_DOM_FILEHANDLE_NOT_ALLOWED_ERR);
+    return false;
+  }
+
+  // Argument checking for read
+  if (!aSize) {
+    aRv.ThrowTypeError(MSG_INVALID_READ_SIZE);
+    return false;
+  }
+
+  return true;
+}
+
+bool
+LockedFile::CheckStateForWrite(ErrorResult& aRv)
+{
+  // Common state checking
+  if (!CheckState(aRv)) {
+    return false;
+  }
+
+  // Additional state checking for write
+  if (mMode != FileMode::Readwrite) {
+    aRv.Throw(NS_ERROR_DOM_FILEHANDLE_READ_ONLY_ERR);
+    return false;
+  }
+
+  return true;
+}
+
 already_AddRefed<FileRequest>
-LockedFile::WriteOrAppend(nsIInputStream* aInputStream, uint64_t aInputLength,
+LockedFile::WriteInternal(nsIInputStream* aInputStream, uint64_t aInputLength,
                           bool aAppend, ErrorResult& aRv)
 {
-  NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
+  MOZ_ASSERT(NS_IsMainThread(), "Wrong thread!");
 
-  if (!aAppend && mLocation == UINT64_MAX) {
-    aRv.Throw(NS_ERROR_DOM_FILEHANDLE_NOT_ALLOWED_ERR);
-    return nullptr;
-  }
-
-  if (!CheckStateForWrite(aRv)) {
-    return nullptr;
-  }
-
-  if (!aInputLength) {
-    return nullptr;
-  }
+  DebugOnly<ErrorResult> error;
+  MOZ_ASSERT(CheckStateForWrite(error));
+  MOZ_ASSERT_IF(!aAppend, mLocation != UINT64_MAX);
+  MOZ_ASSERT(aInputStream);
+  MOZ_ASSERT(aInputLength);
+  MOZ_ASSERT(GetOwner());
 
   nsRefPtr<FileRequest> fileRequest = GenerateFileRequest();
 
@@ -730,7 +718,7 @@ LockedFile::WriteOrAppend(nsIInputStream* aInputStream, uint64_t aInputLength,
   nsRefPtr<WriteHelper> helper =
     new WriteHelper(this, fileRequest, location, aInputStream, aInputLength);
 
-  if (NS_FAILED(helper->Enqueue())) {
+  if (NS_WARN_IF(NS_FAILED(helper->Enqueue()))) {
     aRv.Throw(NS_ERROR_DOM_FILEHANDLE_UNKNOWN_ERR);
     return nullptr;
   }
@@ -763,12 +751,62 @@ LockedFile::Finish()
   return NS_OK;
 }
 
-/* virtual */ JSObject*
-LockedFile::WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope)
+// static
+already_AddRefed<nsIInputStream>
+LockedFile::GetInputStream(const ArrayBuffer& aValue, uint64_t* aInputLength,
+                           ErrorResult& aRv)
 {
-  return LockedFileBinding::Wrap(aCx, aScope, this);
+  const char* data = reinterpret_cast<const char*>(aValue.Data());
+  uint32_t length = aValue.Length();
+
+  nsCOMPtr<nsIInputStream> stream;
+  aRv = NS_NewByteInputStream(getter_AddRefs(stream), data, length,
+                              NS_ASSIGNMENT_COPY);
+  if (aRv.Failed()) {
+    return nullptr;
+  }
+
+  *aInputLength = length;
+  return stream.forget();
 }
 
+// static
+already_AddRefed<nsIInputStream>
+LockedFile::GetInputStream(nsIDOMBlob* aValue, uint64_t* aInputLength,
+                           ErrorResult& aRv)
+{
+  uint64_t length;
+  aRv = aValue->GetSize(&length);
+  if (aRv.Failed()) {
+    return nullptr;
+  }
+
+  nsCOMPtr<nsIInputStream> stream;
+  aRv = aValue->GetInternalStream(getter_AddRefs(stream));
+  if (aRv.Failed()) {
+    return nullptr;
+  }
+
+  *aInputLength = length;
+  return stream.forget();
+}
+
+// static
+already_AddRefed<nsIInputStream>
+LockedFile::GetInputStream(const nsAString& aValue, uint64_t* aInputLength,
+                           ErrorResult& aRv)
+{
+  NS_ConvertUTF16toUTF8 cstr(aValue);
+
+  nsCOMPtr<nsIInputStream> stream;
+  aRv = NS_NewCStringInputStream(getter_AddRefs(stream), cstr);
+  if (aRv.Failed()) {
+    return nullptr;
+  }
+
+  *aInputLength = cstr.Length();
+  return stream.forget();
+}
 
 FinishHelper::FinishHelper(LockedFile* aLockedFile)
 : mLockedFile(aLockedFile),

dom/file/LockedFile.h

@@ -95,40 +95,57 @@ public:
                   nsIInputStream** aResult);
 
   // WrapperCache
-  nsPIDOMWindow* GetParentObject() const
-  {
-    return GetOwner();
-  }
-
   virtual JSObject*
   WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;
 
   // WebIDL
-  FileHandle* GetFileHandle() const
+  nsPIDOMWindow*
+  GetParentObject() const
   {
+    return GetOwner();
+  }
+
+  FileHandle*
+  GetFileHandle() const
+  {
+    MOZ_ASSERT(NS_IsMainThread(), "Wrong thread!");
+
     return Handle();
   }
 
-  FileMode Mode() const
+  FileMode
+  Mode() const
   {
+    MOZ_ASSERT(NS_IsMainThread(), "Wrong thread!");
+
     return mMode;
   }
 
-  bool Active() const
+  bool
+  Active() const
   {
+    MOZ_ASSERT(NS_IsMainThread(), "Wrong thread!");
+
     return IsOpen();
   }
 
-  Nullable<uint64_t> GetLocation() const
+  Nullable<uint64_t>
+  GetLocation() const
   {
+    MOZ_ASSERT(NS_IsMainThread(), "Wrong thread!");
+
     if (mLocation == UINT64_MAX) {
       return Nullable<uint64_t>();
     }
+
     return Nullable<uint64_t>(mLocation);
   }
 
-  void SetLocation(const Nullable<uint64_t>& aLocation)
+  void
+  SetLocation(const Nullable<uint64_t>& aLocation)
   {
+    MOZ_ASSERT(NS_IsMainThread(), "Wrong thread!");
+
     // Null means the end-of-file.
     if (aLocation.IsNull()) {
       mLocation = UINT64_MAX;
@@ -150,24 +167,18 @@ public:
   already_AddRefed<FileRequest>
   Write(const T& aValue, ErrorResult& aRv)
   {
-    uint64_t length;
-    nsCOMPtr<nsIInputStream> stream = GetInputStream(aValue, &length, aRv);
-    if (aRv.Failed()) {
-      return nullptr;
-    }
-    return Write(stream, length, aRv);
+    MOZ_ASSERT(NS_IsMainThread(), "Wrong thread!");
+
+    return WriteOrAppend(aValue, false, aRv);
   }
 
   template<class T>
   already_AddRefed<FileRequest>
   Append(const T& aValue, ErrorResult& aRv)
   {
-    uint64_t length;
-    nsCOMPtr<nsIInputStream> stream = GetInputStream(aValue, &length, aRv);
-    if (aRv.Failed()) {
-      return nullptr;
-    }
-    return Append(stream, length, aRv);
+    MOZ_ASSERT(NS_IsMainThread(), "Wrong thread!");
+
+    return WriteOrAppend(aValue, true, aRv);
   }
 
   already_AddRefed<FileRequest>
@@ -176,7 +187,8 @@ public:
   already_AddRefed<FileRequest>
   Flush(ErrorResult& aRv);
 
-  void Abort(ErrorResult& aRv);
+  void
+  Abort(ErrorResult& aRv);
 
   IMPL_EVENT_HANDLER(complete)
   IMPL_EVENT_HANDLER(abort)
@@ -192,26 +204,55 @@ private:
   void
   OnRequestFinished();
 
-  bool CheckStateAndArgumentsForRead(uint64_t aSize, ErrorResult& aRv);
-  bool CheckStateForWrite(ErrorResult& aRv);
+  bool
+  CheckState(ErrorResult& aRv);
+
+  bool
+  CheckStateAndArgumentsForRead(uint64_t aSize, ErrorResult& aRv);
+
+  bool
+  CheckStateForWrite(ErrorResult& aRv);
 
   already_AddRefed<FileRequest>
   GenerateFileRequest();
 
+  template<class T>
   already_AddRefed<FileRequest>
-  Write(nsIInputStream* aInputStream, uint64_t aInputLength, ErrorResult& aRv)
+  WriteOrAppend(const T& aValue, bool aAppend, ErrorResult& aRv)
   {
-    return WriteOrAppend(aInputStream, aInputLength, false, aRv);
+    MOZ_ASSERT(NS_IsMainThread(), "Wrong thread!");
+
+    // State checking for write
+    if (!CheckStateForWrite(aRv)) {
+      return nullptr;
+    }
+
+    // Additional state checking for write
+    if (!aAppend && mLocation == UINT64_MAX) {
+      aRv.Throw(NS_ERROR_DOM_FILEHANDLE_NOT_ALLOWED_ERR);
+      return nullptr;
+    }
+
+    uint64_t length;
+    nsCOMPtr<nsIInputStream> stream = GetInputStream(aValue, &length, aRv);
+    if (aRv.Failed()) {
+      return nullptr;
+    }
+
+    if (!length) {
+      return nullptr;
+    }
+
+    // Do nothing if the window is closed
+    if (!GetOwner()) {
+      return nullptr;
+    }
+
+    return WriteInternal(stream, length, aAppend, aRv);
   }
 
   already_AddRefed<FileRequest>
-  Append(nsIInputStream* aInputStream, uint64_t aInputLength, ErrorResult& aRv)
-  {
-    return WriteOrAppend(aInputStream, aInputLength, true, aRv);
-  }
-
-  already_AddRefed<FileRequest>
-  WriteOrAppend(nsIInputStream* aInputStream, uint64_t aInputLength,
+  WriteInternal(nsIInputStream* aInputStream, uint64_t aInputLength,
                 bool aAppend, ErrorResult& aRv);
 
   nsresult
@@ -220,8 +261,10 @@ private:
   static already_AddRefed<nsIInputStream>
   GetInputStream(const ArrayBuffer& aValue, uint64_t* aInputLength,
                  ErrorResult& aRv);
+
   static already_AddRefed<nsIInputStream>
   GetInputStream(nsIDOMBlob* aValue, uint64_t* aInputLength, ErrorResult& aRv);
+
   static already_AddRefed<nsIInputStream>
   GetInputStream(const nsAString& aValue, uint64_t* aInputLength,
                  ErrorResult& aRv);