1159401 - Make dom::File sane to use from c++

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Right now, if I have a dom::File there are some methods that are footguns to call, because they will assert if !IsFile().

We should do one of two possible things here, imo:

1)  Remove those asserts, just returns some sort of default values (empty
    strings, invalid dates, whatever).  We may want to rename the class to Blob
    in that case anyway.
2)  Have separate C++ classes for Blob and File, put the relevant methods on File
    only, have a ToFile() on Blob that asserts IsFile() and returns File* (or
    returns null if not IsFile(), or something).

Either one would be better than what we have right now, where a File in the C++ might just mean Blob.

Thoughts?

Comment 1

[Andrea Marchesini [:baku]]

What about this: instead removing these asserts, we can move them from  the FileImpl to the dom::File class. From C++ usually it's better to use FileImpl and leave File just for the JS DOM.

What you suggest as section options, is actually FileImpl.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think doing 1) would be the simplest.

I don't see a purpose to these assertions anyway since both Blobs and Files may or may not actually be backed by an OS-file, so whether functions like GetMozFullPath can return a "real value" is independent of if it's a File or a Blob object.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> we can move them from  the FileImpl to the dom::File class

That doesn't solve my problem, which is that I have an array of dom::File, some of which are JS File and some of which are JS Blob and the code right now assumes they're all JS File and calls these methods.

> From C++ usually it's better to use FileImpl

All the stuff around file inputs is working with an array of File.

> What you suggest as section options

I'm not sure what you mean here....

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

2) seems to obviously be the correct design

Comment 5

[Andrea Marchesini [:baku]]

Attachment: blob.patch

Comment 6

[Andrea Marchesini [:baku]]

Attachment: blob.patch

Comment 7

