Closed Bug 1185381 Opened 4 years ago Closed 4 years ago

Make FileList clonable

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(4 files, 1 obsolete file)

Currently we clone FileList only window to window.
In order to unify the postMessages (in workers, MessagePort, etc) FileList must be fully clonable.
To be more precise, looks like we currently just share FileLists between same origin windows. No cloning happening.
This patch just moves the code of FileList in 2 separate files.
Attachment #8635833 - Flags: review?(bugs)
Attachment #8635834 - Flags: review?(bugs)
Comment on attachment 8635834 [details] [diff] [review]
part2: BLOBIMPL_IID

I guess there isn't technically any need to change the IID in this kind of case.
Attachment #8635834 - Flags: review?(bugs) → review+
Attachment #8635833 - Flags: review?(bugs) → review+
Here the cloning part. Basically it creates a thread-safe FileListClonedData object that is sent to the destination. It's thread-safe because I need to use it for worker postMessages.
Attachment #8635835 - Flags: review?(bugs)
Attached patch part 4: tests (obsolete) — Splinter Review
Attachment #8635836 - Flags: review?(bugs)
Comment on attachment 8635835 [details] [diff] [review]
part 3: FileListClonedData

>+FileList::Create(nsISupports *aParent, FileListClonedData* aData)
nsISupports* aParent

>+    // What we get back from the reader is a FileListClonedData.
>+    // From that we create a new FileList.
>+    FileListClonedData* fileListClonedData;
>+    if (JS_ReadBytes(reader, &fileListClonedData, sizeof(fileListClonedData))) {
>+      MOZ_ASSERT(fileListClonedData);
Ah, this is the reason for FileListClonedData.
You could have used also just nsTArray<nsRefPtr<BlobImpl>> I think, but perhaps this is a bit nicer, 
since StoreISupports can be reused this way.

>+  // See if this is a FileList object.
>+  {
>+    FileList* fileList = nullptr;
>+    if (NS_SUCCEEDED(UNWRAP_OBJECT(FileList, obj, fileList))) {
>+      nsRefPtr<FileListClonedData> fileListClonedData =
>+        fileList->CreateClonedData();
>+      MOZ_ASSERT(fileListClonedData);
>+      FileListClonedData* ptr = fileListClonedData.get();
>+      if (JS_WriteUint32Pair(writer, SCTAG_DOM_FILELIST, 0) &&
>+          JS_WriteBytes(writer, &ptr, sizeof(ptr))) {
>+        scInfo->event->StoreISupports(fileListClonedData);
>+        return true;
>+      }
So StoreISupports
Attachment #8635835 - Flags: review?(bugs) → review+
Comment on attachment 8635836 [details] [diff] [review]
part 4: tests

So the subsumes removal patch will add a test where filelist is cloned across domains, right?
Attachment #8635836 - Flags: review?(bugs) → review+
Attached patch part 4: testsSplinter Review
I improved the tests to support cross-origin fileList + blob cloning.
Attachment #8635836 - Attachment is obsolete: true
Attachment #8635899 - Flags: review?(bugs)
Comment on attachment 8635899 [details] [diff] [review]
part 4: tests

I would use just "iframe_cloning_fileList.html" for the same origin.
I never recall which port is the default port while running mochitests.
Attachment #8635899 - Flags: review?(bugs) → review+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.