827823 - DOMFile/DOMBlob should be CC

Status: RESOLVED FIXED

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

Comment 1

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

<khuey> I have part of a patch for it
<khuey> Blob is a total disaster area right now though
<khuey> we're going to refactor the underlying objects first

Comment 2

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

fwiw since Blobs are already available on workers (with handwritten JSAPI bindings) I don't expect this will block doing stuff on workers.

Comment 3

[Jean-Yves Perrier [:teoli]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #2)
> fwiw since Blobs are already available on workers (with handwritten JSAPI
> bindings) I don't expect this will block doing stuff on workers.
Kyle: does this mean that Blob should already be listed here: https://developer.mozilla.org/en-US/docs/Web/Reference/Functions_and%20classes_available_to_workers#DOM-related_classes_available_in_workers ?

Comment 4

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

Yes.  Blobs in workers were implemented in bug 664783.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: WIP

This is just to share something. This patch is huge it will be bigger and bigger.

Comment 6

[Bobby Holley (:bholley)]

We'll also need to fix the blob/file cloning that happens in XPConnect (via Cu.cloneInto), since it looks like we've historically supported that in ObjectWrapper.jsm. Baku, are you aware of this, or should I file a separate blocking bug?

Comment 7

[Andrea Marchesini [:baku]]

This comment is enough, thanks.

Comment 8

[Andrea Marchesini [:baku]]

Attachment: patch 1 - CC on DOMFile

This is the first patch for this porting. The next ones will be about WebIDL.

Comment 9

[Andrea Marchesini [:baku]]

The patch is green on try: https://tbpl.mozilla.org/?tree=Try&rev=447fb1857e29

Comment 10

[(no longer active)]

Comment on attachment 8446625 [details] [diff] [review]
patch 1 - CC on DOMFile

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

::: content/base/public/nsDOMFile.h
@@ +156,5 @@
> +  ~DOMFile() {};
> +
> +  // The member is the real backend implementation of this DOMFile/DOMBlob.
> +  // It's thread-safe and not CC-able and it's the only element that is moved
> +  // between threads.

I think we should augment this comment by saying that we should not store any other state in this class.  Otherwise this patch will be horribly wrong!  What do you think?

@@ +162,3 @@
>  };
>  
>  // This is the virtual class for any DOMFile backend. It must be nsISupports

I gotta sign up to some C++ course...  Not sure what a virtual class is.  ;-)

::: content/base/src/nsDOMBlobBuilder.cpp
@@ +225,5 @@
>        JS::Rooted<JSObject*> obj(aCx, &element.toObject());
>        nsCOMPtr<nsIDOMBlob> blob = aUnwrapFunc(aCx, obj);
>        if (blob) {
> +        nsRefPtr<DOMFileImpl> blobImpl =
> +          static_cast<DOMFile*>(blob.get())->Impl();

We should really add an AsDOMFile() helper on nsIDOMBlob instead of casting things all over the place like this...  Wanna file that follow-up?

@@ +280,4 @@
>  
>  #ifdef DEBUG
> +    // XXX This is only safe so long as all blob implementations in our tree
> +    //     inherit nsDOMFileBase.

Please get rid of the comment too.

::: content/base/src/nsDOMFile.cpp
@@ +740,5 @@
>    mImmutable = !aMutable;
>    return rv;
>  }
>  
> +NS_IMPL_ISUPPORTS_INHERITED0(DOMFileImplBase, DOMFileImpl)

This is not needed.

::: content/base/src/nsHostObjectProtocolHandler.cpp
@@ +494,5 @@
>      NS_ASSERTION(info->mPrincipal == principal, "Wrong principal!");
>    }
>  #endif
>  
> +  nsRefPtr<DOMFileImpl> blob = static_cast<DOMFileImpl*>(blobImpl.get());

Why is this a strong ref?

@@ +582,4 @@
>      return NS_ERROR_DOM_BAD_URI;
>    }
>  
> +  nsRefPtr<DOMFileImpl> blob = static_cast<DOMFileImpl*>(blobImpl.get());

Why is this a strong ref?

