1328964 - Worklet should run in a separate thread.

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

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.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - Remove the window from worklet

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - WorkletThread

This patch works, but it crashes on shutdown.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 3 - Console API exposed to workers

Comment 4

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

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()?

Comment 5

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

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.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 2 - WorkletThread

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 3 - Console API exposed to worklets

Comment 8

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

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

Comment 9

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

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

Comment 10

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

If a worklet script hangs ( while(true); ), does it get killed?

Comment 11

[Andrea Marchesini [:baku]]

Attachment: part 2 - WorkletThread - second part

Comment 12

[Andrea Marchesini [:baku]]

(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 13

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

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)

Comment 14

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Assuming P3 due to lack of recent activity. Feel free to correct me.

Comment 15

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 16

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1328964 - part 1 - Remove the window from worklet

Review commit: https://reviewboard.mozilla.org/r/199422/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199422/

Comment 17

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1328964 - part 2 - WorkletThread

Review commit: https://reviewboard.mozilla.org/r/199424/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199424/

Comment 18

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1328964 - Add names to Runnables to make it compile.

Review commit: https://reviewboard.mozilla.org/r/199426/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199426/

Comment 19

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1328964 - Split WorkletJSContext into WorkletJSRuntime to catch up w/bug 1343396.

Review commit: https://reviewboard.mozilla.org/r/199428/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199428/

Comment 20

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1328964 - part 3 - Console API exposed to worklets

Review commit: https://reviewboard.mozilla.org/r/199430/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199430/

Comment 21

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1328964 - Name one more Runnable to make it compile.

Review commit: https://reviewboard.mozilla.org/r/199432/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199432/

Comment 22

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1328964 - part 4 - WorkletThread - second part

Review commit: https://reviewboard.mozilla.org/r/199434/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199434/

Comment 23

[Code Review Bot [:reviewbot]]

Comment on attachment 8928212 [details]
Bug 1328964 - part 1 - Remove the window from worklet

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


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

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


::: dom/worklet/AudioWorkletGlobalScope.cpp:14
(Diff revision 1)
>  #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 1)
>  #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 1)
>  #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 1)
>  #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/WorkletGlobalScope.cpp:38
(Diff revision 1)
>    NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
>    NS_INTERFACE_MAP_ENTRY(nsIGlobalObject)
>    NS_INTERFACE_MAP_ENTRY(WorkletGlobalScope)
>  NS_INTERFACE_MAP_END
>  
> -WorkletGlobalScope::WorkletGlobalScope(nsPIDOMWindowInner* aWindow)
> +WorkletGlobalScope::WorkletGlobalScope()

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

Comment 24

[Code Review Bot [:reviewbot]]

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 25

[Code Review Bot [:reviewbot]]

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 26

[Code Review Bot [:reviewbot]]

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]

Comment 27

[Karl Tomlinson (:karlt)]

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

Comment 28

[Karl Tomlinson (:karlt)]

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.)

Comment 29

[Karl Tomlinson (:karlt)]

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.

Comment 30

[Karl Tomlinson (:karlt)]

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.

Comment 31

[Karl Tomlinson (:karlt)]

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.

Comment 32

[Karl Tomlinson (:karlt)]

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()?

Comment 33

[Karl Tomlinson (:karlt)]

Attachment: Bug 1328964 - part 1 - Remove the window from worklet

Review commit: https://reviewboard.mozilla.org/r/227624/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/227624/

Comment 34

[Karl Tomlinson (:karlt)]

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 35

[Karl Tomlinson (:karlt)]

Attachment: Bug 1328964 - part 2 - WorkletThread

Review commit: https://reviewboard.mozilla.org/r/227634/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/227634/

Comment 36

[Karl Tomlinson (:karlt)]

Attachment: Bug 1328964 - part 2b - WorkletThread - second part

Review commit: https://reviewboard.mozilla.org/r/227636/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/227636/

Comment 37

[Karl Tomlinson (:karlt)]

