876683 - Refactor DOM File code

Status: RESOLVED FIXED

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

Description

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

Our DOM blob/file code is a disaster.  It needs to be refactored so we can convert it to WebIDL.

IMO the most important change to make is to separate whether the object is a blob or a file from where the data lives.  So we would split it into two C++ objects: a front end object that implements the Blob or the File interface and a back end "backing store" object that implements actual data transfer stuff.

Other issues to deal with:

We currently use the same nsDOMFile on multiple threads.  After we start wrapper caching we can't do that anymore.  So we need a separate front end object for each thread.  Thankfully postMessage()ing to a worker and back returns a new Blob/File, so we don't have to try to keep track of them.

Some data sources are CCd and can't be used off the main thread right now.  We have to at least not crash when postMessage()ing those to a worker.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: file.patch

It's not perfect: it leaks, and I don't know if all the tests pass but the basic idea is this:

DOMFile is a single class. It proxies any method call to an internal |nsRefPtr<DOMFileImpl> mImpl|.

DOMFileImpl is a virtual class, extended by DOMFileImplBase, DOMFileImplMemory, DOMFileImplTemporaryFileBlob, DOMFileImplFile, DOMFileImplBaseCC, ...

The next steps are:

1. CC for DOMFile.
2. WebIDL

Comment 2

[Andrea Marchesini [:baku]]

Attachment: file.patch

Almost green on try. There is still an issue with IPC but for the rest it works.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: WIP

Comment 4

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 5

[Andrea Marchesini [:baku]]

Attachment: patch

A couple of issues about try are fixed in this patch.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: file.patch

Comment 7

[(no longer active)]

Comment on attachment 8445672 [details] [diff] [review]
file.patch

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

r=me with the comments below addressed.  Also flagging Ben in case he wants to look this over.

::: content/base/public/nsDOMFile.h
@@ +63,5 @@
> +  NS_DECL_NSIXHRSENDABLE
> +  NS_DECL_NSIMUTABLE
> +
> +  // XXX bug 827823 will make this class CC and not thread-safe
> +  NS_DECL_THREADSAFE_ISUPPORTS

This is going to mean that DOMFileCC is going to have two refcounts, one here and the other one declared in DOMFileCC itself.  I suggest refactoring most of this class into DOMFileBase which would be pretty similar to the current setup with nsDOMFileBase.  The ctor functions would of course need to still go on DOMFile.

@@ +78,5 @@
> +  Constructor(const nsAString& aContentType, uint64_t aLength);
> +
> +  static already_AddRefed<DOMFile>
> +  Constructor(const nsAString& aContentType, uint64_t aStart,
> +              uint64_t aLength);

Nit: please call these Create.

@@ +87,5 @@
> +                      uint64_t aLastModifiedDate);
> +
> +  static already_AddRefed<DOMFile>
> +  ConstructorInMemory(void* aMemoryBuffer, uint64_t aLength,
> +                      const nsAString& aContentType);

Nit: please call this CreateMemoryFile.

@@ +92,5 @@
> +
> +  static already_AddRefed<DOMFile>
> +  ConstructorTemporaryFileBlob(PRFileDesc* aFD, uint64_t aStartPos,
> +                               uint64_t aLength,
> +                               const nsAString& aContentType);

Nit: CreateTemporatyFileBlob.

@@ +111,5 @@
> +  ConstructFromFile(nsIFile* aFile, indexedDB::FileInfo* aFileInfo);
> +
> +  static already_AddRefed<DOMFile>
> +  ConstructFromFile(nsIFile* aFile, const nsAString& aName,
> +                    const nsAString& aContentType);

Nit: CreateFromFile.

@@ +113,5 @@
> +  static already_AddRefed<DOMFile>
> +  ConstructFromFile(nsIFile* aFile, const nsAString& aName,
> +                    const nsAString& aContentType);
> +
> +  DOMFile(DOMFileImpl* aImpl);

Nit: explicit.

@@ +116,5 @@
> +
> +  DOMFile(DOMFileImpl* aImpl);
> +  virtual ~DOMFile() {};
> +
> +  DOMFileImpl* GetImpl() const

Nit: please call this Impl(), since it never returns null.

@@ +130,5 @@
> +
> +  bool IsFile() const;
> +
> +  void SetLazyData(const nsAString& aName, const nsAString& aContentType,
> +                  uint64_t aLength, uint64_t aLastModifiedDate);