::: dom/base/URL.cpp
@@ +115,5 @@
>                       const objectURLOptions& aOptions,
>                       nsString& aResult,
>                       ErrorResult& aError)
>  {
> +  nsRefPtr<DOMFile> blob = static_cast<DOMFile*>(aBlob);

Why is this a strong ref?

::: dom/base/nsGlobalWindow.cpp
@@ +7844,5 @@
> +    NS_ASSERTION(!data, "Data should be empty");
> +
> +    nsISupports* supports;
> +    if (JS_ReadBytes(reader, &supports, sizeof(supports))) {
> +      nsCOMPtr<nsIDOMBlob> file = new DOMFile(static_cast<DOMFileImpl*>(supports));

Please add a comment explaining what's happening here.

@@ +7896,2 @@
>        scTag = SCTAG_DOM_BLOB;
> +      nsRefPtr<DOMFile> file = static_cast<DOMFile*>(blob.get());

Why is this a strong ref?

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +2887,5 @@
>              ->SendPDeviceStorageRequestConstructor(child, params);
>            return NS_OK;
>          }
> +
> +        nsRefPtr<DOMFile> blob = static_cast<DOMFile*>(mBlob.get());

Why is this a strong ref?

@@ +2931,5 @@
>            ContentChild::GetSingleton()
>              ->SendPDeviceStorageRequestConstructor(child, params);
>            return NS_OK;
>          }
> +        nsRefPtr<DOMFile> blob = static_cast<DOMFile*>(mBlob.get());

Why is this a strong ref?

::: dom/workers/URL.cpp
@@ +856,5 @@
>      aRv.ThrowTypeError(MSG_DOES_NOT_IMPLEMENT_INTERFACE, &argStr, &blobStr);
>      return;
>    }
>  
> +  nsRefPtr<DOMFile> domBlob = static_cast<DOMFile*>(blob.get());

Why is this a strong ref?

::: dom/workers/WorkerPrivate.cpp
@@ +555,5 @@
> +        nsRefPtr<DOMFileImpl> fileImpl =
> +          static_cast<DOMFile*>(file.get())->Impl();
> +
> +        if (fileImpl->IsCCed()) {
> +          NS_WARNING("Cycle collected objects are not supported!");

Nit: file objects.

@@ +575,5 @@
> +        nsRefPtr<DOMFileImpl> blobImpl =
> +          static_cast<DOMFile*>(blob.get())->Impl();
> +
> +        if (blobImpl->IsCCed()) {
> +          NS_WARNING("Cycle collected objects are not supported!");

Nit: blob objects.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: patch 1 - CC

Comment 12

[Andrea Marchesini [:baku]]

This patch is ready to land. I'll upload the others soon.

Comment 13

[Andrea Marchesini [:baku]]

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

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 16

[Andrea Marchesini [:baku]]

Attachment: patch 2 - WebIDL - WIP

I'll continue this patch when I'm back from PTO.

Comment 17

[Andrea Marchesini [:baku]]

Attachment: patch v1

This patch ports DOMFile/DOMBlob to WebIDL bindings. It's a big patch I don't know how to split it in independent sub-patches.

I would like to receive a review from

. hbolley for any js/xpconnect/* changes
. smaug for dom/browser-element/* and for nsFrameMessageManager

bz, I marked you for a review, but feel free to assign it to somebody else. khuey maybe? Thanks.

This patch is the first of 3 and it's only about the porting. In a separated patch I renamed nsDOMFile.h to mozilla/dom/DOMFile.h. Then in another patch I'll get rid of nsDOMBlobBuilder, integrating its methods into DOMFile.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Did some other patch land already so that it is on Aurora already?
If so, could we please file a new bug for the new patch - to ease possible regression handling.

Comment 19

[Andrea Marchesini [:baku]]

(In reply to Olli Pettay [:smaug] from comment #18)
> Did some other patch land already so that it is on Aurora already?
> If so, could we please file a new bug for the new patch - to ease possible
> regression handling.

All the other patches I was talking about are not landed yet and they will be rebased on top of this once this is r+.

Comment 20

[Andrea Marchesini [:baku]]

I completely forgot about the CC patch. I'm closing this bug and I'll fine a new one for the porting.

Comment 21

[Andrea Marchesini [:baku]]

Comment on attachment 8466256 [details] [diff] [review]
patch v1

Moving this patch to bug 1047483