1455210 - misleading use of CycleCollectedJSContext::Get() in WorkerPrivate

Status: RESOLVED FIXED

(Core :: DOM: Workers, enhancement, P3)

Description

[Karl Tomlinson (:karlt)]

This is just code clean-up.  The behavior will not be changed.

https://hg.mozilla.org/mozilla-central/diff/068c59c7c4ec/dom/workers/WorkerPrivate.cpp
added a few CycleCollectedJSContext::Get() calls, most, but not all, with a
null check.  The null checks on the CycleCollectedJSContext pointer are
misleading because they imply that it may be null.

Some of these null checks were requested for no reason in review. e.g.
https://bugzilla.mozilla.org/show_bug.cgi?id=1193394#c50

Similarly, fetching the CycleCollectedJSContext multiple times in one function
is misleading as it implies the result may change.

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: bug 1455210 remove misleading CycleCollectedJSContext null checks

The caller of WorkerPrivate::DoRunLoop() keeps the WorkerJSContext alive, and
so there will always be a CycleCollectedJSContext in DoRunLoop.

WorkerPrivate::EnterDebuggerEventLoop() assumes that
WorkerPrivate::GetJSContext() returns a JSContext, and so
EnterDebuggerEventLoop also must only be called while DoRunLoop is on the
stack and therefore keeping the WorkerJSContext alive.

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

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: bug 1455210 call CycleCollectedJSContext::Get() only once in EnterDebuggerEventLoop()

The WorkerJSContext is created and destroyed after entry and before exit from
WorkerThreadPrimaryRunnable::Run().  WorkerPrivate::EnterDebuggerEventLoop()
is called only while WorkerThreadPrimaryRunnable::Run is on the stack, and so
the CycleCollectedJSContext will not change.

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

Comment 3

[Andrea Marchesini [:baku]]

Comment on attachment 8969161 [details]
bug 1455210 remove misleading CycleCollectedJSContext null checks

https://reviewboard.mozilla.org/r/237892/#review243620

Comment 4

[Andrea Marchesini [:baku]]

Comment on attachment 8969162 [details]
bug 1455210 call CycleCollectedJSContext::Get() only once in EnterDebuggerEventLoop()

https://reviewboard.mozilla.org/r/237894/#review243622

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7c7be4064df1
remove misleading CycleCollectedJSContext null checks r=baku
https://hg.mozilla.org/integration/autoland/rev/eb34acceb9dc
call CycleCollectedJSContext::Get() only once in EnterDebuggerEventLoop() r=baku

Comment 6

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/7c7be4064df1
https://hg.mozilla.org/mozilla-central/rev/eb34acceb9dc