Nit: indentation.

@@ +141,5 @@
> +  NS_IMETHOD Initialize(nsISupports* aOwner, JSContext* aCx, JSObject* aObj,
> +                        const JS::CallArgs& aArgs) MOZ_OVERRIDE;
> +
> +private:
> +  nsRefPtr<DOMFileImpl> mImpl;

Nit: please make this const for now.

Also, I think this setup deserves some documentation explaining why we use the pimpl idiom here, and what it buys us, etc.

@@ +158,5 @@
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_CLASS_AMBIGUOUS(DOMFileCC, nsIDOMFile)
> +};
> +
> +class DOMFileImpl : public nsISupports

Please add a comment stating why this needs to be an nsISupports.

@@ +221,5 @@
> +                           const nsAString& aContentType,
> +                           uint64_t aLength,
> +                           uint64_t aLastModifiedDate) = 0;
> +
> +  virtual bool IsMemoryFile() = 0;

Nit: please check to see if some of the non-const methods above can be const.

@@ +297,5 @@
> +    // Ensure non-null mContentType by default
> +    mContentType.SetIsVoid(false);
> +  }
> +
> +  virtual ~DOMFileImplBase() {}

You shouldn't need this, right?

@@ +569,5 @@
> +class DOMFileImplFile MOZ_FINAL : public DOMFileImplBase
> +{
> +public:
> +  // Create as a file
> +  DOMFileImplFile(nsIFile* aFile)

Nit: explicit.

@@ +680,5 @@
> +  nsresult GetInternalStream(nsIInputStream**) MOZ_OVERRIDE;
> +
> +  void SetPath(const nsAString& aFullPath);
> +
> +protected:

final class -> private!

::: content/base/src/nsDOMBlobBuilder.cpp
@@ +313,4 @@
>    }
>  
> +  DOMFile* domFile = static_cast<DOMFile*>(blob);
> +  MOZ_ASSERT(domFile);

How can this assertion fire?  See the if condition above!

