Closed Bug 1328964 Opened 3 years ago Closed 2 years ago

Worklet should run in a separate thread.

Categories

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

50 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: baku, Assigned: karlt)

References

(Blocks 3 open bugs)

Details

Attachments

(23 files, 11 obsolete files)

7.82 KB, patch
smaug
: review+
Details | Diff | Splinter Review
41.22 KB, patch
smaug
: review+
Details | Diff | Splinter Review
45.51 KB, patch
smaug
: review+
Details | Diff | Splinter Review
10.11 KB, patch
smaug
: review+
Details | Diff | Splinter Review
59 bytes, text/x-review-board-request
karlt
: review+
Details
59 bytes, text/x-review-board-request
karlt
: review+
Details
59 bytes, text/x-review-board-request
karlt
: review+
Details
59 bytes, text/x-review-board-request
karlt
: review+
Details
59 bytes, text/x-review-board-request
karlt
: review+
Details
59 bytes, text/x-review-board-request
karlt
: review+
Details
59 bytes, text/x-review-board-request
karlt
: review+
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
This is still an intermediate state for the final design of worklet.
Currently worklets run on the main-thread. This step is about having a separate worklet thread.
Assignee: nobody → amarchesini
Attachment #8824173 - Flags: review?(bugs)
Attached patch part 2 - WorkletThread (obsolete) — Splinter Review
This patch works, but it crashes on shutdown.
Attachment #8824174 - Flags: feedback?(bugs)
Attachment #8824175 - Flags: review?(bugs)
Comment on attachment 8824174 [details] [diff] [review]
part 2 - WorkletThread

I tried to see why the sScriptSettingsTLS stack isn't empty, but couldn't.
Or are we getting bogus value from sScriptSettingsTLS.get()?
Attachment #8824174 - Flags: feedback?(bugs)
Attachment #8824173 - Flags: review?(bugs) → review+
Comment on attachment 8824175 [details] [diff] [review]
part 3 - Console API exposed to workers

Can I review this after the main patch is working, in case fixes to it also change this one.
Attachment #8824175 - Flags: review?(bugs)
Blocks: 1329166
Attachment #8824174 - Attachment is obsolete: true
Attachment #8828881 - Flags: review?(bugs)
Attachment #8824175 - Attachment is obsolete: true
Attachment #8828882 - Flags: review?(bugs)
Attachment #8824173 - Attachment description: patch 1 - Remove the window from worklet → part 1 - Remove the window from worklet
Comment on attachment 8828881 [details] [diff] [review]
part 2 - WorkletThread

> nsContentUtils::GetCurrentJSContextForThread()
> {
>   MOZ_ASSERT(IsInitialized());
>   if (MOZ_LIKELY(NS_IsMainThread())) {
>     return GetCurrentJSContext();
>-  } else {
>-    return workers::GetCurrentThreadJSContext();
>-  }
>+  }
>+
>+  if (WorkletThread::IsOnWorkletThread()) {
>+    return WorkletThread::Get()->GetJSContext();
>+  }
>+
>+  return workers::GetCurrentThreadJSContext();
This looks a bit ugly, especially given what IsOnWorkletThread does.
Couldn't you just use CycleCollectedJSContext::Get()->Context(); here, though
since workers seem to have special handling, perhaps add a flag to 
CycleCollectedJSContext to tell it is shutting down and then rename Context() to GetContext() and make it return null
during shutdown.


CycleCollectedJSContext might be useful also to implement IsOnWorkletThread.
Perhaps CycleCollectedJSContext could have enum to describe the type of the thread it is attached to, and then in
IsOnWorkletThread do something like
CycleCollectedJSContext* ccjc = CycleCollectedJSContext::Get();
return ccjs && ccjs->IsWorkletThread()



>+
>+  JS::CompileOptions compileOptions(cx);
>+  compileOptions.setIntroductionType("Worklet");
>+  compileOptions.setFileAndLine(NS_ConvertUTF16toUTF8(mHandler->URL()).get(), 0);
>+  compileOptions.setVersion(JSVERSION_DEFAULT);
>+  compileOptions.setIsRunOnce(true);
>+
>+  // We only need the setNoScriptRval bit when compiling off-thread here,
>+  // since otherwise nsJSUtils::EvaluateString will set it up for us.
Unrelated comment. You aren't using nsJSUtils