Attachment: Bug 1328964 - part 3 - Console API exposed to worklets

Review commit: https://reviewboard.mozilla.org/r/227638/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/227638/

Comment 38

[Karl Tomlinson (:karlt)]

Comment on attachment 8958720 [details]
Bug 1328964 - part 1 - Remove the window from worklet

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227624/diff/1-2/

Comment 39

[Karl Tomlinson (:karlt)]

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 40

[Karl Tomlinson (:karlt)]

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.

Comment 41

[Karl Tomlinson (:karlt)]

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 42

[Karl Tomlinson (:karlt)]

Attachment: Bug 1328964 - Add names to Runnables to make it compile.

Review commit: https://reviewboard.mozilla.org/r/228830/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228830/

Comment 43

[Karl Tomlinson (:karlt)]

Attachment: Bug 1328964 - Split WorkletJSContext into WorkletJSRuntime to catch up w/bug 1343396.

Review commit: https://reviewboard.mozilla.org/r/228832/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228832/

Comment 44

[Karl Tomlinson (:karlt)]

Attachment: Bug 1328964 - Name one more Runnable to make it compile.

Review commit: https://reviewboard.mozilla.org/r/228834/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228834/

Comment 45

[Karl Tomlinson (:karlt)]

Comment on attachment 8958720 [details]
Bug 1328964 - part 1 - Remove the window from worklet

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227624/diff/2-3/

Comment 46

[Karl Tomlinson (:karlt)]

Comment on attachment 8958721 [details]
Bug 1328964 - part 2 - WorkletThread

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227634/diff/1-2/

Comment 47

[Karl Tomlinson (:karlt)]

Comment on attachment 8958722 [details]
Bug 1328964 - part 2b - WorkletThread - second part

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227636/diff/1-2/

Comment 48

[Karl Tomlinson (:karlt)]

Comment on attachment 8958723 [details]
Bug 1328964 - part 3 - Console API exposed to worklets

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227638/diff/1-2/

Comment 49

[Karl Tomlinson (:karlt)]

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 50

[Code Review Bot [:reviewbot]]

Comment on attachment 8958720 [details]
Bug 1328964 - part 1 - Remove the window from worklet

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


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 3)
>  #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 3)
>  #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 3)
>  #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 3)
>  #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]

Comment 51

[Code Review Bot [:reviewbot]]

Comment on attachment 8960061 [details]
Bug 1328964 - Split WorkletJSContext into WorkletJSRuntime to catch up w/bug 1343396.

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


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 52

[Code Review Bot [:reviewbot]]

Comment on attachment 8958723 [details]
Bug 1328964 - part 3 - Console API exposed to worklets

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


Code analysis found 2 defects in this patch:
 - 2 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/console/Console.cpp:635
(Diff revision 2)
> +    WorkletThread::AssertIsOnWorkletThread();
> +    MOZ_ASSERT(aCallData);
> +    aCallData->AssertIsOnOwningThread();
> +  }
> +
> +  ~ConsoleCallDataWorkletRunnable()

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

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

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

Comment 53

[Karl Tomlinson (:karlt)]

Attachment: bug 1328964 restore removal of declaration for non-existant Worklet::GetOrCreateGlobalScope()

Lost in rebase of "part 2 - WorkletThread"

Addresses https://reviewboard.mozilla.org/r/199424/#comment296308

Review commit: https://reviewboard.mozilla.org/r/228840/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228840/

Comment 54

[Karl Tomlinson (:karlt)]

Attachment: bug 1328964 restore fix for stack use after scope bug 1415086

Lost in rebase of "part 2 - WorkletThread" across
https://hg.mozilla.org/mozilla-central/rev/90a7bc300af3

Addresses https://reviewboard.mozilla.org/r/199424/#comment296306

Review commit: https://reviewboard.mozilla.org/r/228842/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228842/

Comment 55

[Karl Tomlinson (:karlt)]

Attachment: bug 1328964 restore fix for crash in runnable after thread shutdown bug 1315446