[Andrea Marchesini [:baku]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4dcccb385511

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8599896 [details] [diff] [review]
blob.patch

> ArchiveRequest::GetFileResult(JSContext* aCx,
>+      if (!GetOrCreateDOMReflector(aCx, file, aValue)) {

Could probably use ToJSValue here too.  This comes up multiple places in the patch.

>+Blob::Create(nsISupports* aParent, const nsAString& aContentType,
>+                uint64_t aStart, uint64_t aLength)

Fix indent?

>+    nsCOMPtr<nsIDOMBlob> domBlob = this;
>+    nsCOMPtr<nsIDOMFile> domFile = do_QueryInterface(domBlob);

Should just be able to do_QueryObject(this).

But also, can we not just have a protected virtual method to answer the "am I actually a File" question instead of relying on the much slower QI?

>+Blob::ToFile()

Seems like this could up front check IsFile() and return nullptr if not, then outdent the rest of the logic.

>+Blob::GetLastModifiedDate(ErrorResult& aRv)

I'm not sure I follow why this is on Blob, not File.  It ends up asserting mIsFile, no?  And in IDL terms it's on File, not on Blob.

>+Blob::GetLastModified(ErrorResult& aRv)

Likewise.

>+File::Create(nsISupports* aParent, FileImpl* aImpl)

Are there any actual consumers that would expect this to return null?  If not, can we MOZ_CRASH if !aImpl->IsFile()?  I guess there's nsFilePickerProxy, but we could just make that check IsFile() on the FileImpl, right?

>+  already_AddRefed<File> ToFile();

This needs some documentation.

>+  already_AddRefed<File> ToFile(const nsAString& aName) const;

And this.  Esp. given how much the differ from each other!

>+  static File*
>+  Create(nsISupports* aParent, FileImpl* aImpl);

Should document the constraints on FileImpl.

>+  // File constructor should never be used directly. Use Blob instead.

Blob constructor should never be used either.  Do you mean use Blob::Create?  Or File::Create?  Or something else?

>+nsDOMFileReader::ReadAsArrayBuffer(nsIDOMBlob* aBlob, JSContext* aCx)
...
>+  nsRefPtr<Blob> blob = static_cast<Blob*>(aBlob);

So I _think_ this is a problem.

The issue is that File now multiply-inherits from nsIDOMBlob with your changes: one path via Blob and one path via nsIDOMFile.  So if aBlob is actually a File, and that's a pointer to the nsIDOMBlob version that came from nsIDOMFile (which is NOT the canonical nsIDOMBlob* for a File, of course), this cast will do the wrong thing.

We need to either QI to nsIDOMBlob in all places where we do casts like this, or get rid of the multiple inheritance by making nsIDOMFile no longer inherit from nsIDOMBlob.  I would prefer the latter, I think; it's way less surprising.  Might want a comment in the IDL explaining why the inheritance is not there, of course.

>+nsFormData::Append(const nsAString& aName, Blob& aBlob,
>+  nsRefPtr<Blob> blob = CreateNewFileInstance(aBlob, aFilename);

The whole point of CreateNewFileInstance is to return a File.  So why are we assigning it into a Blob?  We should be assigning into a File, and AddNameFilePair should take a File* for the second arg.  And then it won't need the weird ToFile and assert.  In fact, you could just leave the old code for it in the header, with aBlob renamed to aFile.

>+++ b/dom/events/DataTransfer.cpp
>+          File::Create(GetParentObject(),
>+                       static_cast<FileImpl*>(fileImpl.get()));

Why is that static_cast, or the .get() call needed?  Seems like you should be able to just pass fileImpl here.

>+++ b/dom/filesystem/FileSystemTaskBase.cpp
>+  {
>+    ErrorResult rv;
>+    aFile->GetSize(rv);
>+  }

We should either SuppressException() or assert that there is no exception or something here.

>+    aFile->GetLastModified(rv);

Likewise.

And maybe document that we're just calling these for the side-effects?  Because it looks pretty confusing as it is.

> nsFSMultipartFormData::AddNameFilePair(const nsAString& aName,
>     // Since Bug 1127150, any Blob received from FormData must be a File

Can we just leave this function taking a File*?  That will make it clearer what's going on, and when that invariant changes the compiler will tell us that we need to change this function accordingly.

Same for the other AddNameFilePair impls.

>@@ -385,18 +385,19 @@ MmsMessage::GetData(ContentParent* aPare
>+      if (rv.Failed()) {
>         NS_WARNING("Failed to get last modified date!");

rv.SuppressException()?

We should consider a followup to rename FileImpl to BlobImpl...

r=me with the above nits fixed (esp. the nsIDOM* inheritance thing).  Thank you for doing this!

Comment 9

[Andrea Marchesini [:baku]]

> We need to either QI to nsIDOMBlob in all places where we do casts like
> this, or get rid of the multiple inheritance by making nsIDOMFile no longer
> inherit from nsIDOMBlob.  I would prefer the latter, I think; it's way less
> surprising.  Might want a comment in the IDL explaining why the inheritance
> is not there, of course.


What about if we get rid of nsIDOMFile completely? I see just Firefox OS emulator addon using nsIDOMFile. Do you think it's doable?

Comment 10

[Boris Zbarsky [:bzbarsky]]

> What about if we get rid of nsIDOMFile completely?

Sounds lovely to me, esp. if we can do it as a followup instead of blocking this patch on it.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: patch 2

It seems that I don't have to include all the members of nsIDOMBlob in nsIDOMFile, but tell me if I'm wrong.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eab217e34c1b

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8603822 [details] [diff] [review]
patch 2

r=me if you mention in the comment that we want to avoid multiple inheritance so we can downcast from nsIDOMBlob to Blob safely.

Comment 13

[Andrea Marchesini [:baku]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/96a3ebfe09d8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4287533203fb

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/49678aa590e2

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

Comment 15

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/70c63c8546e3
https://hg.mozilla.org/integration/mozilla-inbound/rev/adfee1efb1e1

Comment 16

[Daniel Holbert [:dholbert]]

The first patch here introduced some -Winconsistent-missing-override build warnings (in clang 3.6+) for nsFormSubmission.cpp -- specifically, for the class "nsFSURLEncoded" methods SupportsIsindexSubmission() & AddIsindex().

(Those methods (and their overriding status) predated this patch.  I think the warnings appeared just now because the patch marked a *different* overridding method as 'override', which made the lack-of-override-keyword on these other methods appear 'inconsistent', hence -Winconsistent-missing-override.)

Pushed a patch with rs=ehsan (granted in bug 1126447 comment 2):
https://hg.mozilla.org/integration/mozilla-inbound/rev/5eec0168c916

Comment 17

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

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/86efbb9287a4 for b2g build bustage, even after that followup:
https://treeherder.mozilla.org/logviewer.html#?job_id=9750973&repo=mozilla-inbound

Comment 18

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

And because I pushed that backout before I had realized there was a followup, here's a backout of the followup: https://hg.mozilla.org/integration/mozilla-inbound/rev/0c80b9af0593

Comment 19

[Daniel Holbert [:dholbert]]

(When this re-lands, ideally it'd be great to have the 'override' followup just merged into the first patch here, so that every intermediate cset builds cleanly with clang > 3.6.)

Comment 20

[Andrea Marchesini [:baku]]

Attachment: blob.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3bf5ee35d40

single patch.

Comment 21

[Pulsebot]

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

Comment 22

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

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