>+struct WorkletPrincipal final : public JSPrincipals
>+{
>+  bool write(JSContext* aCx, JSStructuredCloneWriter* aWriter) override {
{ goes to its own line.


>+
>+  void
>+  DispatchToMicroTask(already_AddRefed<nsIRunnable> aRunnable) override
>+  {
>+    RefPtr<nsIRunnable> runnable(aRunnable);
>+
>+    MOZ_ASSERT(!NS_IsMainThread());
>+    MOZ_ASSERT(runnable);
>+
>+    WorkletThread* workletThread = WorkletThread::Get();
>+    MOZ_ASSERT(workletThread);
>+
>+    JSContext* cx = workletThread->GetJSContext();
>+    MOZ_ASSERT(cx);
>+
>+    JS::Rooted<JSObject*> global(cx, JS::CurrentGlobalOrNull(cx));
>+    MOZ_ASSERT(global);
Could you put all these assertions and such inside #ifdef DEBUG, since .push() is really the only real functionality the method has.


>+WorkletThread::RunEventLoop(JSContext* aParentContext)
>+{
>+  MOZ_ASSERT(!NS_IsMainThread());
>+
>+  PR_SetCurrentThreadName("worklet");
>+
>+  WorkletJSContext context(this);
>+  nsresult rv = context.Initialize(aParentContext);
>+  if (NS_WARN_IF(NS_FAILED(rv))) {
>+    // TODO: error propagation
>+    return;
>+  }
>+
>+  // FIXME: JS_SetDefaultLocale
>+  // FIXME: JSSettings
>+  // FIXME: JS_SetNativeStackQuota
>+  // FIXME: JS_SetSecurityCallbacks
>+  // FIXME: JS::SetAsmJSCacheOps
>+  // FIXME: JS::SetAsyncTaskCallbacks
>+  // FIXME: JS_AddInterruptCallback
>+  // FIXME: JS::SetCTypesActivityCallback
>+  // FIXME: JS_SetGCZeal

File a bug for the FIXME part


>+WorkletThread::IsOnWorkletThread()
>+{
>+  const char* threadName = PR_GetThreadName(PR_GetCurrentThread());
>+  return threadName && !strcmp(threadName, "worklet");
>+}
So as I said earlier, I think we can do better than this, and rely on CCJSContext


This is mostly good, though it is a bit annoying that so much is copied from workers.

Could you fix the nits, possibly in a separate patch and I think we need so clean ups after this has landed to
consolidate worker and worklet code when possible.
r+, if there is that followup patch
Attachment #8828881 - Flags: review?(bugs) → review+
Comment on attachment 8828882 [details] [diff] [review]
part 3 - Console API exposed to worklets

These "move tons of code and do also changes" patches are so difficult to review.
Please do code moves first and then modifications in separate patches. 
Close to impossible patch to review, so I don't have great trust on the review quality.
Most of the patch is code move, but there is also other stuff...



>+    // We don't need to set a parent object in mCallData bacause there are not
because
Attachment #8828882 - Flags: review?(bugs) → review+
If a worklet script hangs ( while(true); ), does it get killed?
Blocks: 1333412
(In reply to Olli Pettay [:smaug] from comment #10)
> If a worklet script hangs ( while(true); ), does it get killed?

Not yet. This must be implemented.
Comment on attachment 8829914 [details] [diff] [review]
part 2 - WorkletThread - second part

>+  virtual JSContext* GetWorkerWorkletSafeContext() const
>+  {
>+    MOZ_CRASH("This method should be override in Worklet and Worker code.");
>+    return nullptr;
>+  }
>+
What if this method returns Context() by default.
Then nsContentUtils::GetCurrentJSContextForThread() could just always return
CycleCollectedJSContext::Get()->GetWorkerWorkletSafeContext();
And add a comment then in nsContentUtils::GetCurrentJSContextForThread() why it is being called.


(I still wonder what could be better name for GetWorkerWorkletSafeContext. I know I suggested it, but something more generic name which hints about
shutdown of the thread would be better)
Attachment #8829914 - Flags: review?(bugs) → review+
Assuming P3 due to lack of recent activity. Feel free to correct me.
Priority: -- → P3
Karl, thanks for taking this. I'm uploading baku's patches rebased to reviewboard. There's been some serious bitrot, but I tried to put fix-ups in separate patches to leave the originals alone as much as possible. Still, earlier reviews probably need to be redone due to age. - Last I tried these patches months ago, I still got GC crashes. There's also a chance I messed something up by rebasing them.
Assignee: amarchesini → karlt
Comment on attachment 8928213 [details]
Bug 1328964 - part 2 - WorkletThread

https://reviewboard.mozilla.org/r/199424/#review204544


C/C++ static analysis found 11 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: dom/worklet/Worklet.cpp:43
(Diff revision 1)
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +  }
> +
> +  NS_IMETHOD
> +  Run() override;

Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]

::: dom/worklet/Worklet.cpp:223
(Diff revision 1)
>        return NS_OK;
>      }
>  
>      // Moving the ownership of the buffer
> -    JS::SourceBufferHolder buffer(scriptTextBuf, scriptTextLength,
> -                                  JS::SourceBufferHolder::GiveOwnership);
> +    nsCOMPtr<nsIRunnable> runnable =
> +      new ExecutionRunnable(this, mWorklet->Type(), scriptTextBuf,

Error: Variable of type 'mozilla::dom::executionrunnable' only valid on the stack [clang-tidy: mozilla-scope]

::: dom/worklet/Worklet.cpp:223
(Diff revision 1)
>        return NS_OK;
>      }
>  
>      // Moving the ownership of the buffer
> -    JS::SourceBufferHolder buffer(scriptTextBuf, scriptTextLength,
> -                                  JS::SourceBufferHolder::GiveOwnership);
> +    nsCOMPtr<nsIRunnable> runnable =
> +      new ExecutionRunnable(this, mWorklet->Type(), scriptTextBuf,

Error: Variable of type 'mozilla::dom::executionrunnable' only valid on the stack [clang-tidy: mozilla-scope]

::: dom/worklet/Worklet.cpp:223
(Diff revision 1)
>        return NS_OK;
>      }
>  
>      // Moving the ownership of the buffer
> -    JS::SourceBufferHolder buffer(scriptTextBuf, scriptTextLength,
> -                                  JS::SourceBufferHolder::GiveOwnership);
> +    nsCOMPtr<nsIRunnable> runnable =
> +      new ExecutionRunnable(this, mWorklet->Type(), scriptTextBuf,

Error: Variable of type 'mozilla::dom::executionrunnable' only valid on the stack [clang-tidy: mozilla-scope]

::: dom/worklet/Worklet.cpp:223
(Diff revision 1)
>        return NS_OK;
>      }
>  
>      // Moving the ownership of the buffer
> -    JS::SourceBufferHolder buffer(scriptTextBuf, scriptTextLength,
> -                                  JS::SourceBufferHolder::GiveOwnership);
> +    nsCOMPtr<nsIRunnable> runnable =
> +      new ExecutionRunnable(this, mWorklet->Type(), scriptTextBuf,

Error: Variable of type 'mozilla::dom::executionrunnable' only valid on the stack [clang-tidy: mozilla-scope]

::: dom/worklet/Worklet.cpp:223
(Diff revision 1)
>        return NS_OK;
>      }
>  
>      // Moving the ownership of the buffer
> -    JS::SourceBufferHolder buffer(scriptTextBuf, scriptTextLength,
> -                                  JS::SourceBufferHolder::GiveOwnership);
> +    nsCOMPtr<nsIRunnable> runnable =
> +      new ExecutionRunnable(this, mWorklet->Type(), scriptTextBuf,

Error: Variable of type 'mozilla::dom::executionrunnable' only valid on the stack [clang-tidy: mozilla-scope]

::: dom/worklet/WorkletThread.cpp:58
(Diff revision 1)
> +    MOZ_ASSERT(mWorkletThread);
> +    return mWorkletThread;
> +  }
> +
> +private:
> +  WorkletThreadContextPrivate(const WorkletThreadContextPrivate&) = delete;

Warning: Deleted member function should be public [clang-tidy: modernize-use-equals-delete]

::: dom/worklet/WorkletThread.cpp:59
(Diff revision 1)
> +    return mWorkletThread;
> +  }
> +
> +private:
> +  WorkletThreadContextPrivate(const WorkletThreadContextPrivate&) = delete;
> +  WorkletThreadContextPrivate& operator=(const WorkletThreadContextPrivate&) = delete;

Warning: Deleted member function should be public [clang-tidy: modernize-use-equals-delete]

::: dom/worklet/WorkletThread.cpp:114
(Diff revision 1)
> +    MOZ_ASSERT(!NS_IsMainThread());
> +
> +    nsCycleCollector_startup();
> +  }
> +
> +  ~WorkletJSContext()

Warning: Annotate this function with 'override' or (rarely) 'final' [clang-tidy: modernize-use-override]

  ~WorkletJSContext()
  ^
                      override

::: dom/worklet/WorkletThread.cpp:234
(Diff revision 1)
> +      JS_GetParentContext(CycleCollectedJSContext::Get()->Context());
> +    MOZ_ASSERT(mParentContext);
> +  }
> +
> +  NS_IMETHOD
> +  Run() override

Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]