Lost in rebase of "part 2 - Worklet Thread - second part" across
https://hg.mozilla.org/mozilla-central/rev/df94a2bfc2436b00a776c9083cc3ae4dda01a1fe#l1.12

Addresses https://reviewboard.mozilla.org/r/199434/#comment296312

Review commit: https://reviewboard.mozilla.org/r/228844/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228844/

Comment 56

[Karl Tomlinson (:karlt)]

Attachment: bug 1328964 restore null check for bug 1357503

Lost in rebase of part 3 across https://hg.mozilla.org/mozilla-central/rev/bbd8e6ecf4a0#l1.12

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

Review commit: https://reviewboard.mozilla.org/r/228846/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228846/

Comment 57

[Karl Tomlinson (:karlt)]

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 58

[Code Review Bot [:reviewbot]]

Comment on attachment 8958720 [details]
Bug 1328964 - part 1 - Remove the window from worklet

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


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 3)
>  #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 3)
>  #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 3)
>  #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 3)
>  #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]

Comment 59

[Code Review Bot [:reviewbot]]

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 60

[Code Review Bot [:reviewbot]]

Comment on attachment 8958723 [details]
Bug 1328964 - part 3 - Console API exposed to worklets

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


Code analysis found 2 defects in this patch:
 - 2 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/console/Console.cpp:635
(Diff revision 2)
> +    WorkletThread::AssertIsOnWorkletThread();
> +    MOZ_ASSERT(aCallData);
> +    aCallData->AssertIsOnOwningThread();
> +  }
> +
> +  ~ConsoleCallDataWorkletRunnable()

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

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

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

Comment 61

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8960067 [details]
bug 1328964 restore removal of declaration for non-existant Worklet::GetOrCreateGlobalScope()

https://reviewboard.mozilla.org/r/228840/#review234602

Comment 62

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8960068 [details]
bug 1328964 restore fix for stack use after scope bug 1415086

https://reviewboard.mozilla.org/r/228842/#review234604

Comment 63

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 64

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8960070 [details]
bug 1328964 restore null check for bug 1357503

https://reviewboard.mozilla.org/r/228846/#review234614

Comment 65

[Karl Tomlinson (:karlt)]

Comment on attachment 8958720 [details]
Bug 1328964 - part 1 - Remove the window from worklet

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227624/diff/3-4/

Comment 66

[Karl Tomlinson (:karlt)]

Comment on attachment 8958721 [details]
Bug 1328964 - part 2 - WorkletThread

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227634/diff/2-3/

Comment 67

[Karl Tomlinson (:karlt)]

Comment on attachment 8958722 [details]
Bug 1328964 - part 2b - WorkletThread - second part

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227636/diff/2-3/

Comment 68

[Karl Tomlinson (:karlt)]

Comment on attachment 8958723 [details]
Bug 1328964 - part 3 - Console API exposed to worklets

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227638/diff/2-3/

Comment 69

[Karl Tomlinson (:karlt)]

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 70

[Karl Tomlinson (:karlt)]

Attachment: bug 1328964 changes for worletthread rebase for microtask bug 1343396

For folding into "part 2 - WorkletThread".

Review commit: https://reviewboard.mozilla.org/r/235154/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235154/

Comment 71

[Karl Tomlinson (:karlt)]

Comment on attachment 8958720 [details]
Bug 1328964 - part 1 - Remove the window from worklet

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227624/diff/4-5/

Comment 72

[Karl Tomlinson (:karlt)]

Comment on attachment 8958721 [details]
Bug 1328964 - part 2 - WorkletThread

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227634/diff/3-4/

Comment 73

[Karl Tomlinson (:karlt)]

Comment on attachment 8958722 [details]
Bug 1328964 - part 2b - WorkletThread - second part

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227636/diff/3-4/

Comment 74

[Karl Tomlinson (:karlt)]

Comment on attachment 8958723 [details]
Bug 1328964 - part 3 - Console API exposed to worklets

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227638/diff/3-4/

