1185381 - Make FileList clonable

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

Currently we clone FileList only window to window.
In order to unify the postMessages (in workers, MessagePort, etc) FileList must be fully clonable.

Comment 1

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

To be more precise, looks like we currently just share FileLists between same origin windows. No cloning happening.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 1: move code into FileList.h/.cpp

This patch just moves the code of FileList in 2 separate files.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part2: BLOBIMPL_IID

Comment 4

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

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.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 3: FileListClonedData

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.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 4: tests

Comment 7

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

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

Comment 8

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

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?

Comment 9

[Andrea Marchesini [:baku]]

Attachment: part 4: tests

I improved the tests to support cross-origin fileList + blob cloning.

Comment 10

[Andrea Marchesini [:baku]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=724d0b89ad4f

Comment 11

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

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.

Comment 12

[Andrea Marchesini [:baku]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/da598d1d8f73
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9b04e954b2b2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1af9f6c0ed28
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5c58139ed723

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/da598d1d8f73
https://hg.mozilla.org/mozilla-central/rev/9b04e954b2b2
https://hg.mozilla.org/mozilla-central/rev/1af9f6c0ed28
https://hg.mozilla.org/mozilla-central/rev/5c58139ed723