::: dom/worklet/WorkletThread.cpp:257
(Diff revision 1)
> +    MOZ_ASSERT(aWorkletThread);
> +    MOZ_ASSERT(NS_IsMainThread());
> +  }
> +
> +  NS_IMETHOD
> +  Run() override

Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]
Comment on attachment 8928215 [details]
Bug 1328964 - Split WorkletJSContext into WorkletJSRuntime to catch up w/bug 1343396.

https://reviewboard.mozilla.org/r/199428/#review204546


C/C++ static analysis found 7 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: dom/worklet/WorkletThread.cpp:113
(Diff revision 1)
> +  explicit WorkletJSRuntime(JSContext* aCx)
> +    : CycleCollectedJSRuntime(aCx)
> +  {
> +  }
> +
> +  ~WorkletJSRuntime()

Warning: Annotate this function with 'override' or (rarely) 'final' [clang-tidy: modernize-use-override]

  ~WorkletJSRuntime()
  ^
                      override

::: dom/worklet/WorkletThread.cpp:113
(Diff revision 1)
> +  explicit WorkletJSRuntime(JSContext* aCx)
> +    : CycleCollectedJSRuntime(aCx)
> +  {
> +  }
> +
> +  ~WorkletJSRuntime()

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

::: dom/worklet/WorkletThread.cpp:118
(Diff revision 1)
> +  ~WorkletJSRuntime()
> +  {
> +  }
> +
> +  virtual void
> +  PrepareForForgetSkippable() override

Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]

::: dom/worklet/WorkletThread.cpp:123
(Diff revision 1)
> +  PrepareForForgetSkippable() override
> +  {
> +  }
> +
> +  virtual void
> +  BeginCycleCollectionCallback() override

Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]

::: dom/worklet/WorkletThread.cpp:128
(Diff revision 1)
> +  BeginCycleCollectionCallback() override
> +  {
> +  }
> +
> +  virtual void
> +  EndCycleCollectionCallback(CycleCollectorResults &aResults) override

Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]

::: dom/worklet/WorkletThread.cpp:133
(Diff revision 1)
> +  EndCycleCollectionCallback(CycleCollectorResults &aResults) override
> +  {
> +  }
> +
> +  virtual void
> +  DispatchDeferredDeletion(bool aContinuation, bool aPurge) override

Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]

::: dom/worklet/WorkletThread.cpp:140
(Diff revision 1)
> +    MOZ_ASSERT(!aContinuation);
> +    nsCycleCollector_doDeferredDeletion();
> +  }
> +
> +  virtual void
> +  CustomGCCallback(JSGCStatus aStatus) override

Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]
Comment on attachment 8928216 [details]
Bug 1328964 - part 3 - Console API exposed to worklets

https://reviewboard.mozilla.org/r/199430/#review204548


