Porting DOMFile/DOMBlob to WebIDL

RESOLVED FIXED in mozilla35

Status

()

defect
RESOLVED FIXED
5 years ago
3 months ago

People

(Reporter: baku, Assigned: baku)

Tracking

(Blocks 3 bugs, {dev-doc-needed})

Trunk
mozilla35
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 10 obsolete attachments)

11.16 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
274.73 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
28.40 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
369.78 KB, patch
Details | Diff | Splinter Review
8.52 KB, patch
smaug
: review+
Details | Diff | Splinter Review
7.54 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Assignee

Description

5 years ago
No description provided.
Assignee

Comment 1

5 years ago
Posted patch patch 1 - v1 (obsolete) — Splinter Review
This patch is about the porting of DOMFile/DOMBlob to WebIDL bindings. It's a big patch but 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 #8466273 - Flags: review?(bzbarsky)
Attachment #8466273 - Flags: review?(bugs)
Attachment #8466273 - Flags: review?(bobbyholley)
Comment on attachment 8466273 [details] [diff] [review]
patch 1 - v1

I spoke to Boris on IRC and offered to help with his review queue.  He suggested I could do the majority of the review here and pass off to Ollie or Kyle for the webidl parts requiring a DOM peer.
Attachment #8466273 - Flags: review?(bzbarsky) → review?(bkelly)
Comment on attachment 8466273 [details] [diff] [review]
patch 1 - v1

I'm going to punt most of the review to bkelly, but here are some comments on the IDL:

1) .type doesn't need GetterThrows.  Just return an empty string from DOMFileImplFile::GetType if there is no MIME service, just like you would if there is a MIME service that doesn't know about the file type.

2)  Why does the "contentType" argument to slice not have "" as the default value?  Seems like it should; please raise a spec issue if you agree that this gives the same black-box behavior as the spec doing the empty string thing in prose.

3)  In the File constructor, "fileName" should be a ScalarValueString, right?

4) 
>+ // These constructors is just for chrome callee:

s/is/are/ and s/callee/callers/?

5) The FilePropertyBag dictionary in this patch looks nothing like the one in the spec.  Why not?  If the spec is just wrong, raise issues as needed, I guess...

6) Need a spec issue about the prose about FilePropertyBag being confused.  The dictionary is always present, and it always has a type member.

7) Is there a bug on deprecating/removing lastModifiedDate?

8) Should mozFullPath really be exposed to the web?  That seems moderately fishy.

r=me on the idl with those issues addressed.
Attachment #8466273 - Flags: review+
Maybe I am looking at the wrong spec, but shouldn't there also be a constructor like this on File:

  [Constructor(Blob fileBits, [EnsureUTF16] DOMString fileName)]

Looking at this:

  http://www.w3.org/TR/FileAPI/#file
Welcome to the world of the W3C.  You're looking at the last "officially published" draft, dated 2013-09-12.

The editor's draft is linked from that and is at http://dev.w3.org/2006/webapi/FileAPI/ and was last updated 2014-07-15.  Use that as your spec reference.  ;)
Comment on attachment 8466273 [details] [diff] [review]
patch 1 - v1

Could you ask review after bkelly has gone through this once, so that
we don't both need to comment on the same issues.
(This is a *huge* patch after all.)
Attachment #8466273 - Flags: review?(bugs)
This patch also needs to be rebased as it does not apply cleanly to mozilla-central.
Assignee

Comment 9

5 years ago
Comment on attachment 8466273 [details] [diff] [review]
patch 1 - v1

I'll ask a review to smaug and bholley when bkelly finishes its review process.
Attachment #8466273 - Flags: review?(bobbyholley)
Assignee

Comment 10

5 years ago
> 3)  In the File constructor, "fileName" should be a ScalarValueString, right?

Do we have such type?
(In reply to Andrea Marchesini (:baku) from comment #10)
> > 3)  In the File constructor, "fileName" should be a ScalarValueString, right?
> 
> Do we have such type?

It just landed in inbound on Friday via bug 1025183.
I'm about half done.  I'll have to finish up tomorrow morning.
Assignee

Comment 13

5 years ago
I'm uploading the other patches but I don't ask for a review yet.
Assignee

Updated

5 years ago
Attachment #8466273 - Attachment description: patch 1 - v1 → patch 1 - v1 - DOMFile/DOMBlob to WebIDL
Assignee

Updated

5 years ago
Blocks: 1048291
Assignee

Updated

5 years ago
Blocks: 1048293
Comment on attachment 8466273 [details] [diff] [review]
patch 1 - v1

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

Overall this looks good.  It would definitely be cleaner if we could use DOMFile in more places instead of converting back and forth with nsIDOMBlob.  I understand this is coming in a follow-up patch, though.

I think the only major question/concern I have is about DOMFile::GetParentObject().  Right now it returns nullptr in all cases which does not seem correct.

I was previously concerned about using static_cast to convert between nsIDOMBlob and DOMFile, but it was explained to me in IRC this is safe as long as the XPCOM interface is builtintype and there is only one implementation.

I did not review the xpconnect portions.

r=me with these items addressed.

::: content/base/public/nsDOMFile.h
@@ +8,3 @@
>  
>  #include "mozilla/Attributes.h"
> +

Nit: Remove extra line.

@@ +22,2 @@
>  #include "mozilla/dom/DOMError.h"
> +#include "mozilla/dom/FileBinding.h"

Please move the includes for BlobBinding.h and FileBinding.h to nsDOMFile.cpp.  All you need in the header here are forward includes for BlobPropertyBag and FilePropertyBag.

You'll also then need to add the binding headers to cpp files that use UNWRAP_OBJECT.

@@ +61,5 @@
> +/* FOLLOWUP TODO:
> +1. remove nsDOMBlobBuilder.h
> +2. rename nsDOMFile.h/cpp to DOMFile.h/cpp
> +3. rename nsDOMFileList to DOMFileList
> +*/

Can you reference bug numbers for these?

@@ +77,5 @@
>    NS_DECL_CYCLE_COLLECTING_ISUPPORTS
>    NS_DECL_CYCLE_COLLECTION_CLASS_AMBIGUOUS(DOMFile, nsIDOMFile)
>  
> +  static already_AddRefed<DOMFile> CreateDOMBlob();
> +

