RESOLVED CONFIGURATION CHANGED 171781
expose File to web workers
https://bugs.webkit.org/show_bug.cgi?id=171781
Summary expose File to web workers
Ben Kelly
Reported 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.
Attachments
Patch (8.25 KB, patch)
2017-05-10 20:28 PDT, Ben Kelly
no flags
Patch (8.38 KB, patch)
2017-05-13 18:33 PDT, Ben Kelly
no flags
Patch (10.53 KB, patch)
2017-05-13 19:12 PDT, Ben Kelly
no flags
Alexey Proskuryakov
Comment 1 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.
Ben Kelly
Comment 2 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.
Ben Kelly
Comment 3 2017-05-10 20:28:58 PDT
Ben Kelly
Comment 4 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.
Ben Kelly
Comment 5 2017-05-13 18:33:15 PDT
Ben Kelly
Comment 6 2017-05-13 19:12:46 PDT
Alexey Proskuryakov
Comment 7 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.
Ben Kelly
Comment 8 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.
Alexey Proskuryakov
Comment 9 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?
Chris Dumez
Comment 10 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.
Chris Dumez
Comment 11 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.
Alexey Proskuryakov
Comment 12 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.
Chris Dumez
Comment 13 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.
Ben Kelly
Comment 14 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.
Sam Weinig
Comment 15 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.
Note You need to log in before you can comment on or make changes to this bug.