C/C++ static analysis found 9 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: dom/console/Console.cpp:321
(Diff revision 1)
> -class ConsoleRunnable : public WorkerProxyToMainThreadRunnable
> -                      , public StructuredCloneHolderBase
> +// This base class must be extended for Worker and for Worklet.
> +class ConsoleRunnable : public StructuredCloneHolderBase
>  {
>  public:
> -  explicit ConsoleRunnable(Console* aConsole)
> +  virtual
> +  ~ConsoleRunnable()

Warning: Prefer using 'override' or (rarely) 'final' instead of 'virtual' [clang-tidy: modernize-use-override]

  ~ConsoleRunnable()
  ^
                     override

::: dom/console/Console.cpp:562
(Diff revision 1)
> +    WorkletThread::AssertIsOnWorkletThread();
> +    MOZ_ASSERT(mWorkletThread);
> +  }
> +
> +  virtual
> +  ~ConsoleWorkletRunnable() = default;

Warning: Prefer using 'override' or (rarely) 'final' instead of 'virtual' [clang-tidy: modernize-use-override]

  ~ConsoleWorkletRunnable() = default;
  ^
                            override 

::: dom/console/Console.cpp:565
(Diff revision 1)
> +
> +  virtual
> +  ~ConsoleWorkletRunnable() = default;
> +
> +  NS_IMETHOD
> +  Run() override

Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]

::: dom/console/Console.cpp:635
(Diff revision 1)
> +    WorkletThread::AssertIsOnWorkletThread();
> +    MOZ_ASSERT(aCallData);
> +    aCallData->AssertIsOnOwningThread();
> +  }
> +
> +  ~ConsoleCallDataWorkletRunnable()

Warning: Annotate this function with 'override' or (rarely) 'final' [clang-tidy: modernize-use-override]

  ~ConsoleCallDataWorkletRunnable()
  ^
                                    override

::: dom/console/Console.cpp:641
(Diff revision 1)
> +  {
> +    MOZ_ASSERT(!mCallData);
> +  }
> +
> +  virtual void
> +  RunOnMainThread() override

Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]

::: dom/console/Console.cpp:666
(Diff revision 1)
> +
> +    ProcessCallData(cx, mConsole, mCallData);
> +  }
> +
> +  virtual void
> +  ReleaseData() override

Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]

::: dom/console/Console.cpp:685
(Diff revision 1)
>      : WorkerProxyToMainThreadRunnable(GetCurrentThreadWorkerPrivate())
>      , mConsole(aConsole)
>    {}
>  
>    virtual
> -  ~ConsoleRunnable()
> +  ~ConsoleWorkerRunnable() = default;

Warning: Prefer using 'override' or (rarely) 'final' instead of 'virtual' [clang-tidy: modernize-use-override]

  ~ConsoleWorkerRunnable() = default;
  ^
                           override 

::: dom/console/Console.cpp:822
(Diff revision 1)
>      mWorkerPrivate->AssertIsOnWorkerThread();
>      mCallData->AssertIsOnOwningThread();
>    }
>  
>  private:
> -  ~ConsoleCallDataRunnable()
> +  ~ConsoleCallDataWorkerRunnable()

Warning: Annotate this function with 'override' or (rarely) 'final' [clang-tidy: modernize-use-override]

  ~ConsoleCallDataWorkerRunnable()
  ^
                                   override

::: dom/console/Console.cpp:941
(Diff revision 1)
> +
> +    ProcessProfileData(cx, mConsole, mAction);
>    }
>  
> -  RefPtr<ConsoleCallData> mCallData;
> +  virtual void
> +  ReleaseData() override

Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]
Depends on: 1442776
Comment on attachment 8928214 [details]
Bug 1328964 - Add names to Runnables to make it compile.

https://reviewboard.mozilla.org/r/199426/#review233052

Thank you.  I'll fold this into "part 2 - WorkletThread" before landing, so that each changeset compiles.

