Closed Bug 1462964 Opened 2 years ago Closed 2 years ago

Get rid of nsIDOMBlob

Categories

(Core :: DOM: File, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Which is an empty interface superseded by Blob.webidl.
Blocks: 1387169
Comment on attachment 8979073 [details]
Bug 1462964: Remove obsolete nsIDOMBlob interface.

https://reviewboard.mozilla.org/r/245340/#review251512

::: dom/file/Blob.h:67
(Diff revision 1)
>    BlobImpl* Impl() const
>    {
>      return mImpl;
>    }
>  
> +  nsISupports* GetAsSupports()

The usual way to do this is to add a ToSupports overload taking Blob* to this header.  Please do that instead.

::: dom/file/Blob.cpp:40
(Diff revision 1)
>  NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(Blob)
>    NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
> -  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIDOMBlob)
> -  NS_INTERFACE_MAP_ENTRY(nsIDOMBlob)
> +  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIMutable)
> +  // Alas, there is no helper macro for this.
> +  if (aIID.Equals(NS_GET_IID(Blob))) {
> +    *aInstancePtr = do_AddRef(this).take();

Most places that implement this sort of thing do:

    foundInterface = reinterpret_cast<nsISupports*>(this);
    
or a static_cast to the "first" interface, and then fall through and let the addref in the query tail deal.  The reinterpret_cast is a more correct indication of how this is really going to work on the other end (where the nsISupports* gets static_cast to void* and then reinterpret_cast to Blob*.

I think I'd prefer the reinterpret_cast version here.

::: dom/file/tests/test_ipc_messagemanager_blob.html:25
(Diff revision 1)
>        "use strict";
>  
> +      Cu.importGlobalProperties(["Blob"]);
> +
>        addMessageListener("test:ipcClonedMessage", function(message) {
> -        if (!(message.json instanceof Ci.nsIDOMBlob)) {
> +        if (!(Blob.isInstance(message.json))) {

This is now overparenthesized.

::: dom/html/HTMLInputElement.cpp:520
(Diff revision 1)
>        if (!domBlob) {
>          continue;
>        }
>  
>        OwningFileOrDirectory* element = newFilesOrDirectories.AppendElement();
>        element->SetAsFile() = static_cast<File*>(domBlob.get());

I wonder whether it would be cleaner to use ToFile() here too...

::: dom/indexedDB/test/file.js:114
(Diff revision 1)
>  
>  function verifyBlob(blob1, blob2, fileId, blobReadHandler)
>  {
>    // eslint-disable-next-line mozilla/use-cc-etc
> -  is(blob1 instanceof Components.interfaces.nsIDOMBlob, true,
> +  is(SpecialPowers.wrap(Blob).isInstance(blob1), true,
>       "Instance of nsIDOMBlob");

s/nsIDOMBlob/Blob/ ?

::: widget/nsBaseFilePicker.cpp:56
(Diff revision 1)
>  
>      directory.forget(aResult);
>      return NS_OK;
>    }
>  
> -  nsCOMPtr<nsIDOMBlob> blob = File::CreateFromFile(aWindow, aFile);
> +  *aResult = File::CreateFromFile(aWindow, aFile).take()->GetAsSupports();

I guess this:

    RefPtr<Blob> blob = File::CreateFromFile(aWindow, aFile);
    blob.forget(aResult);
    
would fail due to the ambiguous inheritance?  Given that, the take() version is fine, with ToSupports().
Attachment #8979073 - Flags: review?(bzbarsky) → review+
Comment on attachment 8979073 [details]
Bug 1462964: Remove obsolete nsIDOMBlob interface.

https://reviewboard.mozilla.org/r/245340/#review251512

> The usual way to do this is to add a ToSupports overload taking Blob* to this header.  Please do that instead.

Yeah, I realized that afterwards. I was thinking of GlobalObject::GetAsSupports() when I wrote this, but that has a different purpose.

> Most places that implement this sort of thing do:
> 
>     foundInterface = reinterpret_cast<nsISupports*>(this);
>     
> or a static_cast to the "first" interface, and then fall through and let the addref in the query tail deal.  The reinterpret_cast is a more correct indication of how this is really going to work on the other end (where the nsISupports* gets static_cast to void* and then reinterpret_cast to Blob*.
> 
> I think I'd prefer the reinterpret_cast version here.

I looked around for other places that do this, and there doesn't seem to be a particular prevailing pattern. I was thinking about writing a follow-up to add a macro for this, and replace the existing cases.

That said, the `reinterpret_cast` version makes me nervous. It works as long as the first interface is nsISupports, or inherits from nsISupports as its first interface... But otherwise the AddRef call is invalid. And we don't have any assertions to make sure that's always true.

The `static_cast` version makes me a bit nervous, too. It works as long as we pass the right interface, and that interface doesn't change... But, again, no assertions.

This version makes me the least nervous without a helper macro, so I'd prefer to leave it as-is, and replace it with a macro as a follow-up. I think the macro should probably look something like:

    NS_INTERFACE_MAP_ENTRY_CONCRETE(Blob, nsIMutable)

and expand to something like:

    foundInterface = static_cast<_implClass*>(static_cast<interface_*>(this));
    static_assert((void*)static_cast<_implClass*>(static_cast<interface_*>(this)) ==
                  (void*)static_cast<interface_*>(result),
                  "Result pointer not valid for requested interface");

And eventually when we can use constexpr functions, something like:

    NS_INTERFACE_MAP_ENTRY_CONCRETE(Blob)
    
    ...
    
    foundInterface = ToCanonicalSupports(this);
    static_assert((void*)ToCanonicalSupports(this) == (void*)static_cast<interface_>(result));

Or maybe just go with the latter now, and make it a MOZ_ASSERT rather than a static_assert.

> I wonder whether it would be cleaner to use ToFile() here too...

I think it would, yeah.

> I guess this:
> 
>     RefPtr<Blob> blob = File::CreateFromFile(aWindow, aFile);
>     blob.forget(aResult);
>     
> would fail due to the ambiguous inheritance?  Given that, the take() version is fine, with ToSupports().

Yeah. I tried to find a good workaround for this, but gave up. I was leaning towards just defining `inline nsISupports* ToSupports(nsISupports*)` in nsISupportsImpl.h and overloading `RefPtr::forget(nsISupports**)` to call that. But that's definitely a separate bug.
Comment on attachment 8979073 [details]
Bug 1462964: Remove obsolete nsIDOMBlob interface.

https://reviewboard.mozilla.org/r/245340/#review251512

> I looked around for other places that do this, and there doesn't seem to be a particular prevailing pattern. I was thinking about writing a follow-up to add a macro for this, and replace the existing cases.
> 
> That said, the `reinterpret_cast` version makes me nervous. It works as long as the first interface is nsISupports, or inherits from nsISupports as its first interface... But otherwise the AddRef call is invalid. And we don't have any assertions to make sure that's always true.
> 
> The `static_cast` version makes me a bit nervous, too. It works as long as we pass the right interface, and that interface doesn't change... But, again, no assertions.
> 
> This version makes me the least nervous without a helper macro, so I'd prefer to leave it as-is, and replace it with a macro as a follow-up. I think the macro should probably look something like:
> 
>     NS_INTERFACE_MAP_ENTRY_CONCRETE(Blob, nsIMutable)
> 
> and expand to something like:
> 
>     foundInterface = static_cast<_implClass*>(static_cast<interface_*>(this));
>     static_assert((void*)static_cast<_implClass*>(static_cast<interface_*>(this)) ==
>                   (void*)static_cast<interface_*>(result),
>                   "Result pointer not valid for requested interface");
> 
> And eventually when we can use constexpr functions, something like:
> 
>     NS_INTERFACE_MAP_ENTRY_CONCRETE(Blob)
>     
>     ...
>     
>     foundInterface = ToCanonicalSupports(this);
>     static_assert((void*)ToCanonicalSupports(this) == (void*)static_cast<interface_>(result));
> 
> Or maybe just go with the latter now, and make it a MOZ_ASSERT rather than a static_assert.

> That said, the `reinterpret_cast` version makes me nervous. It works as long
> as the first interface is nsISupports, or inherits from nsISupports as its first
> interface...

That's fair.  If there's a vtable before the nsISupports vtable, then bad things will happen.  In that case, the only safe thing to do is to call addref on ourselves before doing any casting and then static_cast to void*.

Creating a new macro for this is the way to go for sure.

> It works as long as we pass the right interface, and that interface doesn't change...

And is the first interface, as you said (because again, the resulting void* will get reinterpret_cast to Foo* in the end).  So it's strictly less safe than the reinterpret_cast version.

I guess I'm OK with leaving the code as you have it and filing a followup for that macro.  We can use it all sorts of places.

> and expand to something like:

You can't do static_cast in a static_assert, unfortunately.  That's why binding Wrap() methods have a MOZ_ASSERT instead of a static_assert.  :(

I think the safest thing in the new macro would probably be similar to what you're doing: just addref and return without trying to go through the nsISupports machinery at all.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #4)
> You can't do static_cast in a static_assert, unfortunately.  That's why
> binding Wrap() methods have a MOZ_ASSERT instead of a static_assert.  :(

Oh, right, because static_cast does a null check...
Comment on attachment 8979073 [details]
Bug 1462964: Remove obsolete nsIDOMBlob interface.

https://reviewboard.mozilla.org/r/245340/#review251512

> Yeah. I tried to find a good workaround for this, but gave up. I was leaning towards just defining `inline nsISupports* ToSupports(nsISupports*)` in nsISupportsImpl.h and overloading `RefPtr::forget(nsISupports**)` to call that. But that's definitely a separate bug.

> I was leaning towards just defining inline nsISupports* ToSupports(nsISupports*) in nsISupportsImpl.h

It's already there: https://searchfox.org/mozilla-central/rev/2aa42f2cab4a110edf21dd7281ac23a1ea8901f9/xpcom/base/nsISupportsImpl.h#37
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #6)
> > I was leaning towards just defining inline nsISupports* ToSupports(nsISupports*) in nsISupportsImpl.h
> 
> It's already there:
> https://searchfox.org/mozilla-central/rev/
> 2aa42f2cab4a110edf21dd7281ac23a1ea8901f9/xpcom/base/nsISupportsImpl.h#37

Well, I guess that simplifies things.
https://hg.mozilla.org/mozilla-central/rev/4f8b341fea03
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.