Bug 1619518 - part 2 - Better File.lastModified attribute handling, r=ssengupta,ttung,dom-workers-and-storage-reviewers,smaug

Differential Revision: https://phabricator.services.mozilla.com/D65074

--HG--
extra : moz-landing-system : lando

dom/base/BodyUtil.cpp

@@ -252,7 +252,7 @@ class MOZ_STACK_CLASS FormDataParser {
       }
       p = nullptr;
 
-      RefPtr<Blob> file = File::CreateMemoryFile(
+      RefPtr<Blob> file = File::CreateMemoryFileWithCustomLastModified(
           mParentObject, reinterpret_cast<void*>(copy), body.Length(),
           NS_ConvertUTF8toUTF16(mFilename), NS_ConvertUTF8toUTF16(mContentType),
           /* aLastModifiedDate */ 0);

dom/events/DataTransferItem.cpp

@@ -397,8 +397,8 @@ already_AddRefed<File> DataTransferItem::CreateFileFromInputStream(
     return nullptr;
   }
 
-  return File::CreateMemoryFile(global, data, available, fileName, mType,
+  return File::CreateMemoryFileWithLastModifiedNow(global, data, available,
-                                PR_Now());
+                                                   fileName, mType);
 }
 
 void DataTransferItem::GetAsString(FunctionStringCallback* aCallback,

dom/file/BaseBlobImpl.cpp

@@ -48,20 +48,9 @@ void BaseBlobImpl::GetType(nsAString& aType) { aType = mContentType; }
 
 int64_t BaseBlobImpl::GetLastModified(ErrorResult& aRv) {
   MOZ_ASSERT(mIsFile, "Should only be called on files");
-  if (IsDateUnknown()) {
-    mLastModificationDate =
-        nsRFPService::ReduceTimePrecisionAsUSecs(PR_Now(), 0);
-    // mLastModificationDate is an absolute timestamp so we supply a zero
-    // context mix-in
-  }
-
   return mLastModificationDate / PR_USEC_PER_MSEC;
 }
 
-void BaseBlobImpl::SetLastModified(int64_t aLastModified) {
-  mLastModificationDate = aLastModified * PR_USEC_PER_MSEC;
-}
-
 int64_t BaseBlobImpl::GetFileId() { return -1; }
 
 nsresult BaseBlobImpl::GetSendInfo(nsIInputStream** aBody,

dom/file/BaseBlobImpl.h

@@ -14,6 +14,7 @@ namespace dom {
 
 class BaseBlobImpl : public BlobImpl {
  public:
+  // File constructor.
   BaseBlobImpl(const nsAString& aBlobImplType, const nsAString& aName,
                const nsAString& aContentType, uint64_t aLength,
                int64_t aLastModifiedDate)
@@ -29,20 +30,7 @@ class BaseBlobImpl : public BlobImpl {
     mContentType.SetIsVoid(false);
   }
 
-  BaseBlobImpl(const nsAString& aBlobImplType, const nsAString& aName,
+  // Blob constructor without starting point.
-               const nsAString& aContentType, uint64_t aLength)
-      : mBlobImplType(aBlobImplType),
-        mIsFile(true),
-        mContentType(aContentType),
-        mName(aName),
-        mStart(0),
-        mLength(aLength),
-        mLastModificationDate(INT64_MAX),
-        mSerialNumber(NextSerialNumber()) {
-    // Ensure non-null mContentType by default
-    mContentType.SetIsVoid(false);
-  }
-
   BaseBlobImpl(const nsAString& aBlobImplType, const nsAString& aContentType,
                uint64_t aLength)
       : mBlobImplType(aBlobImplType),
@@ -50,12 +38,13 @@ class BaseBlobImpl : public BlobImpl {
         mContentType(aContentType),
         mStart(0),
         mLength(aLength),
-        mLastModificationDate(INT64_MAX),
+        mLastModificationDate(0),
         mSerialNumber(NextSerialNumber()) {
     // Ensure non-null mContentType by default
     mContentType.SetIsVoid(false);
   }
 
+  // Blob constructor with starting point.
   BaseBlobImpl(const nsAString& aBlobImplType, const nsAString& aContentType,
                uint64_t aStart, uint64_t aLength)
       : mBlobImplType(aBlobImplType),
@@ -63,7 +52,7 @@ class BaseBlobImpl : public BlobImpl {
         mContentType(aContentType),
         mStart(aStart),
         mLength(aLength),
-        mLastModificationDate(INT64_MAX),
+        mLastModificationDate(0),
         mSerialNumber(NextSerialNumber()) {
     MOZ_ASSERT(aLength != UINT64_MAX, "Must know length when creating slice");
     // Ensure non-null mContentType by default
@@ -78,8 +67,6 @@ class BaseBlobImpl : public BlobImpl {
 
   virtual int64_t GetLastModified(ErrorResult& aRv) override;
 
-  virtual void SetLastModified(int64_t aLastModified) override;
-
   virtual void GetMozFullPath(nsAString& aName,
                               SystemCallerGuarantee /* unused */,
                               ErrorResult& aRv) override;
@@ -134,10 +121,6 @@ class BaseBlobImpl : public BlobImpl {
 
   virtual bool IsMemoryFile() const override { return false; }
 
-  virtual bool IsDateUnknown() const override {
-    return mIsFile && mLastModificationDate == INT64_MAX;
-  }
-
   virtual bool IsFile() const override { return mIsFile; }
 
   virtual bool IsSizeUnknown() const override { return mLength == UINT64_MAX; }

dom/file/BlobImpl.h

@@ -41,8 +41,6 @@ class BlobImpl : public nsISupports {
 
   virtual int64_t GetLastModified(ErrorResult& aRv) = 0;
 
-  virtual void SetLastModified(int64_t aLastModified) = 0;
-
   virtual void GetMozFullPath(nsAString& aName,
                               SystemCallerGuarantee /* unused */,
                               ErrorResult& aRv) = 0;
@@ -97,8 +95,6 @@ class BlobImpl : public nsISupports {
 
   virtual bool IsSizeUnknown() const = 0;
 
-  virtual bool IsDateUnknown() const = 0;
-
   virtual bool IsFile() const = 0;
 
   // Returns true if the BlobImpl is backed by an nsIFile and the underlying

dom/file/File.cpp

@@ -38,37 +38,28 @@ File* File::Create(nsIGlobalObject* aGlobal, BlobImpl* aImpl) {
 }
 
 /* static */
-already_AddRefed<File> File::Create(nsIGlobalObject* aGlobal,
+already_AddRefed<File> File::CreateMemoryFileWithCustomLastModified(
-                                    const nsAString& aName,
+    nsIGlobalObject* aGlobal, void* aMemoryBuffer, uint64_t aLength,
-                                    const nsAString& aContentType,
+    const nsAString& aName, const nsAString& aContentType,
-                                    uint64_t aLength,
+    int64_t aLastModifiedDate) {
-                                    int64_t aLastModifiedDate) {
+  RefPtr<MemoryBlobImpl> blobImpl =
-  MOZ_ASSERT(aGlobal);
+      MemoryBlobImpl::CreateWithCustomLastModified(
-  if (NS_WARN_IF(!aGlobal)) {
+          aMemoryBuffer, aLength, aName, aContentType, aLastModifiedDate);
-    return nullptr;
+  MOZ_ASSERT(blobImpl);
-  }
 
-  RefPtr<File> file = new File(
+  RefPtr<File> file = File::Create(aGlobal, blobImpl);
-      aGlobal, new BaseBlobImpl(NS_LITERAL_STRING("BaseBlobImpl"), aName,
-                                aContentType, aLength, aLastModifiedDate));
   return file.forget();
 }
 
 /* static */
-already_AddRefed<File> File::CreateMemoryFile(nsIGlobalObject* aGlobal,
+already_AddRefed<File> File::CreateMemoryFileWithLastModifiedNow(
-                                              void* aMemoryBuffer,
+    nsIGlobalObject* aGlobal, void* aMemoryBuffer, uint64_t aLength,
-                                              uint64_t aLength,
+    const nsAString& aName, const nsAString& aContentType) {
-                                              const nsAString& aName,
+  RefPtr<MemoryBlobImpl> blobImpl = MemoryBlobImpl::CreateWithLastModifiedNow(
-                                              const nsAString& aContentType,
+      aMemoryBuffer, aLength, aName, aContentType);
-                                              int64_t aLastModifiedDate) {
+  MOZ_ASSERT(blobImpl);
-  MOZ_ASSERT(aGlobal);
-  if (NS_WARN_IF(!aGlobal)) {
-    return nullptr;
-  }
 
-  RefPtr<File> file =
+  RefPtr<File> file = File::Create(aGlobal, blobImpl);
-      new File(aGlobal, new MemoryBlobImpl(aMemoryBuffer, aLength, aName,
-                                           aContentType, aLastModifiedDate));
   return file.forget();
 }
 

dom/file/File.h

@@ -26,21 +26,19 @@ class File final : public Blob {
   // Check impl->IsFile().
   static File* Create(nsIGlobalObject* aGlobal, BlobImpl* aImpl);
 
-  static already_AddRefed<File> Create(nsIGlobalObject* aGlobal,
-                                       const nsAString& aName,
-                                       const nsAString& aContentType,
-                                       uint64_t aLength,
-                                       int64_t aLastModifiedDate);
-
   // The returned File takes ownership of aMemoryBuffer. aMemoryBuffer will be
   // freed by free so it must be allocated by malloc or something
   // compatible with it.
-  static already_AddRefed<File> CreateMemoryFile(nsIGlobalObject* aGlobal,
+  static already_AddRefed<File> CreateMemoryFileWithLastModifiedNow(
-                                                 void* aMemoryBuffer,
+      nsIGlobalObject* aGlobal, void* aMemoryBuffer, uint64_t aLength,
-                                                 uint64_t aLength,
+      const nsAString& aName, const nsAString& aContentType);
-                                                 const nsAString& aName,
+
-                                                 const nsAString& aContentType,
+  // You should not use this method! Please consider to use the
-                                                 int64_t aLastModifiedDate);
+  // CreateMemoryFileWithLastModifiedNow.
+  static already_AddRefed<File> CreateMemoryFileWithCustomLastModified(
+      nsIGlobalObject* aGlobal, void* aMemoryBuffer, uint64_t aLength,
+      const nsAString& aName, const nsAString& aContentType,
+      int64_t aLastModifiedDate);
 
   // This method creates a BlobFileImpl for the new File object. This is
   // thread-safe, cross-process, cross-thread as any other BlobImpl, but, when

dom/file/FileBlobImpl.cpp

@@ -20,7 +20,10 @@ namespace dom {
 
 FileBlobImpl::FileBlobImpl(nsIFile* aFile)
     : BaseBlobImpl(NS_LITERAL_STRING("FileBlobImpl"), EmptyString(),
-                   EmptyString(), UINT64_MAX, INT64_MAX),
+                   EmptyString(), UINT64_MAX,
+                   // We pass 0 as lastModified because FileblobImpl has a
+                   // different way to compute the value of this attribute
+                   0),
       mMutex("FileBlobImpl::mMutex"),
       mFile(aFile),
       mFileId(-1),
@@ -37,7 +40,10 @@ FileBlobImpl::FileBlobImpl(const nsAString& aName,
                            const nsAString& aContentType, uint64_t aLength,
                            nsIFile* aFile)
     : BaseBlobImpl(NS_LITERAL_STRING("FileBlobImpl"), aName, aContentType,
-                   aLength, UINT64_MAX),
+                   aLength,
+                   // We pass 0 as lastModified because FileblobImpl has a
+                   // different way to compute the value of this attribute
+                   0),
       mMutex("FileBlobImpl::mMutex"),
       mFile(aFile),
       mFileId(-1),
@@ -64,7 +70,7 @@ FileBlobImpl::FileBlobImpl(const nsAString& aName,
 FileBlobImpl::FileBlobImpl(nsIFile* aFile, const nsAString& aName,
                            const nsAString& aContentType,
                            const nsAString& aBlobImplType)
-    : BaseBlobImpl(aBlobImplType, aName, aContentType, UINT64_MAX, INT64_MAX),
+    : BaseBlobImpl(aBlobImplType, aName, aContentType, UINT64_MAX, 0),
       mMutex("FileBlobImpl::mMutex"),
       mFile(aFile),
       mFileId(-1),
@@ -226,17 +232,17 @@ int64_t FileBlobImpl::GetLastModified(ErrorResult& aRv) {
 
   MutexAutoLock lock(mMutex);
 
-  if (BaseBlobImpl::IsDateUnknown()) {
+  if (mLastModified.isNothing()) {
     PRTime msecs;
     aRv = mFile->GetLastModifiedTime(&msecs);
     if (NS_WARN_IF(aRv.Failed())) {
       return 0;
     }
 
-    mLastModificationDate = msecs;
+    mLastModified.emplace(int64_t(msecs));
   }
 
-  return mLastModificationDate;
+  return mLastModified.value();
 }
 
 const uint32_t sFileStreamFlags =

dom/file/FileBlobImpl.h

@@ -8,6 +8,7 @@
 #define mozilla_dom_FileBlobImpl_h
 
 #include "mozilla/dom/BaseBlobImpl.h"
+#include "mozilla/Maybe.h"
 #include "mozilla/Mutex.h"
 
 class nsIFile;
@@ -45,9 +46,15 @@ class FileBlobImpl : public BaseBlobImpl {
 
   virtual bool IsDirectory() const override;
 
-  // We always have size and date for this kind of blob.
+  virtual void SetLazyData(const nsAString& aName,
+                           const nsAString& aContentType, uint64_t aLength,
+                           int64_t aLastModifiedDate) override {
+    BaseBlobImpl::SetLazyData(aName, aContentType, aLength, 0);
+    mLastModified.emplace(aLastModifiedDate);
+  }
+
+  // We always have size for this kind of blob.
   virtual bool IsSizeUnknown() const override { return false; }
-  virtual bool IsDateUnknown() const override { return false; }
 
   void SetName(const nsAString& aName) { mName = aName; }
 
@@ -61,6 +68,10 @@ class FileBlobImpl : public BaseBlobImpl {
 
   void SetMozFullPath(const nsAString& aPath) { mMozFullPath = aPath; }
 
+  void SetLastModified(int64_t aLastModified) {
+    mLastModified.emplace(aLastModified);
+  }
+
  protected:
   virtual ~FileBlobImpl() = default;
 
@@ -82,11 +93,12 @@ class FileBlobImpl : public BaseBlobImpl {
   // - GetMozFullPathInternal - mMozFullPath
   // - GetSize - mLength
   // - GetType - mContentType
-  // - GetLastModified - mLastModificationDate
+  // - GetLastModified - mLastModifed
   Mutex mMutex;
 
   nsCOMPtr<nsIFile> mFile;
   nsString mMozFullPath;
+  Maybe<int64_t> mLastModified;
   int64_t mFileId;
   bool mWholeFile;
 };

dom/file/MemoryBlobImpl.cpp

@@ -25,6 +25,25 @@ NS_INTERFACE_MAP_BEGIN(MemoryBlobImpl::DataOwnerAdapter)
   NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIInputStream)
 NS_INTERFACE_MAP_END
 
+// static
+already_AddRefed<MemoryBlobImpl> MemoryBlobImpl::CreateWithCustomLastModified(
+    void* aMemoryBuffer, uint64_t aLength, const nsAString& aName,
+    const nsAString& aContentType, int64_t aLastModifiedDate) {
+  RefPtr<MemoryBlobImpl> blobImpl = new MemoryBlobImpl(
+      aMemoryBuffer, aLength, aName, aContentType, aLastModifiedDate);
+  return blobImpl.forget();
+}
+
+// static
+already_AddRefed<MemoryBlobImpl> MemoryBlobImpl::CreateWithLastModifiedNow(
+    void* aMemoryBuffer, uint64_t aLength, const nsAString& aName,
+    const nsAString& aContentType) {
+  int64_t lastModificationDate =
+      nsRFPService::ReduceTimePrecisionAsUSecs(PR_Now(), 0);
+  return CreateWithCustomLastModified(aMemoryBuffer, aLength, aName,
+                                      aContentType, lastModificationDate);
+}
+
 nsresult MemoryBlobImpl::DataOwnerAdapter::Create(DataOwner* aDataOwner,
                                                   uint32_t aStart,
                                                   uint32_t aLength,

dom/file/MemoryBlobImpl.h

@@ -24,13 +24,15 @@ class MemoryBlobImpl final : public BaseBlobImpl {
   NS_INLINE_DECL_REFCOUNTING_INHERITED(MemoryBlobImpl, BaseBlobImpl)
 
   // File constructor.
-  MemoryBlobImpl(void* aMemoryBuffer, uint64_t aLength, const nsAString& aName,
+  static already_AddRefed<MemoryBlobImpl> CreateWithLastModifiedNow(
-                 const nsAString& aContentType, int64_t aLastModifiedDate)
+      void* aMemoryBuffer, uint64_t aLength, const nsAString& aName,
-      : BaseBlobImpl(NS_LITERAL_STRING("MemoryBlobImpl"), aName, aContentType,
+      const nsAString& aContentType);
-                     aLength, aLastModifiedDate),
+
-        mDataOwner(new DataOwner(aMemoryBuffer, aLength)) {
+  // File constructor with custom lastModified attribue value. You should
-    MOZ_ASSERT(mDataOwner && mDataOwner->mData, "must have data");
+  // probably use CreateWithLastModifiedNow() instead of this one.
-  }
+  static already_AddRefed<MemoryBlobImpl> CreateWithCustomLastModified(
+      void* aMemoryBuffer, uint64_t aLength, const nsAString& aName,
+      const nsAString& aContentType, int64_t aLastModifiedDate);
 
   // Blob constructor.
   MemoryBlobImpl(void* aMemoryBuffer, uint64_t aLength,
@@ -142,6 +144,15 @@ class MemoryBlobImpl final : public BaseBlobImpl {
   };
 
  private:
+  // File constructor.
+  MemoryBlobImpl(void* aMemoryBuffer, uint64_t aLength, const nsAString& aName,
+                 const nsAString& aContentType, int64_t aLastModifiedDate)
+      : BaseBlobImpl(NS_LITERAL_STRING("MemoryBlobImpl"), aName, aContentType,
+                     aLength, aLastModifiedDate),
+        mDataOwner(new DataOwner(aMemoryBuffer, aLength)) {
+    MOZ_ASSERT(mDataOwner && mDataOwner->mData, "must have data");
+  }
+
   // Create slice
   MemoryBlobImpl(const MemoryBlobImpl* aOther, uint64_t aStart,
                  uint64_t aLength, const nsAString& aContentType)

dom/file/MultipartBlobImpl.cpp

@@ -233,7 +233,8 @@ void MultipartBlobImpl::InitializeBlob(const Sequence<Blob::BlobPart>& aData,
 
 void MultipartBlobImpl::SetLengthAndModifiedDate(ErrorResult& aRv) {
   MOZ_ASSERT(mLength == UINT64_MAX);
-  MOZ_ASSERT(mLastModificationDate == INT64_MAX);
+  MOZ_ASSERT_IF(mIsFile, mLastModificationDate ==
+                             MULTIPARTBLOBIMPL_UNKNOWN_LAST_MODIFIED);
 
   uint64_t totalLength = 0;
   int64_t lastModified = 0;
@@ -245,7 +246,6 @@ void MultipartBlobImpl::SetLengthAndModifiedDate(ErrorResult& aRv) {
 
 #ifdef DEBUG
     MOZ_ASSERT(!blob->IsSizeUnknown());
-    MOZ_ASSERT(!blob->IsDateUnknown());
 #endif
 
     uint64_t subBlobLength = blob->GetSize(aRv);
@@ -329,3 +329,7 @@ void MultipartBlobImpl::GetBlobImplType(nsAString& aBlobImplType) const {
 
   aBlobImplType.AppendLiteral("]");
 }
+
+void MultipartBlobImpl::SetLastModified(int64_t aLastModified) {
+  mLastModificationDate = aLastModified * PR_USEC_PER_MSEC;
+}

dom/file/MultipartBlobImpl.h

@@ -16,6 +16,10 @@
 namespace mozilla {
 namespace dom {
 
+// This is just a sentinel value to be sure that we don't call
+// SetLengthAndModifiedDate more than once.
+constexpr int64_t MULTIPARTBLOBIMPL_UNKNOWN_LAST_MODIFIED = INT64_MAX;
+
 class MultipartBlobImpl final : public BaseBlobImpl {
  public:
   NS_INLINE_DECL_REFCOUNTING_INHERITED(MultipartBlobImpl, BaseBlobImpl)
@@ -33,7 +37,8 @@ class MultipartBlobImpl final : public BaseBlobImpl {
   // Create as a file to be later initialized
   explicit MultipartBlobImpl(const nsAString& aName)
       : BaseBlobImpl(NS_LITERAL_STRING("MultipartBlobImpl"), aName,
-                     EmptyString(), UINT64_MAX) {}
+                     EmptyString(), UINT64_MAX,
+                     MULTIPARTBLOBIMPL_UNKNOWN_LAST_MODIFIED) {}
 
   // Create as a blob to be later initialized
   MultipartBlobImpl()
@@ -68,13 +73,18 @@ class MultipartBlobImpl final : public BaseBlobImpl {
 
   void GetBlobImplType(nsAString& aBlobImplType) const override;
 
+  void SetLastModified(int64_t aLastModified);
+
  protected:
+  // File constructor.
   MultipartBlobImpl(nsTArray<RefPtr<BlobImpl>>&& aBlobImpls,
                     const nsAString& aName, const nsAString& aContentType)
       : BaseBlobImpl(NS_LITERAL_STRING("MultipartBlobImpl"), aName,
-                     aContentType, UINT64_MAX),
+                     aContentType, UINT64_MAX,
+                     MULTIPARTBLOBIMPL_UNKNOWN_LAST_MODIFIED),
         mBlobImpls(std::move(aBlobImpls)) {}
 
+  // Blob constructor.
   MultipartBlobImpl(nsTArray<RefPtr<BlobImpl>>&& aBlobImpls,
                     const nsAString& aContentType)
       : BaseBlobImpl(NS_LITERAL_STRING("MultipartBlobImpl"), aContentType,

dom/html/HTMLCanvasElement.cpp

@@ -893,8 +893,8 @@ nsresult HTMLCanvasElement::MozGetAsFileImpl(const nsAString& aName,
       do_QueryInterface(OwnerDoc()->GetScopeObject());
 
   // The File takes ownership of the buffer
-  RefPtr<File> file = File::CreateMemoryFile(win->AsGlobal(), imgData, imgSize,
+  RefPtr<File> file = File::CreateMemoryFileWithLastModifiedNow(
-                                             aName, type, PR_Now());
+      win->AsGlobal(), imgData, imgSize, aName, type);
   if (NS_WARN_IF(!file)) {
     return NS_ERROR_FAILURE;
   }