Comment 75

[Karl Tomlinson (:karlt)]

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.

Comment 76

[Code Review Bot [:reviewbot]]

Comment on attachment 8958720 [details]
Bug 1328964 - part 1 - Remove the window from worklet

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


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 4)
>  #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 4)
>  #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 4)
>  #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 4)
>  #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]

Comment 77

[Code Review Bot [:reviewbot]]

Comment on attachment 8958720 [details]
Bug 1328964 - part 1 - Remove the window from worklet

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


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 5)
>  #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 5)
>  #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 5)
>  #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 5)
>  #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]

Comment 78

[Karl Tomlinson (:karlt)]

(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 79

[Code Review Bot [:reviewbot]]

Comment on attachment 8958723 [details]
Bug 1328964 - part 3 - Console API exposed to worklets

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


Code analysis found 2 defects in this patch:
 - 2 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/console/Console.cpp:639
(Diff revision 3)
> +    WorkletThread::AssertIsOnWorkletThread();
> +    MOZ_ASSERT(aCallData);
> +    aCallData->AssertIsOnOwningThread();
> +  }
> +
> +  ~ConsoleCallDataWorkletRunnable()

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

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

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

Comment 80

[Code Review Bot [:reviewbot]]

Comment on attachment 8958723 [details]
Bug 1328964 - part 3 - Console API exposed to worklets

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


Code analysis found 2 defects in this patch:
 - 2 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/console/Console.cpp:626
(Diff revision 4)
> +
> +    // Marking this CallData as in use.
> +    mCallData->mStatus = ConsoleCallData::eInUse;
> +  }
> +
> +  ~ConsoleCallDataWorkletRunnable()

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

::: dom/console/Console.cpp:815
(Diff revision 4)
>      // Marking this CallData as in use.
>      mCallData->mStatus = ConsoleCallData::eInUse;
>    }
>  
>  private:
> -  ~ConsoleCallDataRunnable()
> +  ~ConsoleCallDataWorkerRunnable()

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

Comment 81

[Karl Tomlinson (:karlt)]

Comment on attachment 8958722 [details]
Bug 1328964 - part 2b - WorkletThread - second part

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227636/diff/4-5/

Comment 82

[Karl Tomlinson (:karlt)]

Comment on attachment 8958723 [details]
Bug 1328964 - part 3 - Console API exposed to worklets

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227638/diff/4-5/

Comment 83

[Karl Tomlinson (:karlt)]

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.

Comment 85

[Karl Tomlinson (:karlt)]

Attachment: bug 1328964 use = default in WorkletGlobalScope and derived constructors

Addresses modernize-use-equals-default clang-tidy suggestions
in "part 1 - Remove the window from worklet".

Review commit: https://reviewboard.mozilla.org/r/235170/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235170/

Comment 86

[Karl Tomlinson (:karlt)]

Attachment: bug 1328964 add override to ~WorkletJSContext/Runtime and use = default

Addresses modernize-use-override and modernize-use-equals-default
clang-tidy suggestions in "part 2 - Worklet Thread".

Review commit: https://reviewboard.mozilla.org/r/235172/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235172/

Comment 87

[Karl Tomlinson (:karlt)]

Attachment: bug 1328964 don't allocate SourceBufferHolder on the heap

I'm not aware of any reason why SourceBufferHolder must be allocated only on
the stack, but bz and bkelly seemed to agree that it should be MOZ_STACK_CLASS
in https://bugzilla.mozilla.org/show_bug.cgi?id=987556#c72

FWIW avoiding SourceBufferHolder saves by not needing to store GiveOwnership.

UniquePtr makes the transfer of ownership clearer.

Review commit: https://reviewboard.mozilla.org/r/235174/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235174/

Comment 88

[Karl Tomlinson (:karlt)]

Attachment: bug 1328964 add CycleCollectedJSContext::GetAsWorkletJSContext() and use it in IsOnWorkletThread()

Review commit: https://reviewboard.mozilla.org/r/235176/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235176/