I don't see this method referenced.  It seems you've removed its only use in nsLayoutModule.cpp.  Can it just be removed?

@@ +152,5 @@
> +  // WebIDL methods
> +  nsISupports* GetParentObject() const
> +  {
> +    return nullptr;
> +  }

If I understand correctly, this should return a valid object when the File is constructed within a Window context.  If thats not the case, can you document why?

It seems most bindings just return the value of GlobalObject::GetAsSupports() from aGlobal passed into the constructor.

@@ +196,5 @@
> +              ErrorResult& aRv);
> +
> +  virtual JSObject* WrapObject(JSContext* aCx) MOZ_OVERRIDE;
> +
> +  uint64_t GetSize(ErrorResult& aRv);

Can this be made const?  The webidl binding allows it and might prevent some const_casts further up the call stack.  Please double-check the const-ness of the other accessors as well.

@@ +200,5 @@
> +  uint64_t GetSize(ErrorResult& aRv);
> +
> +  void GetType(nsAString& aType, ErrorResult& aRv);
> +
> +  int64_t GetLastModified(ErrorResult& aRv);

Maybe note in a comment that GetName() for the WebIDL name attribute is declared as part of NS_DECL_NSIDOMFILE above.

::: content/base/src/nsDOMBlobBuilder.cpp
@@ +157,5 @@
> +
> +    else if (data.IsArrayBuffer()) {
> +      const ArrayBuffer& buffer = data.GetAsArrayBuffer();
> +      buffer.ComputeLengthAndData();
> +      aRv = blobSet.AppendVoidPtr(buffer.Data(), buffer.Length());

Can you expand the tests to verify that this works as expected with zero length ArrayBuffers.

@@ +173,5 @@
> +      }
> +    }
> +
> +    else {
> +      MOZ_ASSUME_UNREACHABLE("This should not happen.");

Better message please.  Maybe something like "Impossible blob data type"?

@@ +365,2 @@
>  {
> +  nsCString utf8Str = NS_ConvertUTF16toUTF8(aString);

I think this can be simplified to:

  NS_ConvertUTF16toUTF8 utf8Str(aString);

::: content/base/src/nsDOMBlobBuilder.h
@@ +76,2 @@
>  
> +  void InitializeChromeFile(DOMFile& aData,

Can this be |const DOMFile& aData|?

@@ +113,5 @@
> +
> +  void SetFromNsIFile(bool aValue)
> +  {
> +    mIsFromNsiFile = aValue;
> +  }

Nit: Can you fix these to be spelled the same?  "NsI" vs "Nsi".  I don't have a preference for which is chosen.

::: content/base/src/nsDOMFile.cpp
@@ +342,5 @@
> +  if (rv.Failed()) {
> +    return rv.ErrorCode();
> +  }
> +
> +  JS::Rooted<JSObject*> date(aCx, JS_NewDateObjectMsec(aCx, value));

I think this can be simplified to avoid duplicating some code by doing something like this:

  ErrorResult rv;
  Date value = GetLastModifiedDate(rv);
  if (rv.Failed()) {
    return rv.ErrorCode();
  }

  if (!value.ToDateObject(aCx, aDate)) {
    return NS_ERROR_OUT_OF_MEMORY;
  }

  return NS_OK;

@@ +395,5 @@
>  DOMFile::GetSize(uint64_t* aSize)
>  {
> +  ErrorResult rv;
> +  *aSize = GetSize(rv);
> +  return rv.ErrorCode();

MOZ_ASSERT(aSize) above here.

@@ +423,5 @@
>  DOMFile::GetMozLastModifiedDate(uint64_t* aDate)
>  {
> +  ErrorResult rv;
> +  *aDate = GetLastModified(rv);
> +  return rv.ErrorCode();

MOZ_ASSERT(aDate) above here.

@@ +480,5 @@
> +  }
> +
> +  Optional<nsAString> contentType;
> +  if (aArgc > 1) {
> +    contentType = &aContentType;

I believe this should be |if (aArgc > 2)|.

On a related note, can you expand test_fileapi_slice.html to test the content type argument?

@@ +577,5 @@
> +/* static */ already_AddRefed<DOMFile>
> +DOMFile::Constructor(const GlobalObject& aGlobal, ErrorResult& aRv)
> +{
> +  nsRefPtr<DOMMultipartFileImpl> impl = new DOMMultipartFileImpl();
> +  MOZ_ASSERT(!impl->IsFile());

The other constructors place this assert after InitializeBlob().  Why the different ordering here?

@@ +616,5 @@
> +        ErrorResult& aRv)
> +{
> +  nsRefPtr<DOMMultipartFileImpl> impl = new DOMMultipartFileImpl(aName);
> +
> +// TODO: read the spec about the FilePropertyBag

Is this stale at this point?  If not, can we get a bug number in the comment to follow-up?

@@ +715,5 @@
>  
> +  int64_t start = aStart.WasPassed() ? aStart.Value() : 0;
> +  int64_t end = aEnd.WasPassed() ? aEnd.Value() : (int64_t)thisLength;
> +
> +  nsString contentType;

I believe we favor nsAutoString for stack variables.

@@ +907,5 @@
> +  ErrorResult error;
> +  *aContentLength = GetSize(error);
> +  if (NS_WARN_IF(error.Failed())) {
> +    return error.ErrorCode();
> +  }

MOZ_ASSERT(aContentLength) above here.

@@ +912,3 @@
>  
>    nsString contentType;
> +  this->GetType(contentType, error);

Nit: Lets drop the |this->| as this file seems to mostly not use it and you removed it from GetSize() above.

@@ +912,4 @@
>  
>    nsString contentType;
> +  this->GetType(contentType, error);
> +  if (error.Failed()) {

NS_WARN_IF() here.

@@ +944,1 @@
>      nsString dummyString;

nsAutoString here.

@@ +944,3 @@
>      nsString dummyString;
> +    GetType(dummyString, error);
> +    if (error.Failed()) {

NS_WARN_IF() here.

::: content/base/src/nsDOMFileReader.h
@@ +18,5 @@
>  #include "prtime.h"
>  #include "nsITimer.h"
>  #include "nsIAsyncInputStream.h"
>  
> +#include "nsDOMFile.h"

This can be replaced with a forward declaration of DOMFile.

::: content/base/src/nsFormData.h
@@ +5,5 @@
>  #ifndef nsFormData_h__
>  #define nsFormData_h__
>  
>  #include "mozilla/Attributes.h"
> +#include "nsDOMFile.h"

This can be replaced with a forward declaration.

::: content/base/src/nsFrameMessageManager.cpp
@@ +210,5 @@
>      uint32_t length = blobs.Length();
>      blobList.SetCapacity(length);
>      for (uint32_t i = 0; i < length; ++i) {
>        typename BlobTraits<Flavor>::BlobType* protocolActor =
> +        aManager->GetOrCreateActorForBlob(static_cast<DOMFile*>(blobs[i].get()));

Why do you need a cast here?  Shouldn't nsRefPtr<DOMFile>::get() return DOMFile* already?

::: content/base/src/nsHostObjectProtocolHandler.cpp
@@ +16,5 @@
>  #include "nsIMemoryReporter.h"
>  #include "mozilla/Preferences.h"
>  #include "mozilla/LoadInfo.h"
>  
> +using mozilla;

This doesn't compile.  It should be |using namespace mozilla;|.

That being said, I think it would be nicer to be explicit about the class you need.  So I would prefer if it was |using mozilla::ErrorResult;|.  This also helps minimize unified build bleed over.

@@ +518,4 @@
>  
>    if (blob->IsFile()) {
>      nsString filename;
> +    blob->GetName(filename);

Why drop the NS_ENSURE_SUCCESS?  Even if it always returns NS_OK today, it may not always do so.  It would seem the safe thing to do would be to leave the return code checking in.

::: content/base/src/nsXMLHttpRequest.h
@@ +355,3 @@
>      {
>        mValue.mBlob = aBlob;
>      }

Nit: Might be slightly nicer to pass a DOMFile& reference into the constructor since you already have one where this is called.  Then convert to a pointer when assigning to mValue.mBlob.

::: dom/archivereader/ArchiveReader.cpp
@@ +50,2 @@
>                               const nsACString& aEncoding)
> +  : mBlob(&aBlob)

Rather than cast back to an XPCOM interface here (as mBlob is nsCOMPtr<nsIDOMBlob>), would it be better to just change mBlob to be nsRefPtr<DOMFile>?

::: dom/archivereader/ArchiveZipEvent.cpp
@@ +85,5 @@
>    }
>  
> +  nsCOMPtr<nsPIDOMWindow> window =
> +    do_QueryInterface(aArchiveReader->GetParentObject());
> +

This variable doesn't seem to be used.  Can you explain what this statement is here for?

::: dom/base/MessagePort.cpp
@@ +10,5 @@
>  #include "mozilla/dom/MessagePortBinding.h"
>  #include "mozilla/dom/MessagePortList.h"
>  #include "mozilla/dom/StructuredCloneTags.h"
>  #include "nsContentUtils.h"
> +#include "nsDOMFile.h"

#include "mozilla/dom/BlobBinding.h" for UNWRAP_OBJECT(Blob).

::: dom/browser-element/BrowserElementParent.jsm
@@ +542,5 @@
>      }
>      else {
>        debug("Got error in gotDOMRequestResult.");
> +      Services.DOMRequest.fireErrorAsync(req,
> +        Cu.cloneInto(data.json.errorMsg, this._window));

Can you explain why these clones are now necessary?

::: dom/camera/DOMCameraControl.cpp
@@ +1182,2 @@
>    ErrorResult ignored;
> +  cb->Call(*blob, ignored);

Please add a MOZ_ASSERT(blob) or MOZ_ASSERT(aPicture) above this.

::: dom/canvas/ImageEncoder.cpp
@@ +48,5 @@
>          jsapi.Init(mGlobal);
>          JS_updateMallocCounter(jsapi.cx(), mImgSize);
>        }
>  
> +      mCallback->Call(*blob, rv);

Please add a MOZ_ASSERT(blob) above this.

::: dom/contacts/ContactManager.js
@@ +91,5 @@
>  
>    _convertContact: function(aContact) {
> +    var contact = Cu.cloneInto(aContact, this._window);
> +    let newContact = new this._window.mozContact(contact.properties);
> +    newContact.setMetadata(contact.id, contact.published, contact.updated);

Again, can you explain why these clones are now needed?  They seem unrelated.

::: dom/filehandle/FileHandle.cpp
@@ +626,3 @@
>                                 ErrorResult& aRv)
>  {
> +  DOMFile& file = const_cast<DOMFile&>(aValue);

If we could make DOMFile::GetSize() const, then perhaps this cast could go away.

::: dom/indexedDB/IDBObjectStore.cpp
@@ +1623,5 @@
>      return true;
>    }
>  
> +  DOMFile* domBlob = nullptr;
> +  if (NS_SUCCEEDED(UNWRAP_OBJECT(Blob, aObj, domBlob))) {

#include "mozilla/dom/BlobBinding.h" for UNWRAP_OBJECT(Blob).

::: dom/ipc/StructuredCloneUtils.cpp
@@ +9,5 @@
>  #include "nsIDOMDOMException.h"
>  #include "nsIMutable.h"
>  #include "nsIXPConnect.h"
>  
> +#include "nsDOMFile.h"

#include "mozilla/dom/BlobBinding.h" for UNWRAP_OBJECT(Blob).

::: dom/webidl/Blob.webidl
@@ +21,3 @@
>    readonly attribute DOMString type;
>  
> +  // TODO readonly attribute boolean isClosed;

Can we reference a bug so we don't lose track of this TODO?

@@ +31,2 @@
>  
> +  // TODO void close();

Reference follow-up bug.

::: dom/workers/URL.cpp
@@ +849,5 @@
> +
> +  NS_NAMED_LITERAL_STRING(argStr, "Argument 1 of URL.createObjectURL");
> +  NS_NAMED_LITERAL_STRING(blobStr, "Blob");
> +  aRv.ThrowTypeError(MSG_DOES_NOT_IMPLEMENT_INTERFACE, &argStr, &blobStr);
> +}

Do we have to maintain this JSObject* implementation?  Won't the webidl bindings throw an error for us before it calls into the implementation class?

::: dom/workers/WorkerPrivate.cpp
@@ +364,3 @@
>      {
> +      DOMFile* blob = nullptr;
> +      if (NS_SUCCEEDED(UNWRAP_OBJECT(Blob, aObj, blob))) {

#include "mozilla/dom/BlobBinding.h" for UNWRAP_OBJECT(Blob).

::: dom/workers/XMLHttpRequest.cpp
@@ +2150,5 @@
>  void
> +XMLHttpRequest::Send(DOMFile& aBody, ErrorResult& aRv)
> +{
> +  JSContext* cx = mWorkerPrivate->GetJSContext();
> +  mWorkerPrivate->AssertIsOnWorkerThread();

Move the assertion to the top of the method please.

::: js/xpconnect/src/ExportHelpers.cpp
@@ +13,5 @@
>  #include "js/StructuredClone.h"
>  #include "mozilla/dom/BindingUtils.h"
>  #include "nsGlobalWindow.h"
>  #include "nsJSUtils.h"
> +#include "nsDOMFile.h"

#include "mozilla/dom/BlobBinding.h" for UNWRAP_OBJECT(Blob).
Attachment #8466273 - Attachment description: patch 1 - v1 - DOMFile/DOMBlob to WebIDL → patch 1 - v1
Attachment #8466273 - Flags: review?(bkelly) → review+
Assignee

Updated

5 years ago
Blocks: 1048321
Assignee

Updated

5 years ago
Blocks: 1048325
Assignee

Comment 17

5 years ago
> Can you reference bug numbers for these?

These are the other patches.

> Can this be made const?  The webidl binding allows it and might prevent some
> const_casts further up the call stack.  Please double-check the const-ness
> of the other accessors as well.

It cannot be const because DOMFileImplFile uses nsIFile->GetFileSize() that is not const.

> ::: content/base/src/nsDOMBlobBuilder.cpp
> @@ +157,5 @@
> > +
> > +    else if (data.IsArrayBuffer()) {
> > +      const ArrayBuffer& buffer = data.GetAsArrayBuffer();
> > +      buffer.ComputeLengthAndData();
> > +      aRv = blobSet.AppendVoidPtr(buffer.Data(), buffer.Length());
> 
> Can you expand the tests to verify that this works as expected with zero
> length ArrayBuffers.


> ::: content/base/src/nsDOMBlobBuilder.h
> @@ +76,2 @@
> >  
> > +  void InitializeChromeFile(DOMFile& aData,
> 
> Can this be |const DOMFile& aData|?

DOMFileImplFile::GetType cannot be const :/

> On a related note, can you expand test_fileapi_slice.html to test the
> content type argument?

with the changes suggested by bz, contentType cannot be null.
And we already have this check:
testSlice(fileFile, size, "", fileData, "fileFile");

is this what you meant?

> @@ +518,4 @@
> >  
> >    if (blob->IsFile()) {
> >      nsString filename;
> > +    blob->GetName(filename);

blob is DOMFileImpl and GetName returns void.

> ::: dom/browser-element/BrowserElementParent.jsm
> @@ +542,5 @@
> >      }
> >      else {
> >        debug("Got error in gotDOMRequestResult.");
> > +      Services.DOMRequest.fireErrorAsync(req,
> > +        Cu.cloneInto(data.json.errorMsg, this._window));
> 
> Can you explain why these clones are now necessary?

DOMFile has a chrome wrapper and cloning it we create a new object for content.
This probably will disappear when DOMFile::GetParentObject() returns something and not null.

> ::: dom/workers/URL.cpp
> @@ +849,5 @@
> > +
> > +  NS_NAMED_LITERAL_STRING(argStr, "Argument 1 of URL.createObjectURL");
> > +  NS_NAMED_LITERAL_STRING(blobStr, "Blob");
> > +  aRv.ThrowTypeError(MSG_DOES_NOT_IMPLEMENT_INTERFACE, &argStr, &blobStr);
> > +}
> 
> Do we have to maintain this JSObject* implementation?  Won't the webidl
> bindings throw an error for us before it calls into the implementation class?

Actually it's not for Blob but for MediaStream. Code updated.

I'm going to proceed to add something in GetParentObject(). Then interdiff.
Thanks.
Assignee

Comment 18

5 years ago
Posted patch patch 1b (obsolete) — Splinter Review
Bent, can you take a look of this? The issue is that DOMFile must have a parent before returning it to content. In order to do that I had to change many components. I'm asking you a review because many of those are related to IPC.

In particular BlobChild/BlobParent::GetBlob() now takes a parent nsISupport object. This is used for creating a new DOMFile if needed. Sometimes I set it to null when I'm 100% sure that that blob is not returned to content.

If you have better ideas, tell me. Thanks
Attachment #8467755 - Flags: review?(bent.mozilla)
Assignee

Comment 19

5 years ago
Comment on attachment 8467755 [details] [diff] [review]
patch 1b

I asked bz to give me a review for the DOM part.
Attachment #8467755 - Flags: review?(bent.mozilla) → review?(bzbarsky)
Comment on attachment 8467755 [details] [diff] [review]
patch 1b

>+++ b/content/base/public/nsDOMFile.h

I'm sorry this took so long.  :(

>+  // threads. This method has to be used in order to set the set the correct

s/set the set/set/

>+++ b/content/html/content/src/HTMLCanvasElement.cpp
>+                                       OwnerDoc()->GetWindow(),

That will pass null in cases when OwnerDoc() is not in a window... but wouldn't "global" make more sense here?  That should be non-null even when GetWindow() is null.

Furthermore, GetWindow() returns the outer, but the owner should be an inner.  We should add an assert somewhere in the blob/file code that the owner it gets passed, if it's a window, is an inner window.

>+  return DOMFile::CreateMemoryFile(OwnerDoc()->GetWindow(), imgData,

Similar issue here.

>+++ b/content/html/content/src/HTMLInputElement.cpp
>+      mFileList[i]->SetParentObject(mInput->OwnerDoc()->GetWindow());

And here.

>+        DOMFile::CreateFromFile(OwnerDoc()->GetWindow(), file);

And here.

>+++ b/dom/devicestorage/DeviceStorageRequestParent.cpp

Please document where, if anywhere, the parents of these blobs and file are set?

>+++ b/dom/filesystem/*

I'm assuming that you've checked that these GetWindow return values are inner windows, not outer ones?

>+++ b/dom/ipc/Blob.cpp
>+      nsCOMPtr<nsIDOMBlob> blob = newActor->GetBlob(nullptr);

Document why nullptr is ok?  Same for other places in this file.

>+  // If aParent is null, this blob will not exposed to content.

"will not be"

>+      nsCOMPtr<nsIDOMBlob> blob =newActor->GetBlob(nullptr);

Add the space after '=' while you're here, please.

>+++ b/dom/ipc/Blob.h
>+  // Note: If this blob is exposed to content, aParent must be a proper value.

This needs some more explanation of what "proper" is, in both places.

>+++ b/dom/ipc/StructuredCloneUtils.cpp
>+    // Let's create a new blob in the with the correct parent.

s/in the //

The rest of this looks good.  r=me with the above addressed.
Attachment #8467755 - Flags: review?(bzbarsky) → review+
Assignee

Comment 21

5 years ago
Posted patch patch 1 - WebIDL (obsolete) — Splinter Review
bholley, this patch is big, and it implements the porting of DOMFile/DOMBlob to WebIDL. bz and bkelly did the first reviews but I would like to have a review from you too.

In particular I would like to have a review on how I use JSCompartments, global objects, parents and and all those things. Of course, any other comment is welcome! Thanks.
Attachment #8466273 - Attachment is obsolete: true
Attachment #8467755 - Attachment is obsolete: true
Attachment #8488767 - Flags: review?(bobbyholley)
I really think you should remove all the WrapObject() calls in this patch in favor of WrapNewBindingObject, which will ensure the result is in your compartment....
:baku, will these patches change the behavior for the user (a quick look seems to confirm this, I see more attribute to the FilePropertyBag, but I'm unsure if this lead to more user-visible change).

(Meanwhile I'm adding dev-doc-needed)
Keywords: dev-doc-needed
Assignee

Comment 24

5 years ago
Posted patch patch 1b - WrapObject (obsolete) — Splinter Review
Attachment #8488979 - Flags: review?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #22)
> I really think you should remove all the WrapObject() calls in this patch in
> favor of WrapNewBindingObject, which will ensure the result is in your
> compartment....

Sounds like we need some documentation somewhere...
http://mxr.mozilla.org/mozilla-central/source/dom/bindings/BindingUtils.h#840 doesn't exactly
hint that it deals with compartments, nor does the method name itself.
Assignee

Comment 26

5 years ago
(In reply to Jean-Yves Perrier [:teoli] from comment #23)
> :baku, will these patches change the behavior for the user (a quick look
> seems to confirm this, I see more attribute to the FilePropertyBag, but I'm
> unsure if this lead to more user-visible change).

You are right. The constructor of the File object is changed and I guess we need to update our documentation pages. No other changes are made.
> Sounds like we need some documentation somewhere...

OK, https://hg.mozilla.org/integration/mozilla-inbound/rev/9de64636f186
Blocks: 842498
Comment on attachment 8488979 [details] [diff] [review]
patch 1b - WrapObject

>+      if (WrapNewBindingObject(cx, blob, &val)) {
>+        return val.toObjectOrNull();

So if WrapNewBindingObject returns false... where will our control flow end up?

Better would be:

      if (!WrapNewBindingObject(cx, blob, &val)) {
        return nullptr;
      }
      return &val.toObject();

You have this pattern in a few places.

>+++ b/dom/workers/WorkerPrivate.cpp
>-        return result;

That seems wrong.  Why was this change made?

r- because of that result thing...
Attachment #8488979 - Flags: review?(bzbarsky) → review-
Comment on attachment 8488767 [details] [diff] [review]
patch 1 - WebIDL

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

I just skimmed large parts of this patch looking for things that I'd be more likely to have input on. This would have been much, much easier to review as a long series of incremental patches (and I probably would have had to sift through a lot less code that I had no opinion on). Please try to avoid megapatches in the future.

Handwavy r=bholley with comments addressed.

::: content/base/public/nsDOMFile.h
@@ +155,5 @@
> +  }
> +
> +  // Some DOMFile objects are created without a parent maybe on different
> +  // threads. This method has to be used in order to set the correct
> +  // parent before sending these objects to content.

Given this, do we want to assert mParent in GetParentObject?

Looking over the patch, there are lots of places where we set this to null under the assumption that it won't ever be reflected into JS. Given that, I think we need some sort of strong assertion that this holds.

::: content/base/src/nsContentUtils.cpp
@@ +5998,5 @@
> +
> +  JS::Rooted<JSObject*> obj(aCx, blob->WrapObject(aCx));
> +  if (!obj || !JS_WrapObject(aCx, &obj)) {
> +    return NS_ERROR_FAILURE;
> +  }

As Boris notes, this (and things like it) should convert to WrapNewBindingObject.

::: content/html/content/src/HTMLInputElement.cpp
@@ +552,5 @@
>      if (mCanceled) { // The last progress event may have canceled us
>        return NS_OK;
>      }
>  
> +    // Setting the parent to the DOMFile

"to"?

::: dom/base/nsGlobalWindow.cpp
@@ +7913,5 @@
>  
> +  // See if this is a File/Blob object.
> +  {
> +    DOMFile* blob = nullptr;
> +    if (NS_SUCCEEDED(UNWRAP_OBJECT(Blob, obj, blob)) && scInfo->subsumes) {

It makes more sense to me to invert the order here and check subsumes first.

::: dom/indexedDB/ipc/IndexedDBParent.cpp
@@ +1500,5 @@
>  
>      for (uint32_t index = 0; index < length; index++) {
>        BlobParent* actor = static_cast<BlobParent*>(aActors[index]);
> +
> +      // We can use null because this blob is not exposed to content.

I think "exposed to content" in all of these comments isn't quite right. "content" generally means "web content", generally defined in opposition to "chrome" (System JS).

So I think it would make more sense to say "...because this blob is not reflected into JS". Comments like this appear in quite a few places in this patch, and I think they should all be updated.

::: dom/workers/XMLHttpRequest.cpp
@@ +1072,5 @@
>      AutoSafeJSContext cx;
>      JSAutoRequest ar(cx);
> +
> +    JS::Rooted<JSObject*> scope(cx, xpc::UnprivilegedJunkScope());
> +    JSAutoCompartment ac(cx, scope);

Hm, why do we need the junk scope, exactly? Just to find an initial scope to enter? If so, I think that's not a good reason. We should either explicitly enter the scope of the XHR's owner, or add some machinery to BindingUtils to handle to case where the caller is in a null compartment.

I really want to avoid a proliferation of junk scope usage, which is why I'm going to be stubborn about uses like this one. ;-)

@@ +2160,5 @@
>    SendInternal(EmptyString(), Move(buffer), clonedObjects, aRv);
>  }
>  
>  void
> +XMLHttpRequest::Send(DOMFile& aBody, ErrorResult& aRv)

This looks an awful lot like the JSObject overload of Send(). Why can't we just invoke that overload with aBody.WrapObject(cx)? We should find some way to share this code.

::: js/xpconnect/tests/chrome/test_cloneInto.xul
@@ -147,5 @@
>      true,
>      'hello world',
>      [1, 2, 3],
>      { a: 1, b: 2 },
> -    { blob: new Blob([]), file: new File(new Blob([])) },

Er, why do the blob tests go away here? Aren't they explicitly supported with the changes in ExportHelpers.cpp?
Attachment #8488767 - Flags: review?(bobbyholley) → review+
> We should either explicitly enter the scope of the XHR's owner

The problem is that it's a pain to get there.  The XHR's owner is just nsISupports, iirc,, so we have to wrap it, which means we need to be in a scope, etc.

> or add some machinery to BindingUtils to handle to case where the caller is in a null
> compartment.

What would that machinery do?  Enter the junk scope?

The only sane and principled thing here is to quickly get to an nsIGlobalObject and get its JS object or something, but that's a major hassle in the general case.  In this case it _could_ be done, but I for one would be OK with that happening in a followup.
(In reply to Boris Zbarsky [:bz] from comment #30)
> What would that machinery do?  Enter the junk scope?

What code do we have that requires us to be in a compartment before we're dealing with a JSObject at all?
WrapNewBindingObject promises to wrap into the caller's compartment.  That means it assumes there is a caller's compartment, no?
(In reply to Boris Zbarsky [:bz] from comment #32)
> WrapNewBindingObject promises to wrap into the caller's compartment.  That
> means it assumes there is a caller's compartment, no?

It seems like that could easily be changed, right? Or we could move most of the existing code there into WrapNewBindingObjectDontWrapValue, and WrapNewBindingObject could delegate to that and add a tail call to JS_WrapValue?

In general, if we don't have any JSObjects yet, it seems wrong to require us to be in "some compartment". We've had requirements like that in the past that required us to bootstrap a dummy global every time we wanted to create a Window global, and I had to work very hard to fix it.
> It seems like that could easily be changed, right?

To do what?

We need a function, to be used from codegen, that will do exactly what WrapNewBindingObject does right now.

Furthermore, this function is very hot, and needs to be fast in the common (same-compartment) case.

I'm open to refactoring this somehow while preserving those properties, and agree it would be a good idea.  I don't think we should block this bug on it, but rather should do a followup.
(In reply to Boris Zbarsky [:bz] from comment #34)
> > It seems like that could easily be changed, right?
> 
> To do what?
> 
> We need a function, to be used from codegen, that will do exactly what
> WrapNewBindingObject does right now.
> 
> Furthermore, this function is very hot, and needs to be fast in the common
> (same-compartment) case.

Right, so we do what I suggest in comment 33. Is there anything about that which doesn't satisfy the above?

> I'm open to refactoring this somehow while preserving those properties, and
> agree it would be a good idea.  I don't think we should block this bug on
> it, but rather should do a followup.

It depends - what kind of SLA would the followup have? The current patch uses the JunkScope, which is governed by special rules to avoid it becoming another hiddenDOMWindow.
> Is there anything about that which doesn't satisfy the above?

I'd need to write the code to make sure.

> It depends - what kind of SLA would the followup have?

I'll make sure I fix it before 35 branches.  I just can't do it this week, and I'd much rather get this landed to unblock other things than have it blocked on me fixing up this part.
(In reply to Boris Zbarsky [:bz] from comment #36)
> I'll make sure I fix it before 35 branches.  I just can't do it this week,
> and I'd much rather get this landed to unblock other things than have it
> blocked on me fixing up this part.

Sounds good to me. Thanks a lot for doing that, and let me know if you need help.
Assignee

Updated

5 years ago
Blocks: 972973
Assignee

Comment 38

5 years ago
Posted patch wrapObject.patch (obsolete) — Splinter Review
I'm going to merge this patch in patch 1 if you give me a +
Attachment #8488979 - Attachment is obsolete: true
Attachment #8494607 - Flags: review?(bzbarsky)
Comment on attachment 8494607 [details] [diff] [review]
wrapObject.patch

>+++ b/dom/workers/WorkerPrivate.cpp

The change here is still wrong.  The "return result;" needs to be after the closing curly of that block.  There's even a comment that explains why!

Please fix that.

r=me with that fixed, but please do fix it.
Attachment #8494607 - Flags: review?(bzbarsky) → review+
Assignee

Comment 40

5 years ago
> Sounds good to me. Thanks a lot for doing that, and let me know if you need
> help.

In the meantime, can I keep the code as it is and file a followup?
Assignee

Comment 41

5 years ago
Posted patch patch 1 - WebIDL (obsolete) — Splinter Review
1. rebased
2. fixed the return issue
3. applied (almost) all the bholley comments (read the comments I wrote to know why)

Smaug, there are not so many changes about SystemMessage API, but I changed a couple of things and I appreciate if you can spend a couple of minutes (?) reading this patch.

I guess a couple of tests are broken after the merging.
I'm going to fix them and in case there are big changes, I'll ask a review in a interdiff.
Attachment #8488767 - Attachment is obsolete: true
Attachment #8494607 - Attachment is obsolete: true
Attachment #8497486 - Flags: review?(bugs)
Assignee

Updated

5 years ago
Attachment #8497486 - Attachment description: webidl2.patch → patch 1 - WebIDL
Assignee

Comment 42

5 years ago
> >  void
> > +XMLHttpRequest::Send(DOMFile& aBody, ErrorResult& aRv)
> 
> This looks an awful lot like the JSObject overload of Send(). Why can't we
> just invoke that overload with aBody.WrapObject(cx)? We should find some way
> to share this code.

In that case  I have to wrap it here and then unwrap it on the other Send() just to reuse < 10 lines.
I would like to avoid this.

> > -    { blob: new Blob([]), file: new File(new Blob([])) },
> 
> Er, why do the blob tests go away here? Aren't they explicitly supported
> with the changes in ExportHelpers.cpp?

Because comparing cloned objects fails. The previous blob is actually a different object and we don't have any way to compare the real data from JS yet. Correct?
It is not clear to me what I should review here.
I know nothing about SystemMessage API, but I also don't see SystemMessage API changes in the patch.
Assignee

Comment 44

5 years ago
smaug, yes, sorry, I meant FrameMessageManager. No big changes, bug an additional glance can help.

Comment 45

5 years ago
> Because comparing cloned objects fails. The previous blob is actually a
> different object and we don't have any way to compare the real data from JS
> yet. Correct?

As far as I know, we can compare blobs by read them into array buffers via FileReader. For Example:

  function readAsArrayBuffer(blob) {
    return new Promise(function (resolve) {
      var reader = new FileReader();
      reader.onload = function () {
        resolve(reader.result);
      }
      reader.readAsArrayBuffer(blob);
    });
  }

  function compareArrayBuffer(bufferA, bufferB) {
    if (bufferA.byteLength != bufferB.byteLength) return false;
    var va = new Uint8Array(bufferA);
    var vb = new Uint8Array(bufferB);
    for (var i = 0; i < va.length; i++) {
      if (va[i] != vb[i]) return false;
    }
    return true;
  }

  var a = new Blob(['ABCDE'], {contentType:'text/plain;carset=utf-8'});
  var b = new Blob(['ABCDE'], {contentType:'text/plain;carset=utf-8'});
  var c = new Blob(['01234'], {contentType:'text/plain;carset=utf-8'});

  readAsArrayBuffer(a).then(function (arrayA) {
    readAsArrayBuffer(a).then(function (arrayB) {
      console.log(a === b, compareArrayBuffer(arrayA, arrayB)); // logs `false true`
    });
  });

  readAsArrayBuffer(a).then(function (arrayA) {
    readAsArrayBuffer(c).then(function (arrayC) {
      console.log(a === c, compareArrayBuffer(arrayA, arrayC)); // logs `false false`
    });
  });
> In the meantime, can I keep the code as it is and file a followup?

Yes, but please get a move on with it.  I committed to fixing that followup before Oct 13, but that was two weeks ago!  At this point timing on that is going to be really tight for me.  :(
(In reply to Andrea Marchesini (:baku) from comment #40)
> > Sounds good to me. Thanks a lot for doing that, and let me know if you need
> > help.
> 
> In the meantime, can I keep the code as it is and file a followup?

Yes.
(In reply to Andrea Marchesini (:baku) from comment #42)
> In that case  I have to wrap it here and then unwrap it on the other Send()
> just to reuse < 10 lines.
> I would like to avoid this.

I don't follow - XMLHttpRequest::Send(JS::Handle<JSObject*> aBody, ErrorResult& aRv) doesn't do any unwrapping that I see.
(In reply to Boris Zbarsky [:bz] from comment #46)
> > In the meantime, can I keep the code as it is and file a followup?
> 
> Yes, but please get a move on with it.  I committed to fixing that followup
> before Oct 13, but that was two weeks ago!  At this point timing on that is
> going to be really tight for me.  :(

As long as it gets done in a near-ish timeframe I don't really care about exact dates.
Assignee

Comment 50

5 years ago
Posted patch patch 1 - interdiff (obsolete) — Splinter Review
Interdiff:

1. MOZ_ASSERT(mParent) cannot be there because for chrome code, and workers, sometimes we don't have mParent.

2. Instead SetParentObject() I think it's cleaner to create a new DOMFile.
Attachment #8498286 - Flags: review?(bzbarsky)
Assignee

Comment 51

5 years ago
Posted patch patch 1 - WebIDL (obsolete) — Splinter Review
Attachment #8497486 - Attachment is obsolete: true
Attachment #8497486 - Flags: review?(bugs)
Comment on attachment 8498286 [details] [diff] [review]
patch 1 - interdiff

r=me
Attachment #8498286 - Flags: review?(bzbarsky) → review+
Assignee

Comment 53

5 years ago
Comment on attachment 8467051 [details] [diff] [review]
patch 2 - nsDOMFileList to FileList

A simple renaming for nsDOMFileList.
Attachment #8467051 - Flags: review?(ehsan.akhgari)
Assignee

Comment 54

5 years ago
Comment on attachment 8467052 [details] [diff] [review]
patch 3 - nsDOMFile.h to mozilla/dom/File.h

renaming nsDOMFile to File.
Attachment #8467052 - Flags: review?(ehsan.akhgari)
Assignee

Comment 55

5 years ago
Comment on attachment 8467053 [details] [diff] [review]
patch 4 - Remove nsDOMBlobBuilder.h

Moving code in order to remove nsDOMBlobBuilder.*
Attachment #8467053 - Flags: review?(ehsan.akhgari)

Updated

5 years ago
Attachment #8467051 - Flags: review?(ehsan.akhgari) → review+

Comment 56

5 years ago
Comment on attachment 8467052 [details] [diff] [review]
patch 3 - nsDOMFile.h to mozilla/dom/File.h

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

::: dom/bindings/Bindings.conf
@@ +150,5 @@
>  },
>  
>  'Blob': {
> +    'nativeType': 'mozilla::dom::File',
> +    'headerFile': 'mozilla/dom/File.h',

I think you should be able to drop headerFile...
Attachment #8467052 - Flags: review?(ehsan.akhgari) → review+

Updated

5 years ago
Attachment #8467053 - Flags: review?(ehsan.akhgari) → review+
Assignee

Comment 57

5 years ago
Posted patch patch 1 - interdiff (obsolete) — Splinter Review
bz, sorry for this new interdiff, but making all the tests pass took a while.
Something about this interdiff:

1. hazard rooting fixed

2. lastModified and lastModifiedData return the same value.
I spoke with smaug about the reason this issue appears just now. Without my patch lastModified returns JS_Now() all the time if the lastModified date is unknown.
Wtih my patches we return the creation time if lastModified is unknown.

This is actually better because now lastModifiedDate and lastModified return the same value.

3. JS_Now() vs PR_Now() on windows they can return different values. So I had to use JS_Now() in the constructor. There is a comment about why.

After that try seems to be fully green and we can land these patches.
Attachment #8498286 - Attachment is obsolete: true
Attachment #8500573 - Flags: review?(bzbarsky)
Comment on attachment 8500573 [details] [diff] [review]
patch 1 - interdiff

>+    //   x.getTime() < f.dateModified.getTime()

This might be false with JS_Now() too, because JS_Now() times can go backwards (e.g. due to NTP adjustments and the like).

If you actually need a monotonic clock, you should be using TimeStamp in C++ and peformance.now() in JS.  If you're not doing that, do NOT assume a monotonic clock (e.g. in tests).

>+// In our implementation of File object, lastModifiedDate is unknown only for in new objects.

s/for in/for/ ?

>+++ b/content/base/test/test_bug403852.html

Yeah, this test is bogus because it assumes precisely that thing I said not to assume.  ;)

>+      // called because otherwise the static analysis thinks it can gc the
>+      // JSObject via the stack.

No, because the static analysis thinks dereferencing XPCOM objects can GC (because in some cases it can!), and a return statement with a JSObject* type means that JSObject* is on the stack as a raw pointer while destructors are running.

Same thing in all the other copy-pasted places.

r=me but please file a followup about that test or something?
Attachment #8500573 - Flags: review?(bzbarsky) → review+
Assignee

Comment 59

5 years ago
Attachment #8498290 - Attachment is obsolete: true
Attachment #8500573 - Attachment is obsolete: true
Assignee

Comment 60

5 years ago
Attachment #8500931 - Flags: review?(bugs)
Comment on attachment 8500931 [details] [diff] [review]
patch 5 - FilePropertyBag doesn't have 'name'

Why doesn't ChromeFilePropertyBag just extend FilePropertyBag?
Attachment #8500931 - Flags: review?(bugs) → review-
Assignee

Updated

5 years ago
Blocks: 1079187
Assignee

Comment 62

5 years ago
Attachment #8500931 - Attachment is obsolete: true
Attachment #8500958 - Flags: review?(bugs)
Attachment #8500958 - Flags: review?(bugs) → review+
Assignee

Updated

5 years ago
Blocks: 1079273
Assignee

Updated

5 years ago
Blocks: 1079301
Comment on attachment 8501720 [details] [diff] [review]
patch 6 - webplatform tests fix

>+  [no-argument Blob constructor without 'new']

This should throw (though Web IDL hasn't been updated to say that yet, while we figure out ES6 subclassing stuff).  Can you please file a bug on this test?

>+  [Changes to the blobParts array should be reflected in the returned Blob (pop).]
>+  [Changes to the blobParts array should be reflected in the returned Blob (unshift).]

This is bogus per spec.  The argument is a sequence, which is passed by value, not by reference.  Again, file a bug on this test.

r=me
Attachment #8501720 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #68)

> >+  [Changes to the blobParts array should be reflected in the returned Blob (pop).]
> >+  [Changes to the blobParts array should be reflected in the returned Blob (unshift).]
> 
> This is bogus per spec.  The argument is a sequence, which is passed by
> value, not by reference.  Again, file a bug on this test.

It's not clear to me why this is bogus, but I might be missing something.

It looks like the test causes mutation of the array to happen as it's being converted to a WebIDL sequence. As far as I can tell from the spec, this conversion happens using an iterator over the original array, which appears to just store an index into the array and so the output sequence will reflect changes to the array made during iteration.
My apologies.  I should have looked at the test more carefully; I had assumed (bad me) that it was calling the constructor and then later mutating the array.

The pop() test is still incorrect: the problem is that the test is depending on the old duck-typing behavior of sequences, not the iterator behavior.

For example, stepping through the pop() test:

  1)  We create the array iterator, which has [[ArrayIteratorNextIndex]] set to 0 to start.
  2)  We call next() on it, this sets [[ArrayIteratorNextIndex]] to 1.
  3)  We take the returned object and ToString() it.  This calls pop() on the array.
  4)  We call next() on the iterator again, landing in 
    http://people.mozilla.org/~jorendorff/es6-draft.html#sec-%arrayiteratorprototype%.next
  5)  In step 6, index is 1.
  6)  In step 8 we do .length on the array, which returns 1 (because we popped the value).
  7)  In step 11 we detect index >= len and return an iterator result that indicates
      iteration is done.

The output of this is a Web IDL sequence of length 1, containing the single string "PASS".  But the test expects a sequence of length 2, containing the strings "PASS" and "undefined".

The unshift() test is incorrect for the same reason: it assumes that length is grabbed up front, while ArrayIterator doesn't do that.
Thanks to RyanVM for pointing me at some non-unified build bustage.  I pushed what is hopefully a fix:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/c6bd8c963e94
Adding an include didn't work.  Maybe I need to explicitly prefix ErrorResult with mozilla::
  https://hg.mozilla.org/integration/mozilla-inbound/rev/bb693abe8de0
Or throw in "using namespace mozilla".
Depends on: 1081415
Depends on: 1081453
Hi, are there any effects on site compatibility?
Flags: needinfo?(amarchesini)
Assignee

Comment 78

5 years ago
(In reply to Kohei Yoshino [:kohei] from comment #77)
> Hi, are there any effects on site compatibility?

No. We should not have any effects on site compatibility.
Flags: needinfo?(amarchesini)
Assignee

Updated

4 years ago
Duplicate of this bug: 1026303

Updated

4 years ago
Depends on: 1153900

Updated

4 years ago
Depends on: 1154472
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.