::: dom/worklet/WorkletThread.cpp:87
(Diff revision 1)
>  
>  JSObject*
>  Wrap(JSContext *cx, JS::HandleObject existing, JS::HandleObject obj)
>  {
>    if (existing) {
> -    js::Wrapper::Renew(cx, existing, obj,
> +    js::Wrapper::Renew(existing, obj,

From merge of https://hg.mozilla.org/mozilla-central/rev/7c3418481c2d59a50e8a8b807aecf7ded05de845

::: dom/worklet/WorkletThread.cpp:272
(Diff revision 1)
> -  : nsThread(nsThread::NOT_MAIN_THREAD, kWorkletStackSize)
> +  : nsThread(MakeNotNull<ThreadEventQueue<mozilla::EventQueue>*>(
> +               MakeUnique<mozilla::EventQueue>()),
> +             nsThread::NOT_MAIN_THREAD, kWorkletStackSize)

Consistent with WorkerThread changes
https://hg.mozilla.org/integration/mozilla-inbound/rev/36ef70762b74#l3.30
Attachment #8928214 - Flags: review+
Comment on attachment 8928215 [details]
Bug 1328964 - Split WorkletJSContext into WorkletJSRuntime to catch up w/bug 1343396.

https://reviewboard.mozilla.org/r/199428/#review233066

I'll fold this also into "part 2 - WorkletThread" before landing.

Also includes merge with
https://hg.mozilla.org/mozilla-central/rev/920d5dfea5de

(Looks like the shutdown order with CC and CustomGCCallback
was already broken.  I'll fix that separately.)
Attachment #8928215 - Flags: review+
Comment on attachment 8928217 [details]
Bug 1328964 - Name one more Runnable to make it compile.

https://reviewboard.mozilla.org/r/199432/#review233064

I'll fold this into "part 3 - Console API exposed to worklets" before landing, so that each changeset compiles.
Attachment #8928217 - Flags: review+
Comment on attachment 8928212 [details]
Bug 1328964 - part 1 - Remove the window from worklet

https://reviewboard.mozilla.org/r/199422/#review233070

Reviewed the rebase from https://bugzilla.mozilla.org/attachment.cgi?id=8824173
Nothing remarkable.
Attachment #8928212 - Flags: review+
Comment on attachment 8928213 [details]
Bug 1328964 - part 2 - WorkletThread

https://reviewboard.mozilla.org/r/199424/#review233068

Reviewed the rebase from https://bugzilla.mozilla.org/attachment.cgi?id=8828881

::: dom/worklet/Worklet.h:55
(Diff revision 1)
>    WorkletGlobalScope*
>    GetOrCreateGlobalScope(JSContext* aCx);

Dropped the removal of this declaration, somehow.
The implementation is removed in this patch.

I'll restore removal of the declaration.

::: dom/worklet/Worklet.cpp
(Diff revision 1)
> -
> -    JS::Rooted<JSObject*> globalObj(cx, globalScope->GetGlobalJSObject());
> -
> -    (void) new XPCWrappedNativeScope(cx, globalObj);
> -
> -    NS_ConvertUTF16toUTF8 url(mURL);

A bad merge conflict resolution dropped https://hg.mozilla.org/mozilla-central/rev/90a7bc300af3

I'll add that.
Attachment #8928213 - Flags: review+
Comment on attachment 8928218 [details]
Bug 1328964 - part 4 - WorkletThread - second part

https://reviewboard.mozilla.org/r/199434/#review233074

Reviewed rebase from https://bugzilla.mozilla.org/attachment.cgi?id=8829914

::: commit-message-9c83e:1
(Diff revision 1)
> +Bug 1328964 - part 4 - WorkletThread - second part

I think I'll move this back between parts 2 and 3.

Folding with part 2 may even reduce some churn.

::: dom/workers/RuntimeService.cpp
(Diff revision 1)
> -  WorkerPrivate* wp = GetCurrentThreadWorkerPrivate();
> -  if (!wp) {
> -    return nullptr;
> -  }
> -  return wp->GetJSContext();

I doubt the conflict with this changeset was resolved correctly.
https://hg.mozilla.org/mozilla-central/rev/df94a2bfc2436b00a776c9083cc3ae4dda01a1fe

I assume the !wp check should move to GetWorkerWorkletSafeContext().

Does nsContentUtils::GetCurrentJSContextForThread() need a null check on CycleCollectedJSContext::Get()?
Attachment #8928218 - Flags: review+
Comment on attachment 8958720 [details]
Bug 1328964 - part 1 - Remove the window from worklet

https://reviewboard.mozilla.org/r/227622/#review233378

This is https://bugzilla.mozilla.org/attachment.cgi?id=8824173 on its original revision.
Comment on attachment 8958720 [details]
Bug 1328964 - part 1 - Remove the window from worklet

https://reviewboard.mozilla.org/r/227622/#review233384

This adds Andrea's other patches on top of part 1 rebased to a parent revision where the other patches apply cleanly.
Reviewboard detects some of the moved code, but doesn't do as well as I hoped.
Comment on attachment 8928216 [details]
Bug 1328964 - part 3 - Console API exposed to worklets

https://reviewboard.mozilla.org/r/199430/#review233764

::: dom/console/Console.cpp
(Diff revision 1)
> -    if (NS_WARN_IF(!argumentsObj)) {
> -      return;
> -    }

I think the conflict with
https://hg.mozilla.org/mozilla-central/rev/bbd8e6ecf4a0#l1.12
should be resolved by moving this to ProcessProfileData() with the surrounding code.
Attachment #8928216 - Flags: review+
Comment on attachment 8928218 [details]
Bug 1328964 - part 4 - WorkletThread - second part

https://reviewboard.mozilla.org/r/199434/#review233074

> I doubt the conflict with this changeset was resolved correctly.
> https://hg.mozilla.org/mozilla-central/rev/df94a2bfc2436b00a776c9083cc3ae4dda01a1fe
> 
> I assume the !wp check should move to GetWorkerWorkletSafeContext().
> 
> Does nsContentUtils::GetCurrentJSContextForThread() need a null check on CycleCollectedJSContext::Get()?

This patch is changing WorkerRunnables from calling GetCurrentThreadJSContext()
to calling nsContentUtils::GetCurrentJSContextForThread().

https://bugzilla.mozilla.org/show_bug.cgi?id=1315446#c20
assumed that this call can happen after this point in killing WorkerPrivate::DoRunLoop()
https://hg.mozilla.org/releases/mozilla-release/annotate/7d9973f1bba7/dom/workers/WorkerPrivate.cpp#l4507

The crash stack does not show WorkerThreadPrimaryRunnable on the stack, and so
its WorkerJSContext would now have gone out of scope.

That means that GetCurrentJSContextForThread() needs to null check on
CycleCollectedJSContext::Get().

GetWorkerWorkletSafeContext() could not be called in that situation because
the WorkerJSContext no longer exists.

GetWorkerPrivateFromContext() can not return null from
WorkerJSContext::GetWorkerWorkletSafeContext() because the context private is
not cleared until the WorkerJSContext destructor, and so no null check is
required in GetWorkerWorkletSafeContext() (as indicated by the assert).
Comment on attachment 8958720 [details]
Bug 1328964 - part 1 - Remove the window from worklet

https://reviewboard.mozilla.org/r/227622/#review234506

This is exactly the same code as revision 7a1e7727c7b6 from Jan-Ivar's rebase, but I moved "WorkletThread - second part" to before "part 3 - Console API eposed to worklets".
Comment on attachment 8958720 [details]
Bug 1328964 - part 1 - Remove the window from worklet

https://reviewboard.mozilla.org/r/227622/#review234516

This is essentially https://reviewboard.mozilla.org/r/227622/diff/2/
rebased to the same parent revision c616a6fd5e4b20cca139fcdd3957682afaa862b9
as Jan-Ivar's rebase, but the result is a little different.
The differences are itemized in separate patches.

I've asked Jan-Ivar to review these parts associated with the merges he
performed.  (Then I'll fold them into four parts corresponding to those that
Andrea wrote, address Olli's outstanding review comments, and add the other
parts that are necessary to land this.  I'll ask Andrea for review on rebases
across his recent refactorings.)
Comment on attachment 8960061 [details]
Bug 1328964 - Split WorkletJSContext into WorkletJSRuntime to catch up w/bug 1343396.

https://reviewboard.mozilla.org/r/228832/#review234522


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/worklet/WorkletThread.cpp:113
(Diff revision 1)
> +  explicit WorkletJSRuntime(JSContext* aCx)
> +    : CycleCollectedJSRuntime(aCx)
> +  {
> +  }
> +
> +  ~WorkletJSRuntime()

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]
Comment on attachment 8960067 [details]
bug 1328964 restore removal of declaration for non-existant Worklet::GetOrCreateGlobalScope()

https://reviewboard.mozilla.org/r/228840/#review234602
Attachment #8960067 - Flags: review?(jib) → review+
Comment on attachment 8960068 [details]
bug 1328964 restore fix for stack use after scope bug 1415086

https://reviewboard.mozilla.org/r/228842/#review234604
Attachment #8960068 - Flags: review?(jib) → review+
Comment on attachment 8960069 [details]
bug 1328964 restore fix for crash in runnable after thread shutdown bug 1315446

https://reviewboard.mozilla.org/r/228844/#review234612
Attachment #8960069 - Flags: review?(jib) → review+
Attachment #8960060 - Attachment is obsolete: true
Attachment #8960061 - Attachment is obsolete: true
Attachment #8960062 - Attachment is obsolete: true
Attachment #8960067 - Attachment is obsolete: true
Attachment #8960068 - Attachment is obsolete: true
Attachment #8960069 - Attachment is obsolete: true
Attachment #8960070 - Attachment is obsolete: true
Comment on attachment 8958720 [details]
Bug 1328964 - part 1 - Remove the window from worklet

https://reviewboard.mozilla.org/r/227622/#review240830

Push dc616c18f69d (which took 4m57s) is the same code as in push 64b42a345dcb, but the various changes required for the rebases are folded into four revisions (which compile separately) corresponding to the original patches from Andrea.
Comment on attachment 8958720 [details]
Bug 1328964 - part 1 - Remove the window from worklet

https://reviewboard.mozilla.org/r/227622/#review240832

Push 3248688a1ad6 is a rebase onto a recent revision 83de58ddda20.
(In reply to Olli Pettay [:smaug] (only webcomponents and event handling reviews, please) from comment #8)
> Comment on attachment 8828881 [details] [diff] [review]
> part 2 - WorkletThread
> 
> > nsContentUtils::GetCurrentJSContextForThread()
> > {
> >   MOZ_ASSERT(IsInitialized());
> >   if (MOZ_LIKELY(NS_IsMainThread())) {
> >     return GetCurrentJSContext();
> >-  } else {
> >-    return workers::GetCurrentThreadJSContext();
> >-  }
> >+  }
> >+
> >+  if (WorkletThread::IsOnWorkletThread()) {
> >+    return WorkletThread::Get()->GetJSContext();
> >+  }
> >+
> >+  return workers::GetCurrentThreadJSContext();
> This looks a bit ugly, especially given what IsOnWorkletThread does.
> Couldn't you just use CycleCollectedJSContext::Get()->Context(); here, though
> since workers seem to have special handling, perhaps add a flag to 
> CycleCollectedJSContext to tell it is shutting down and then rename
> Context() to GetContext() and make it return null
> during shutdown.

(In reply to Olli Pettay [:smaug] (only webcomponents and event handling reviews, please) from comment #13)
> Comment on attachment 8829914 [details] [diff] [review]
> part 2 - WorkletThread - second part
> 
> >+  virtual JSContext* GetWorkerWorkletSafeContext() const
> >+  {
> >+    MOZ_CRASH("This method should be override in Worklet and Worker code.");
> >+    return nullptr;
> >+  }
> >+
> What if this method returns Context() by default.
> Then nsContentUtils::GetCurrentJSContextForThread() could just always return
> CycleCollectedJSContext::Get()->GetWorkerWorkletSafeContext();
> And add a comment then in nsContentUtils::GetCurrentJSContextForThread() why
> it is being called.

I can't see any callers of nsContentUtils::GetCurrentJSContextForThread() that
shouldn't use Context(), and so I don't know of any good reason why Context()
should not be used for workers.

IsJSAPIActive() will return false when not in AutoJSAPI, and so should be
sufficient to detect if there are any callers after WorkerPrivate::DoRunLoop()
has returned.
Comment on attachment 8958720 [details]
Bug 1328964 - part 1 - Remove the window from worklet

https://reviewboard.mozilla.org/r/227622/#review240852

Push 882f5bcb9807 removes the nsContentUtils::GetCurrentJSContextForThread() /
GetWorkerWorkletSafeContext() changes addressing the first paragraph of
comment 8, because comment 13 is requesting a somewhat different approach.

I'll address those separately.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fedf433e69cc947fe87863fa09e7e17def0ee35
has the same patches as push 38a702c8b5da, but in a different order.
Comment on attachment 8958720 [details]
Bug 1328964 - part 1 - Remove the window from worklet

https://reviewboard.mozilla.org/r/227624/#review240918
Attachment #8958720 - Flags: review?(amarchesini) → review+
Comment on attachment 8958721 [details]
Bug 1328964 - part 2 - WorkletThread

https://reviewboard.mozilla.org/r/227634/#review240922

::: dom/worklet/WorkletPrincipal.cpp:17
(Diff revision 4)
> +namespace dom {
> +namespace WorkletPrincipal {
> +
> +struct WorkletPrincipal final : public JSPrincipals
> +{
> +  bool write(JSContext* aCx, JSStructuredCloneWriter* aWriter) override {

{ in a new line.

::: dom/worklet/WorkletThread.cpp:69
(Diff revision 4)
> +};
> +
> +// Helper functions
> +
> +bool
> +PreserveWrapper(JSContext *cx, JSObject *obj)

* next to JSContext, and rename to aCx, aObj

::: dom/worklet/WorkletThread.cpp:84
(Diff revision 4)
> +{
> +  MOZ_ASSERT_UNREACHABLE("Worklet principals refcount should never fall below one");
> +}
> +
> +JSObject*
> +Wrap(JSContext *cx, JS::HandleObject existing, JS::HandleObject obj)

same here. aCx. aExisting, aObj and no space before *

::: dom/worklet/WorkletThread.cpp:128
(Diff revision 4)
> +  BeginCycleCollectionCallback() override
> +  {
> +  }
> +
> +  virtual void
> +  EndCycleCollectionCallback(CycleCollectorResults &aResults) override

& next to CycleCollectorResults
Attachment #8958721 - Flags: review?(amarchesini) → review+
Comment on attachment 8958722 [details]
Bug 1328964 - part 2b - WorkletThread - second part

https://reviewboard.mozilla.org/r/227636/#review240930

You can probably merge this patch with the previous one.
Attachment #8958722 - Flags: review?(amarchesini) → review+
Comment on attachment 8958723 [details]
Bug 1328964 - part 3 - Console API exposed to worklets

https://reviewboard.mozilla.org/r/227638/#review240934

::: dom/console/Console.h:471
(Diff revision 5)
>    // This is used when Console is created and it's used only for JSM custom
>    // console instance.
>    mozilla::TimeStamp mCreationTimeStamp;
>  
>    friend class ConsoleCallData;
>    friend class ConsoleInstance;

alphabetic order.

::: dom/worklet/Worklet.cpp:22
(Diff revision 5)
>  #include "mozilla/dom/Response.h"
>  #include "mozilla/dom/ScriptSettings.h"
>  #include "mozilla/dom/ScriptLoader.h"
>  #include "nsIInputStreamPump.h"
>  #include "nsIThreadRetargetableRequest.h"
> +#include "mozilla/dom/DOMPrefs.h"

alphabetic order.
Attachment #8958723 - Flags: review?(amarchesini) → review+
Comment on attachment 8966467 [details]
bug 1328964 changes for worletthread rebase for microtask bug 1343396

https://reviewboard.mozilla.org/r/235154/#review240938
Attachment #8966467 - Flags: review?(amarchesini) → review+
Comment on attachment 8966479 [details]
bug 1328964 use = default in WorkletGlobalScope and derived constructors

https://reviewboard.mozilla.org/r/235170/#review240940
Attachment #8966479 - Flags: review?(amarchesini) → review+
Comment on attachment 8966480 [details]
bug 1328964 add override to ~WorkletJSContext/Runtime and use = default

https://reviewboard.mozilla.org/r/235172/#review240942
Attachment #8966480 - Flags: review?(amarchesini) → review+
Comment on attachment 8966481 [details]
bug 1328964 don't allocate SourceBufferHolder on the heap

https://reviewboard.mozilla.org/r/235174/#review240950
Attachment #8966481 - Flags: review?(amarchesini) → review+
Comment on attachment 8966484 [details]
bug 1328964 address modernize-use-override and modernize-use-equals-default

https://reviewboard.mozilla.org/r/235180/#review240952
Attachment #8966484 - Flags: review?(amarchesini) → review+
Comment on attachment 8966485 [details]
bug 1328964 don't try to cycle collect after worklet cycle collector has been shut down

https://reviewboard.mozilla.org/r/235182/#review240956
Attachment #8966485 - Flags: review?(amarchesini) → review+
Comment on attachment 8966486 [details]
bug 1328964 terminate worklet thread during xpcom shutdown

https://reviewboard.mozilla.org/r/235184/#review240962

::: dom/worklet/WorkletThread.h:94
(Diff revision 1)
>  
>    // Touched only on the worklet thread. This is a raw pointer because it's set
>    // and nullified by RunEventLoop().
>    JSContext* mJSContext;
> +
> +  bool mIsTerminating = false; // main thread

Initialize it in the CTOR.
Attachment #8966486 - Flags: review?(amarchesini) → review+
Comment on attachment 8966487 [details]
bug 1328964 add CycleCollectedJSContext::IsSystemCaller() to make ThreadsafeIsSystemCaller() safe for worklets

https://reviewboard.mozilla.org/r/235186/#review240964
Attachment #8966487 - Flags: review?(amarchesini) → review+
btw, thanks for finishing this!
Comment on attachment 8966482 [details]
bug 1328964 add CycleCollectedJSContext::GetAsWorkletJSContext() and use it in IsOnWorkletThread()

https://reviewboard.mozilla.org/r/235176/#review241026

Don't know quite the context here, but looks fine.
Attachment #8966482 - Flags: review?(bugs) → review+
Comment on attachment 8966483 [details]
bug 1328964 use nsContentUtils::GetCurrentJSContext() on all threads

https://reviewboard.mozilla.org/r/235178/#review241034

this does change the handling of Get*JSContext() but should be fine. And consistency is good.
Attachment #8966483 - Flags: review?(bugs) → review+
Comment on attachment 8966484 [details]
bug 1328964 address modernize-use-override and modernize-use-equals-default

https://reviewboard.mozilla.org/r/235180/#review240858

> Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

https://github.com/mozilla-releng/services/issues/1006

> Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

https://github.com/mozilla-releng/services/issues/1006

> Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

https://github.com/mozilla-releng/services/issues/1006
Comment on attachment 8966482 [details]
bug 1328964 add CycleCollectedJSContext::GetAsWorkletJSContext() and use it in IsOnWorkletThread()

https://reviewboard.mozilla.org/r/235176/#review241026

Yes, it has been a while.
This addresses the second paragraph of https://bugzilla.mozilla.org/show_bug.cgi?id=1328964#c8

Thank you, smaug and baku, for the reviews.
Attachment #8966467 - Attachment is obsolete: true
Attachment #8958722 - Attachment is obsolete: true
Comment on attachment 8958720 [details]
Bug 1328964 - part 1 - Remove the window from worklet

https://reviewboard.mozilla.org/r/227624/#review241686


Code analysis found 4 defects in this patch:
 - 4 defects found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/worklet/AudioWorkletGlobalScope.cpp:14
(Diff revision 6)
>  #include "mozilla/dom/FunctionBinding.h"
>  
>  namespace mozilla {
>  namespace dom {
>  
> -AudioWorkletGlobalScope::AudioWorkletGlobalScope(nsPIDOMWindowInner* aWindow)
> +AudioWorkletGlobalScope::AudioWorkletGlobalScope()

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

::: dom/worklet/AudioWorkletGlobalScope.cpp:14
(Diff revision 6)
>  #include "mozilla/dom/FunctionBinding.h"
>  
>  namespace mozilla {
>  namespace dom {
>  
> -AudioWorkletGlobalScope::AudioWorkletGlobalScope(nsPIDOMWindowInner* aWindow)
> +AudioWorkletGlobalScope::AudioWorkletGlobalScope()

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

::: dom/worklet/PaintWorkletGlobalScope.cpp:14
(Diff revision 6)
>  #include "mozilla/dom/FunctionBinding.h"
>  
>  namespace mozilla {
>  namespace dom {
>  
> -PaintWorkletGlobalScope::PaintWorkletGlobalScope(nsPIDOMWindowInner* aWindow)
> +PaintWorkletGlobalScope::PaintWorkletGlobalScope()

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

::: dom/worklet/PaintWorkletGlobalScope.cpp:14
(Diff revision 6)
>  #include "mozilla/dom/FunctionBinding.h"
>  
>  namespace mozilla {
>  namespace dom {
>  
> -PaintWorkletGlobalScope::PaintWorkletGlobalScope(nsPIDOMWindowInner* aWindow)
> +PaintWorkletGlobalScope::PaintWorkletGlobalScope()

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]
Pushed by ktomlinson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b76bff2ddfc6
part 1 - Remove the window from worklet r=baku
https://hg.mozilla.org/integration/autoland/rev/16197254fa36
part 2 - WorkletThread r=baku
https://hg.mozilla.org/integration/autoland/rev/bdd4af79d6f7
part 3 - Console API exposed to worklets r=baku
https://hg.mozilla.org/integration/autoland/rev/608bd57e774e
use = default in WorkletGlobalScope and derived constructors r=baku
https://hg.mozilla.org/integration/autoland/rev/bb29485e179b
add override to ~WorkletJSContext/Runtime and use = default r=baku
https://hg.mozilla.org/integration/autoland/rev/c3c9f4525efb
don't allocate SourceBufferHolder on the heap r=baku
https://hg.mozilla.org/integration/autoland/rev/06872fbe3904
add CycleCollectedJSContext::GetAsWorkletJSContext() and use it in IsOnWorkletThread() r=smaug
https://hg.mozilla.org/integration/autoland/rev/e90079d5f339
use nsContentUtils::GetCurrentJSContext() on all threads r=smaug
https://hg.mozilla.org/integration/autoland/rev/55823ab4098a
address modernize-use-override and modernize-use-equals-default r=baku
https://hg.mozilla.org/integration/autoland/rev/6c5c5d824c23
don't try to cycle collect after worklet cycle collector has been shut down r=baku
https://hg.mozilla.org/integration/autoland/rev/a81b45daf248
terminate worklet thread during xpcom shutdown r=baku
https://hg.mozilla.org/integration/autoland/rev/860e07a126c0
add CycleCollectedJSContext::IsSystemCaller() to make ThreadsafeIsSystemCaller() safe for worklets r=baku
No longer depends on: 1442776
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.