Comment 89

[Karl Tomlinson (:karlt)]

Attachment: bug 1328964 use nsContentUtils::GetCurrentJSContext() on all threads

nsContentUtils::GetCurrentJSContextForThread() originally needed to use
workers::GetCurrentThreadJSContext() in
https://hg.mozilla.org/mozilla-central/rev/366a45b41539#l2.18
because CycleCollectedJS*()->Context() did not exist until
https://hg.mozilla.org/mozilla-central/rev/1a0cd542e1e9#l7.12

nsContentUtils::GetCurrentJSContext() is thread-safe since these changes:
https://hg.mozilla.org/mozilla-central/rev/e636c7186286#l3.12
https://hg.mozilla.org/mozilla-central/rev/c8029c072776#l2.12

Review commit: https://reviewboard.mozilla.org/r/235178/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235178/

Comment 90

[Karl Tomlinson (:karlt)]

Attachment: bug 1328964 address modernize-use-override and modernize-use-equals-default

clang-tidy suggestions in "part 3 - Console API exposed to worklets".

Review commit: https://reviewboard.mozilla.org/r/235180/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235180/

Comment 91

[Karl Tomlinson (:karlt)]

Attachment: bug 1328964 don't try to cycle collect after worklet cycle collector has been shut down

Review commit: https://reviewboard.mozilla.org/r/235182/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235182/

Comment 92

[Karl Tomlinson (:karlt)]

Attachment: bug 1328964 terminate worklet thread during xpcom shutdown

The final CC does not happen until after threads are shutdown and so we can't
depend on CC to trigger termination.

This management of the worklet thread by worklet code is an intermediate
situation until worklets run on the threads managed by other objects.

Review commit: https://reviewboard.mozilla.org/r/235184/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235184/

Comment 93

[Karl Tomlinson (:karlt)]

Attachment: bug 1328964 add CycleCollectedJSContext::IsSystemCaller() to make ThreadsafeIsSystemCaller() safe for worklets

Review commit: https://reviewboard.mozilla.org/r/235186/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235186/

Comment 94

