Bug 171781 - expose File to web workers
Summary: expose File to web workers
Status: RESOLVED CONFIGURATION CHANGED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 156511 171960
Blocks: 171589
  Show dependency treegraph
 
Reported: 2017-05-06 19:27 PDT by Ben Kelly
Modified: 2020-09-30 20:12 PDT (History)
5 users (show)

See Also:


Attachments
Patch (8.25 KB, patch)
2017-05-10 20:28 PDT, Ben Kelly
no flags Details | Formatted Diff | Diff
Patch (8.38 KB, patch)
2017-05-13 18:33 PDT, Ben Kelly
no flags Details | Formatted Diff | Diff
Patch (10.53 KB, patch)
2017-05-13 19:12 PDT, Ben Kelly
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ben Kelly 2017-05-06 19:27:35 PDT
Bug 156511 implemented the latest File interface, but it appears it did not expose File on web workers.  I'd like to try to fix this so that I can reference it in FormData on workers in bug 171589.
Comment 1 Alexey Proskuryakov 2017-05-07 00:09:31 PDT
I would expect the primary challenge to be making this code thread safe. Not much of WebCore is ready to be run on secondary threads.
Comment 2 Ben Kelly 2017-05-07 17:58:59 PDT
Well, in this case File extends Blob which is already exposed on Workers.  Adding [Exposed=(Worker)] to the idl lets webkit pass this test:

http://w3c-test.org/FileAPI/file/Worker-read-file-constructor.worker.html

That seems too easy, though.
Comment 3 Ben Kelly 2017-05-10 20:28:58 PDT
Created attachment 309686 [details]
Patch
Comment 4 Ben Kelly 2017-05-10 20:29:29 PDT
This patch depends on bug 171960, so I don't think I can try EWS until that lands.
Comment 5 Ben Kelly 2017-05-13 18:33:15 PDT
Created attachment 310045 [details]
Patch
Comment 6 Ben Kelly 2017-05-13 19:12:46 PDT
Created attachment 310049 [details]
Patch
Comment 7 Alexey Proskuryakov 2017-05-14 12:39:13 PDT
What investigations did you perform to ensure that the newly exposed code is safe to run in workers?

Personally, I don't know how much has been done for Blobs either, so those might not be very safe.
Comment 8 Ben Kelly 2017-05-14 20:50:54 PDT
(In reply to Alexey Proskuryakov from comment #7)
> What investigations did you perform to ensure that the newly exposed code is
> safe to run in workers?

Currently, File extends Blob and adds three extra parameters:

    String m_path;
    String m_name;
    std::optional<int64_t> m_overrideLastModifiedDate;

AFAICT these are all immutable after construction.  I don't see anything obviously dangerous with this, assuming the Blob base class is adequately supported on workers.

> Personally, I don't know how much has been done for Blobs either, so those
> might not be very safe.

I looked at Blob.idl blame and it appears that its been exposed to workers for at least 4 years.  I didn't try tracing it back further.  Features like IDB depend on Blob as well.

There are a number of worker blob tests cases in:

LayoutTests/fast/files/workers

The tests I'm trying to import in bug 171960 provide a worker test that exercises the File() constructor and its additional getters.

I've wondered if there is something I need to do to support structure clone on workers properly.  I haven't gotten as far as digging into that yet.  Any pointers on how structured clone is implemented are appreciated.

Anything else you recommend investigating?  Thanks.
Comment 9 Alexey Proskuryakov 2017-05-15 09:13:22 PDT
String objects can't be shared across threads, so any time they are passed over, there needs to be a copy made. More generally, it's not just about the object data, but also about the implementation. For example, are there are IPC messages sent?

> // FIXME: This should be exposed on Workers as well.

The person who added this FIXME must have expected the effort to be larger than a one line change. Who added it, what does svn blame say?
Comment 10 Chris Dumez 2017-05-15 09:21:35 PDT
(In reply to Alexey Proskuryakov from comment #9)
> String objects can't be shared across threads, so any time they are passed
> over, there needs to be a copy made. More generally, it's not just about the
> object data, but also about the implementation. For example, are there are
> IPC messages sent?
> 
> > // FIXME: This should be exposed on Workers as well.
> 
> The person who added this FIXME must have expected the effort to be larger
> than a one line change. Who added it, what does svn blame say?

Sam Weinig.
Comment 11 Chris Dumez 2017-05-15 09:28:02 PDT
(In reply to Alexey Proskuryakov from comment #9)
> String objects can't be shared across threads, so any time they are passed
> over, there needs to be a copy made. More generally, it's not just about the
> object data, but also about the implementation. For example, are there are
> IPC messages sent?
> 
> > // FIXME: This should be exposed on Workers as well.
> 
> The person who added this FIXME must have expected the effort to be larger
> than a one line change. Who added it, what does svn blame say?

As long as the File object is not passed across threads, then we'd be fine. I am not super familiar with our current File API implementation but assuming the File object cannot be passed across thread, then it should not matter if we are in a worker thread or the main thread.

It seems File has a constructor and I expect this constructor would just work in a worker thread.
Comment 12 Alexey Proskuryakov 2017-05-15 09:56:35 PDT
I recall there being quite a bit of IPC, possibly even sync IPC - which is not supported in non-main threads.
Comment 13 Chris Dumez 2017-05-15 10:02:53 PDT
(In reply to Alexey Proskuryakov from comment #12)
> I recall there being quite a bit of IPC, possibly even sync IPC - which is
> not supported in non-main threads.

Yes, I believe there will be IPC in WK2 but it is via ThreadableBlobRegistry::registerFileBlobURL() which properly deals with threads.
Comment 14 Ben Kelly 2017-05-15 10:17:31 PDT
(In reply to Chris Dumez from comment #11)
> It seems File has a constructor and I expect this constructor would just
> work in a worker thread.

The other ways you can produce a File in a worker are at least:

1. postMessage()
2. IDB

Both of these are based on structured clone.  I can write some tests for these, but if anyone has info on how structured clone is implemented for DOM objects in webkit that would be helpful too.
Comment 15 Sam Weinig 2020-09-30 20:12:48 PDT
File has been exposed to workers for a while at this point, I don't think there is anything left to do here.