::: content/base/src/nsDOMBlobBuilder.h
@@ +24,4 @@
>  {
>  public:
>    // Create as a file
> +  DOMMultipartFileImpl(nsTArray<nsCOMPtr<nsIDOMBlob>> aBlobs,

Uh-oh.  I think we should pass this by a const reference.  Please file a follow-up.

@@ +34,5 @@
>      SetLengthAndModifiedDate();
>    }
>  
>    // Create as a blob
> +  DOMMultipartFileImpl(nsTArray<nsCOMPtr<nsIDOMBlob>>& aBlobs,

This should be const right?

@@ +44,5 @@
>      SetLengthAndModifiedDate();
>    }
>  
>    // Create as a file to be later initialized
> +  DOMMultipartFileImpl(const nsAString& aName)

Nit: explicit.

::: content/base/src/nsDOMFile.cpp
@@ +129,5 @@
> +  NS_INTERFACE_MAP_ENTRY(nsIXHRSendable)
> +  NS_INTERFACE_MAP_ENTRY(nsIMutable)
> +  NS_INTERFACE_MAP_ENTRY(nsIJSNativeInitializer)
> +  NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO_CONDITIONAL(File, IsFile())
> +  NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO_CONDITIONAL(Blob, !(IsFile()))

Nit: please assert that the input IID to this method is not the IID of nsIRemoteBlob.

@@ +142,4 @@
>  {
> +  nsRefPtr<DOMFileImpl> impl =
> +    new DOMFileImplBase(aName, aContentType, aLength, aLastModifiedDate);
> +  nsRefPtr<DOMFile> file = new DOMFile(impl);

I suggest making the DOMFile and DOMFileCC constructors accept an already_AddRefed so that you don't have to addref and release unnecessarily here.

@@ +742,5 @@
> +  NS_INTERFACE_MAP_ENTRY(nsISupports)
> +NS_INTERFACE_MAP_END
> +
> +NS_IMPL_ADDREF(DOMFileImplBase)
> +NS_IMPL_RELEASE(DOMFileImplBase)

Just use NS_IMPL_ISUPPORTS?

@@ +781,2 @@
>  {
> +  nsCOMPtr<nsIDOMBlob> t =

Nice variable name!  Makes the code so much faster.  ;-)

@@ +929,2 @@
>  
> +/* static */ StaticAutoPtr<LinkedList<DOMFileImplMemory::DataOwner> >

Nit: >>

@@ +932,3 @@
>  
>  /* static */ bool
> +DOMFileImplMemory::DataOwner::sMemoryReporterRegistered;

Nit: please init to false explicitly.

@@ +1033,5 @@
> +{
> +  if (aStart + aLength > mLength)
> +    return nullptr;
> +
> +  nsCOMPtr<nsIDOMBlob> t =

Nice variable name here too!

::: dom/base/nsDOMWindowUtils.cpp
@@ +2970,5 @@
> +  nsCOMPtr<nsIDOMBlob> blob = do_QueryInterface(file);
> +  MOZ_ASSERT(blob);
> +
> +  nsRefPtr<DOMFile> domFile = static_cast<DOMFile*>(blob.get());
> +  MOZ_ASSERT(domFile);

static_cast can't make things null!

::: dom/filehandle/File.cpp
@@ +17,5 @@
>  
> +// Create as a file
> +FileImpl::FileImpl(const nsAString& aName, const nsAString& aContentType,
> +                   uint64_t aLength, nsIFile* aFile, FileHandle* aFileHandle)
> +: DOMFileImplBase(aName, aContentType, aLength),

Nit: indentation.

@@ +28,5 @@
>  // Create as a stored file
> +FileImpl::FileImpl(const nsAString& aName, const nsAString& aContentType,
> +                   uint64_t aLength, nsIFile* aFile, FileHandle* aFileHandle,
> +                   indexedDB::FileInfo* aFileInfo)
> +: DOMFileImplBase(aName, aContentType, aLength),

Nit: indentation.

@@ +39,5 @@
>  
>  // Create slice
> +FileImpl::FileImpl(const FileImpl* aOther, uint64_t aStart, uint64_t aLength,
> +                   const nsAString& aContentType)
> +: DOMFileImplBase(aContentType, aOther->mStart + aStart, aLength),

Nit: indentation.

::: dom/filehandle/File.h
@@ +38,5 @@
>  
> +  nsresult GetInternalStream(nsIInputStream** aStream) MOZ_OVERRIDE;
> +
> +  void Unlink();
> +  void Traverse(nsCycleCollectionTraversalCallback &aCb);

Nit: MOZ_OVERRIDE on these too please.

Comment 8

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

Comment on attachment 8445672 [details] [diff] [review]
file.patch

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

r=me on dom/ipc and dom/indexedDB with these fixes:

::: dom/indexedDB/IDBObjectStore.cpp
@@ +740,5 @@
>    NS_ASSERTION(!IndexedDatabaseManager::IsMainProcess(), "Wrong process!");
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
>  
> +  nsRefPtr<DOMFile> blob = static_cast<DOMFile*>(aBlob);
> +  nsCOMPtr<nsIRemoteBlob> remoteBlob = do_QueryInterface(blob->GetImpl());

Please make sure that debug builds of DOMFile will assert when QI'd to nsIRemoteBlob.

::: dom/ipc/Blob.cpp
@@ +79,5 @@
>                                public nsISeekableStream,
>                                public nsIIPCSerializableInputStream
>  {
>    nsCOMPtr<nsIInputStream> mStream;
> +  nsRefPtr<mozilla::dom::DOMFileImplBase> mSourceBlob;

Nit: 'mozilla::dom::' shouldn't be needed anywhere here

@@ +755,3 @@
>    GetLastModifiedDate(JSContext* cx,
>                        JS::MutableHandle<JS::Value> aLastModifiedDate)
>                        MOZ_OVERRIDE;

Wait, what's up with these? Please add 'virtual' to all overrides, here and elsewhere.

@@ +1453,5 @@
>    GetLastModifiedDate(JSContext* cx,
>                        JS::MutableHandle<JS::Value> aLastModifiedDate)
>                        MOZ_OVERRIDE;
>  
> +  void*

All these too.

Comment 9

[Andrea Marchesini [:baku]]

Attachment: file.patch

Green on try: https://tbpl.mozilla.org/?tree=Try&rev=060abbace1e4
Ready to land it?

Comment 10

[Andrea Marchesini [:baku]]

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

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d874785394e1