Implement a prototype version of Houdini "Worklets Level 1" spec

RESOLVED FIXED in Firefox 52

Status

()

RESOLVED FIXED
3 years ago
10 days ago

People

(Reporter: baku, Assigned: baku)

Tracking

(Depends on: 1 bug, Blocks: 3 bugs)

50 Branch
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 affected, firefox52 fixed)

Details

(URL)

Attachments

(6 attachments, 5 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
With this first patch I implemented some Worklet WebIDL things and the importing of modules. The importing doesn't follow the spec yet (no cache, for instance, wrong error code returned, etc).

For testing only, I also exposed window.createWorklet(); This is similar to what the spec calls "fakeWorket1" and "fakeWorklet2".
Attachment #8775485 - Flags: feedback?(dholbert)
(Assignee)

Comment 2

3 years ago
Posted patch WIP 2 - WorkletGlobalScope (obsolete) — Splinter Review
This second patch is about WorkletGlobalScope. The imported module is executed using this new global scope. I still use the wrong compilation flag, and the console is not really exposed yet.
Attachment #8775486 - Flags: feedback?(dholbert)
(Assignee)

Comment 3

3 years ago
Daniel, these 2 feedback requests are just to inform about the progress. Don't spend time to read this code yet.
Thanks! Great to see progress here.
Summary: Implement Worklet spec → Implement Houdini "Worklets Level 1" spec
Attachment #8775486 - Flags: feedback?(dholbert)
Attachment #8775485 - Flags: feedback?(dholbert)
Keywords: dev-doc-needed
(Assignee)

Comment 5

3 years ago
Posted patch part 2 - WorkletGlobalScope (obsolete) — Splinter Review
Attachment #8775486 - Attachment is obsolete: true
(Assignee)

Comment 6

3 years ago
(Assignee)

Comment 7

3 years ago
Comment on attachment 8775485 [details] [diff] [review]
WIP 1 - Worklet webIDL interface

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

This patch implements:
1. window.createWorklet() for testing only
2. Worklet.webidl
3. fetch of the script.
4. tests

Missing steps: having an hashtable of fetched scripts as the spec says. This will be done in a separate patch.
Attachment #8775485 - Flags: feedback?(bugs)
(Assignee)

Comment 8

3 years ago
Comment on attachment 8781941 [details] [diff] [review]
part 2 - WorkletGlobalScope

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

This patch implements the WorkletGlobalScope and it executes the fetched script.
Attachment #8781941 - Flags: feedback?(bugs)
(Assignee)

Comment 9

3 years ago
Comment on attachment 8781942 [details] [diff] [review]
part 3 - Console API in worklet

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

Console API exposed to WorkletGlobalScope.
Attachment #8781942 - Flags: feedback?(bugs)
Comment on attachment 8775485 [details] [diff] [review]
WIP 1 - Worklet webIDL interface

>+  static already_AddRefed<Promise>
>+  Fetch(Worklet* aWorklet, const nsAString& aModuleURL, ErrorResult& aRv)
>+  {
>+    RefPtr<Promise> promise = Promise::Create(aWorklet->GetParentObject(), aRv);
>+    if (NS_WARN_IF(aRv.Failed())) {
>+      return nullptr;
>+    }
>+
>+    RequestOrUSVString request;
>+    request.SetAsUSVString().Rebind(aModuleURL.Data(), aModuleURL.Length());
>+
>+    RequestInit init;
>+
>+    RefPtr<Promise> fetchPromise =
>+      FetchRequest(aWorklet->GetParentObject(), request, init, aRv);
Does FetchRequest deal with url being relative or absolute?


>+  ResolveFetch(JSContext* aCx, JS::Handle<JS::Value> aValue)
>+  {
>+    if (!aValue.isObject()) {
>+      mPromise->MaybeReject(NS_ERROR_FAILURE);
>+      return;
>+    }
>+
>+    RefPtr<Response> response;
>+    nsresult rv = UNWRAP_OBJECT(Response, &aValue.toObject(), response);
>+    if (NS_WARN_IF(NS_FAILED(rv))) {
>+      mPromise->MaybeReject(rv);
>+      return;
>+    }
>+
>+    if (!response->Ok()) {
>+      mPromise->MaybeReject(NS_ERROR_FAILURE); // TODO, error TBD
>+      return;
>+    }
>+
>+    ErrorResult error;
>+    RefPtr<Promise> promise = response->Text(error);
>+    if (NS_WARN_IF(error.Failed())) {
>+      mPromise->MaybeReject(error);
>+      return;
>+    }
>+
>+    promise->AppendNativeHandler(this);
>+    mState = eRead;
>+  }
>+
>+  void
>+  ResolveRead(JSContext* aCx, JS::Handle<JS::Value> aValue)
>+  {
>+    if (!aValue.isString()) {
>+      mPromise->MaybeReject(NS_ERROR_FAILURE);
>+      return;
>+    }
>+
>+    nsAutoJSString str;
>+    if (!str.init(aCx, aValue)) {
>+      mPromise->MaybeReject(NS_ERROR_FAILURE);
>+      return;
>+    }
>+
>+    // TODO: do something with this string...
Hmm, we really have no way to avoid JS stuff with Fetch.
If so, I wouldn't use Fetch stuff for loading anything in C++. This is too heavy.
Could we add some stuff to Fetch so that it can deal with C++ better?


>+worklet.import("common.js").then(function() {
>+  ok("Import should load a resource.");
>+  SimpleTest.finish();
>+});
should test also some error case
Attachment #8775485 - Flags: feedback?(bugs) → feedback+
(In reply to Olli Pettay [:smaug] from comment #10)
> >+  {
> >+    if (!aValue.isString()) {
> >+      mPromise->MaybeReject(NS_ERROR_FAILURE);
> >+      return;
> >+    }
> >+
> >+    nsAutoJSString str;
> >+    if (!str.init(aCx, aValue)) {
> >+      mPromise->MaybeReject(NS_ERROR_FAILURE);
> >+      return;
> >+    }
> >+
> >+    // TODO: do something with this string...
> Hmm, we really have no way to avoid JS stuff with Fetch.
> If so, I wouldn't use Fetch stuff for loading anything in C++. This is too
> heavy.
> Could we add some stuff to Fetch so that it can deal with C++ better?
> 
Hmm, or perhaps we don't end up creating a copy of the string after all. Could you ensure that?
Though, still very ugly to go through JSAPI stuff here.
It does make a copy
bz> smaug: Passing the C++ thing to JS will use an external string, but passing from JS to C++ will copy nowadays
bz> smaug: we used to not copy in some cases, but SpiderMonkey made that really hard....
Comment on attachment 8781941 [details] [diff] [review]
part 2 - WorkletGlobalScope

Fine. but need to still figure out how to deal with cycle collection. Since this is now for main thread only, using main thread CC is fine, but will need to support other, non-main-thread, non-worker-thread threads.
Attachment #8781941 - Flags: feedback?(bugs) → feedback+
padenot might have some feedback on this stuff too.
Comment on attachment 8781942 [details] [diff] [review]
part 3 - Console API in worklet

>+WorkletGlobalScope::GetConsole(ErrorResult& aRv)
>+{
>+  if (!mConsole) {
>+    mConsole = Console::Create(mWindow, aRv);
>+    if (NS_WARN_IF(aRv.Failed())) {
>+      return nullptr;
>+    }
>+  }
This doesn't look right. The console object will have mWindow as the parent. It should have WorkletGlobalScope as parent.
This just happens to work since this stuff is same origin and all.

But I do think we should figure out the correct setup for non-main-thread worklets from beginning.

So, f+, but needs work
Attachment #8781942 - Flags: feedback?(bugs) → feedback+
FWIW, blink-dev had just email "Intent to Implement: AudioWorklet".
(Assignee)

Updated

3 years ago
Blocks: 1062849
(Assignee)

Comment 17

3 years ago
Daniel, I guess the next step is to have a valid use-case for testing this code. I don't want to land this as it is because it's not connected to anything. I would like to see some other pieces of the Houdini architecture connected to Worklet.
Or, it can also be the audioWorklet.
Flags: needinfo?(dholbert)
"CSS Paint" (bug 1302328) would be our first CSS Houdini usage of this abstraction.  We were originally (optimistically) planning on having an intern work on CSS Paint this summer, but another prerequisite feature turned out to be thornier & take up more time than our optimistic guess, so CSS Paint ended up being out-of-scope for that internship.

So: I don't immediately know how soon a layout/css full-timer will have cycles to work on CSS Paint, so I don't have a timeframe to give you right away.  (It might be soon, I just don't know.)  So, audioWorklet might be the first usage of this.
Flags: needinfo?(dholbert)
baku: would you be OK landing this but keeping dom.worklet.enabled turned off? I'm still working on staffing up the rendering bits of Houdini Paint, but it would be good to work off of Nightly as we go along. Thanks!
Flags: needinfo?(amarchesini)
(Assignee)

Comment 20

3 years ago
This patch introduces the Worklet object and it tests them.
The fetch requests are correctly done but the content is not handled yet.
Flags: needinfo?(amarchesini)
Attachment #8801164 - Flags: review?(bugs)
(Assignee)

Updated

3 years ago
Attachment #8775485 - Attachment is obsolete: true
(Assignee)

Comment 21

3 years ago
I'm not sure about how I set the compileOptions.
Attachment #8781941 - Attachment is obsolete: true
Attachment #8801165 - Flags: review?(bugs)
(Assignee)

Comment 22

3 years ago
This is main-thread only. I didn't apply your comments because so far I'm not considering to port it out of the main-thread.
Attachment #8781942 - Attachment is obsolete: true
Attachment #8801169 - Flags: review?(bugs)
(Assignee)

Comment 23

3 years ago
Attachment #8801635 - Flags: review?(bugs)
(Assignee)

Comment 24

3 years ago
Posted patch part 5 - dump() (obsolete) — Splinter Review
This is not part of the spec, but it's very useful to have.
Attachment #8801640 - Flags: review?(bugs)
(Assignee)

Comment 25

3 years ago
> baku: would you be OK landing this but keeping dom.worklet.enabled turned
> off? I'm still working on staffing up the rendering bits of Houdini Paint,
> but it would be good to work off of Nightly as we go along. Thanks!

As you can see, I want to have these patches in m-i.
I'm OK if you want to use this first implementation to continue with the Houdini spec, but keep in mind that it could be that we have to change the worklet code a lot if we want to go out of main-thread.
Comment on attachment 8801640 [details] [diff] [review]
part 5 - dump()

This must be behind a pref, perhaps the same pref as window.dump.
Comment on attachment 8801640 [details] [diff] [review]
part 5 - dump()

And without adding the pref now, we'll forget it easily.
Attachment #8801640 - Flags: review?(bugs) → review-
Comment on attachment 8801164 [details] [diff] [review]
part 1 - Worklet webIDL interface

> 
>+  // Exposed only for testing
Useless comment. The method is exposed whenever the pref used in .webidl is set to true,


> 
>+// For testing worklet only
>+partial interface Window {
I guess this comment is fine. You might want to make it even more stronger, saying that it must not be enabled 
anywhere by default.
Attachment #8801164 - Flags: review?(bugs) → review+
(Assignee)

Comment 29

3 years ago
Attachment #8801640 - Attachment is obsolete: true
Attachment #8801749 - Flags: review?(bugs)
Attachment #8801169 - Flags: review?(bugs) → review+
Comment on attachment 8801749 [details] [diff] [review]
part 5 - dump()


>+// Mozilla extensions
>+partial interface WorkletGlobalScope {
  Func=...
>+  void dump(optional DOMString str);


>+void
>+WorkletGlobalScope::Dump(const Optional<nsAString>& aString) const
>+{
>+  if (!nsContentUtils::DOMWindowDumpEnabled()) {
You should move this check to .webidl.
We don't want to expose .dump() in the worklet scope.
(No need to make the same mistakes we've done with Window)


> SimpleTest.waitForExplicitFinish();
> SpecialPowers.pushPrefEnv(
>   {"set": [["dom.worklet.testing.enabled", true],
>            ["dom.worklet.enabled", true]]},
>-  function() { loadTest("file_console.html"); });
>+  function() { loadTest("file_dump.html"); });
So this test never tests anything. I mean it is missing ok() or is() checks. 
I think this will cause orange. At least add dummy ok()
Attachment #8801749 - Flags: review?(bugs) → review+
Comment on attachment 8801635 [details] [diff] [review]
part 4 - cache for the imports

I'm a bit worried about the lifetime management.

Worklet has nsRefPtrHashtable<nsCStringHashKey, WorkletFetchHandler> mImportHandlers; and those handlers own Promises which then own global etc.

The cycle should be killed when resolving or rejecting, so should be fine.
But better to test this a bit on debug builds with
export XPCOM_MEM_LEAK_LOG=1
to see that there aren't at least shutdown leaks.


>+    // Maybe we already have an handler for this URI
a handler

This all is of course quite against the spec. Not dealing with module loading in general. But for testing this might ok.
(though, it is unclear to me why we need this caching when we don't even have module loading.)
Attachment #8801635 - Flags: review?(bugs) → review+
Comment on attachment 8801165 [details] [diff] [review]
part 2 - WorkletGlobalScope

This at least should use ModuleEvaluation I think, to be a bit closer to the spec.
Could we even somehow reuse scriptloader for all this?
jonco might have an opinion.
Attachment #8801165 - Flags: review?(bugs) → review-
(Assignee)

Comment 33

3 years ago
jonco, this is set of patches is about a new API called Worklet. Can you please give me a feedback based on patch 2 and comment 32? Thanks
Flags: needinfo?(jcoppeard)
This doesn't fetch imports or evaluate the script as a module.  This is probably fine for testing but I'm not sure we should expose this until it's more spec-compliant.

It would be really great if we could reuse some of the code in the script loader (and would be useful for other efforts, e.g. bug 1308512).  This is not trivial unfortunately.  I think it would require factoring out the module loader into a separate class with a caller-provided a fetch interface passed in.  Currently nsScriptLoader uses MozPromise whereas this uses Promise so that's something else that would need to be addressed.
Flags: needinfo?(jcoppeard)
Depends on: 1311726
None of this is ready to be exposed to web.
The implementation is just to let layout folks play with worklets.
(In reply to Olli Pettay [:smaug] from comment #35)
Ah OK.  I do think we should at least parse the code as a module though.
Right.
And we need bug 1311726 or something simlar sooner than later to actually expose Worklets to web.
The current patches are just preliminary to fulfill comment 19.
(Assignee)

Comment 38

2 years ago
smaug, do we want to wait for bug 1311726 or continue and change the code later?
Flags: needinfo?(bugs)
Can we somehow load a module without that?

I'm just a bit worried that without supporting modules from the beginning, writing tests for worklets using this preliminary implementation would be rather useless.
Flags: needinfo?(bugs) → needinfo?(jcoppeard)
(In reply to Olli Pettay [:smaug] (reviewing overload, queue closed for a day or two) from comment #39)
As long as the module doesn't import anything you can load it by calling JS::CompileModule to get a module record object, and then calling JS::ModuleDeclarationInstantiation and then JS::ModuleEvaluation on that.
Flags: needinfo?(jcoppeard)
(Assignee)

Comment 41

2 years ago
What about if we land what we have and then we fix everything as follow up? And keep everything behind pref. In the meantime CSS team can continue the implementation of Houdini spec.
Flags: needinfo?(bugs)
I guess we can do that. Script handling isn't quite right so tests written based on it may break later.
Please add some explicit check, at least some assertion, that the code can't be enabled in release builds.

But do we have plans how to get worklets into shippable state? And this prototype implementation doesn't really let one to start implement the audio worklets, since those really need separate thread. (though, I don't know when we have plans to start implement that stuff)
Flags: needinfo?(bugs)
(Assignee)

Comment 43

2 years ago
We don't have real plans for audioWorklet yet but I'm happy to bring this code in a better state if we have a roadmap. Maybe this can be a topic for the next work week?
Flags: needinfo?(padenot)
(Assignee)

Comment 44

2 years ago
Attachment #8807135 - Flags: review?(bugs)
(Assignee)

Updated

2 years ago
Attachment #8801165 - Flags: review- → review?(bugs)
Comment on attachment 8807135 [details] [diff] [review]
part 6 - assertions

You need to then also ensure the tests aren't run in beta or release
Attachment #8807135 - Flags: review?(bugs) → review+
Also, when landing the patches, the commit message could say something about this being just a prototype version, so that no one thinks  we have support for worklets yet.
Summary: Implement Houdini "Worklets Level 1" spec → Implement a prototype version of Houdini "Worklets Level 1" spec
Blocks: 1315239
Comment on attachment 8801165 [details] [diff] [review]
part 2 - WorkletGlobalScope

>+    nsCOMPtr<nsIStreamLoader> loader;
>+    rv = NS_NewStreamLoader(getter_AddRefs(loader), this);
>+    if (NS_WARN_IF(NS_FAILED(rv))) {
>+      mPromise->MaybeReject(rv);
>+      return;
>+    }
>+
>+    rv = pump->AsyncRead(loader, nullptr);
>+    if (NS_WARN_IF(NS_FAILED(rv))) {
>+      mPromise->MaybeReject(rv);
>+      return;
>+    }
>+
>+    nsCOMPtr<nsIThreadRetargetableRequest> rr = do_QueryInterface(pump);
>+    if (rr) {
>+      nsCOMPtr<nsIEventTarget> sts =
>+        do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID);
>+      rv = rr->RetargetDeliveryTo(sts);
>+      if (NS_FAILED(rv)) {
>+        NS_WARNING("Failed to dispatch the nsIInputStreamPump to a IO thread.");
>+      }
>+    }
>+  }
ok, you copy some of the stuff from Fetch handling here.
I wonder why you couldn't just use response->GetBlob and then read the data from that. But I guess this works too

>+
>+  NS_IMETHOD
>+  OnStreamComplete(nsIStreamLoader* aLoader, nsISupports* aContext,
>+                   nsresult aStatus, uint32_t aStringLen,
>+                  const uint8_t* aString)
align the last argument

>+  {
MOZ_ASSERT that we're in the main thread
Attachment #8801165 - Flags: review?(bugs) → review+

Comment 48

2 years ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bed3a0e2332
Implement a prototype version of Houdini "Worklets Level 1" spec - part 1 - WebIDL interface, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/59c14257aba5
Implement a prototype version of Houdini "Worklets Level 1" spec - part 2 - WorkletGlobalScope, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/3769e657d104
Implement a prototype version of Houdini "Worklets Level 1" spec - part 3 - Console API in worklet, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/1946c5517376
Implement a prototype version of Houdini "Worklets Level 1" spec - part 4 - cache for the imports, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/c77883513897
Implement a prototype version of Houdini "Worklets Level 1" spec - part 5 - dump(), r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/f99b99cc076d
Implement a prototype version of Houdini "Worklets Level 1" spec - part 6 - assertions, r=smaug
(In reply to Andrea Marchesini [:baku] from comment #43)
> We don't have real plans for audioWorklet yet but I'm happy to bring this
> code in a better state if we have a roadmap. Maybe this can be a topic for
> the next work week?

Yes, we can also implement some things right now. Ping me on IRC, I'm less busy that a week ago now, we can chat.
Flags: needinfo?(padenot)
I think it is a bit too early to document this (very experimental, behind a flag, and anyway would not be very useful).

I've moved the dev-doc-needed flag to bug 1315239. And we will document when this bug is resolved (or when AudioWorklet, or Houdini CSS Painting API, lands, as we have a dev-doc-needed flag there too :-) )
Keywords: dev-doc-needed
No longer blocks: 1323948
Depends on: 1332378
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.