Closed Bug 827823 Opened 7 years ago Closed 5 years ago

DOMFile/DOMBlob should be CC

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: bzbarsky, Assigned: baku)

References

(Blocks 4 open bugs, )

Details

Attachments

(5 obsolete files)

No description provided.
<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
Assignee: nobody → khuey
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Blocks: 906413
Blocks: 916195
Blocks: AsyncIDB
fwiw since Blobs are already available on workers (with handwritten JSAPI bindings) I don't expect this will block doing stuff on workers.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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 ?
Flags: needinfo?(khuey)
Yes.  Blobs in workers were implemented in bug 664783.
Flags: needinfo?(khuey)
Blocks: 927610
Blocks: 937348
Depends on: 983228
Attached patch WIP (obsolete) — Splinter Review
This is just to share something. This patch is huge it will be bigger and bigger.
Blocks: 993884
Blocks: 1026303
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?
Flags: needinfo?(amarchesini)
This comment is enough, thanks.
Flags: needinfo?(amarchesini)
Attached patch patch 1 - CC on DOMFile (obsolete) — Splinter Review
This is the first patch for this porting. The next ones will be about WebIDL.
Attachment #8396293 - Attachment is obsolete: true
Attachment #8446625 - Flags: review?(ehsan)
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.
Attachment #8446625 - Flags: review?(ehsan) → review+
Keywords: leave-open
Attached patch patch 1 - CC (obsolete) — Splinter Review
Attachment #8446625 - Attachment is obsolete: true
This patch is ready to land. I'll upload the others soon.
Keywords: checkin-needed
Attached patch patch 2 - WebIDL - WIP (obsolete) — Splinter Review
I'll continue this patch when I'm back from PTO.
Attached patch patch v1 (obsolete) — Splinter Review
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.
Attachment #8446876 - Attachment is obsolete: true
Attachment #8448556 - Attachment is obsolete: true
Attachment #8466256 - Flags: review?(bzbarsky)
Attachment #8466256 - Flags: review?(bugs)
Attachment #8466256 - Flags: review?(bobbyholley)
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.
(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+.
I completely forgot about the CC patch. I'm closing this bug and I'll fine a new one for the porting.
Status: NEW → RESOLVED
Closed: 5 years ago
Keywords: leave-open
Resolution: --- → FIXED
Summary: Convert Blob to WebIDL → DOMFile/DOMBlob should be CC
Blocks: 1047483
Comment on attachment 8466256 [details] [diff] [review]
patch v1

Moving this patch to bug 1047483
Attachment #8466256 - Attachment is obsolete: true
Attachment #8466256 - Flags: review?(bzbarsky)
Attachment #8466256 - Flags: review?(bugs)
Attachment #8466256 - Flags: review?(bobbyholley)
Target Milestone: --- → mozilla33
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.