[Karl Tomlinson (:karlt)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fedf433e69cc947fe87863fa09e7e17def0ee35
has the same patches as push 38a702c8b5da, but in a different order.

Comment 96

[Andrea Marchesini [:baku]]

Comment on attachment 8958720 [details]
Bug 1328964 - part 1 - Remove the window from worklet

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

Comment 97

[Andrea Marchesini [:baku]]

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

Comment 98

[Andrea Marchesini [:baku]]

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.

Comment 99

[Andrea Marchesini [:baku]]

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.

Comment 100

[Andrea Marchesini [:baku]]

Comment on attachment 8966467 [details]
bug 1328964 changes for worletthread rebase for microtask bug 1343396

https://reviewboard.mozilla.org/r/235154/#review240938

Comment 101

[Andrea Marchesini [:baku]]

Comment on attachment 8966479 [details]
bug 1328964 use = default in WorkletGlobalScope and derived constructors

https://reviewboard.mozilla.org/r/235170/#review240940

Comment 102

[Andrea Marchesini [:baku]]

Comment on attachment 8966480 [details]
bug 1328964 add override to ~WorkletJSContext/Runtime and use = default

https://reviewboard.mozilla.org/r/235172/#review240942

Comment 103

[Andrea Marchesini [:baku]]

Comment on attachment 8966481 [details]
bug 1328964 don't allocate SourceBufferHolder on the heap

https://reviewboard.mozilla.org/r/235174/#review240950

Comment 104

[Andrea Marchesini [:baku]]

Comment on attachment 8966484 [details]
bug 1328964 address modernize-use-override and modernize-use-equals-default

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

Comment 105

[Andrea Marchesini [:baku]]

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

Comment 106

[Andrea Marchesini [:baku]]

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.

Comment 107

[Andrea Marchesini [:baku]]

Comment on attachment 8966487 [details]
bug 1328964 add CycleCollectedJSContext::IsSystemCaller() to make ThreadsafeIsSystemCaller() safe for worklets

https://reviewboard.mozilla.org/r/235186/#review240964

Comment 108

[Andrea Marchesini [:baku]]

btw, thanks for finishing this!

Comment 109

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

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.

Comment 110

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

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.

Comment 111

[Karl Tomlinson (:karlt)]

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 112

[Karl Tomlinson (:karlt)]

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.

Comment 113

[Karl Tomlinson (:karlt)]

Comment on attachment 8958720 [details]
Bug 1328964 - part 1 - Remove the window from worklet

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227624/diff/5-6/

Comment 114

[Karl Tomlinson (:karlt)]

Comment on attachment 8958721 [details]
Bug 1328964 - part 2 - WorkletThread

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227634/diff/4-5/

Comment 115

[Karl Tomlinson (:karlt)]

Comment on attachment 8958723 [details]
Bug 1328964 - part 3 - Console API exposed to worklets

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227638/diff/5-6/

Comment 116

[Karl Tomlinson (:karlt)]

Comment on attachment 8966479 [details]
bug 1328964 use = default in WorkletGlobalScope and derived constructors

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235170/diff/1-2/

Comment 117

[Karl Tomlinson (:karlt)]

Comment on attachment 8966480 [details]
bug 1328964 add override to ~WorkletJSContext/Runtime and use = default

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235172/diff/1-2/

Comment 118

[Karl Tomlinson (:karlt)]

Comment on attachment 8966481 [details]
bug 1328964 don't allocate SourceBufferHolder on the heap

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235174/diff/1-2/

Comment 119

[Karl Tomlinson (:karlt)]

Comment on attachment 8966482 [details]
bug 1328964 add CycleCollectedJSContext::GetAsWorkletJSContext() and use it in IsOnWorkletThread()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235176/diff/1-2/

Comment 120

[Karl Tomlinson (:karlt)]

Comment on attachment 8966483 [details]
bug 1328964 use nsContentUtils::GetCurrentJSContext() on all threads

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235178/diff/1-2/

Comment 121

[Karl Tomlinson (:karlt)]

Comment on attachment 8966484 [details]
bug 1328964 address modernize-use-override and modernize-use-equals-default

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235180/diff/1-2/

Comment 122

[Karl Tomlinson (:karlt)]

Comment on attachment 8966485 [details]
bug 1328964 don't try to cycle collect after worklet cycle collector has been shut down

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235182/diff/1-2/

Comment 123

[Karl Tomlinson (:karlt)]

Comment on attachment 8966486 [details]
bug 1328964 terminate worklet thread during xpcom shutdown

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235184/diff/1-2/

Comment 124

[Karl Tomlinson (:karlt)]

Comment on attachment 8966487 [details]
bug 1328964 add CycleCollectedJSContext::IsSystemCaller() to make ThreadsafeIsSystemCaller() safe for worklets

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235186/diff/1-2/

Comment 125

[Code Review Bot [:reviewbot]]

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]

Comment 128

[Pulsebot]

Pushed by [e-mail]:
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

Comment 129

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b76bff2ddfc6
https://hg.mozilla.org/mozilla-central/rev/16197254fa36
https://hg.mozilla.org/mozilla-central/rev/bdd4af79d6f7
https://hg.mozilla.org/mozilla-central/rev/608bd57e774e
https://hg.mozilla.org/mozilla-central/rev/bb29485e179b
https://hg.mozilla.org/mozilla-central/rev/c3c9f4525efb
https://hg.mozilla.org/mozilla-central/rev/06872fbe3904
https://hg.mozilla.org/mozilla-central/rev/e90079d5f339
https://hg.mozilla.org/mozilla-central/rev/55823ab4098a
https://hg.mozilla.org/mozilla-central/rev/6c5c5d824c23
https://hg.mozilla.org/mozilla-central/rev/a81b45daf248
https://hg.mozilla.org/mozilla-central/rev/860e07a126c0