1492011 - ConsoleWorkletRunnable thread shutdown incompatibility

Status: RESOLVED FIXED

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

Description

[Karl Tomlinson (:karlt)]

ConsoleWorkletRunnable queues a runnable from main to the worklet thread
to release its reference to Console et al.

If WorkletThread::Terminate has been called (when observing shutdown, for
example), then there may be no worklet thread to execute the runnable.

I guess we need to either ensure the thread is not terminated while there may
be a console runnable, or redesign ConsoleWorkletRunnable so that it doesn't
need to release the reference to any worklet-thread objects.

Comment 1

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #0)

redesign ConsoleWorkletRunnable so that it doesn't
need to release the reference to any worklet-thread objects.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=561c531dae60e398392874284e299111dac86506

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: Bug 1492011 consolidate StoreProfileData and StoreConsoleData into a single method with arguments parameter r?baku

This removes a dependency on JS objects on ConsoleCallData, and a reference to
the arguments on ConsoleProfileWorkerRunnable.

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: Bug 1492011 store raw JS arguments on Console separately from ConsoleCallData r?baku

so that all ConsoleCallData members can be destroyed on either thread.

ArgumentData::mArguments hold the same references that
ConsoleCallData::mCopiedArguments held previously. The name change is because
the references are merely stored rather than any deep copy of objects.

Depends on D67988

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: Bug 1492011 Remove Console::mCallDataStoragePending r?baku,baku

Now that ConsoleCallData has nothing to trace, mCallDataStoragePending has no
purpose.

Depends on D67989

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: Bug 1492011 Remove now-unused Console::mStatus r?baku

Depends on D67990

Comment 6

[Karl Tomlinson (:karlt)]

Attachment: Bug 1492011 change ShouldIncludeStackTrace from instance to class method r?baku

Depends on D67991

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: Bug 1492011 replace ArgumentsToValueList instance method with nsTArray::AppendElements() r?baku

Depends on D67992

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: Bug 1492011 provide group stack parameter to PopulateConsoleNotificationInTheTargetScope() r?baku

This will support removal of Console class usage from main thread.

Depends on D67993

Comment 9

[Karl Tomlinson (:karlt)]

Attachment: Bug 1492011 change ProcessArguments from instance method to nonmember function with internal linkage r?baku

Depends on D67994

Comment 10

[Karl Tomlinson (:karlt)]

Attachment: Bug 1492011 change CreateCounterOrResetCounterValue from instance method to nonmember function with internal linkage r?baku

Depends on D67995

Comment 11

[Karl Tomlinson (:karlt)]

Attachment: Bug 1492011 change CreateStartTimerValue and CreateLogOrEndTimerValue from instance to class method r?baku

Depends on D67996

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: Bug 1492011 provide ID and Prefix on ConsoleCallData r?baku

to remove Console instance access from PopulateConsoleNotificationInTheTargetScope().

Depends on D67997

Comment 13

[Karl Tomlinson (:karlt)]

Attachment: Bug 1492011 introduce a separate class to hold main-thread data associated with each Console r?baku

This provides that ConsoleRunnable no longer has a reference to Console, which
previously needed to be released through a message to the console thread.

Depends on D67998

Comment 14

[Karl Tomlinson (:karlt)]

Attachment: Bug 1492011 make ConsoleCallData::mRefCnt thread-safe r?baku

so that references can be released from the main thread and there is no need
to send a message to Console thread.

Depends on D67999

Comment 15

[Karl Tomlinson (:karlt)]

Attachment: Bug 1492011 assert that ConsoleStructuredCloneData::mGlobal is cleared before ConsoleRunnable destruction r?baku

Depends on D68000

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/97e12e488248
consolidate StoreProfileData and StoreConsoleData into a single method with arguments parameter r=baku
https://hg.mozilla.org/integration/autoland/rev/8c0bf4c5b259
store raw JS arguments on Console separately from ConsoleCallData r=baku
https://hg.mozilla.org/integration/autoland/rev/b422b439d748
Remove Console::mCallDataStoragePending r=baku
https://hg.mozilla.org/integration/autoland/rev/2ffea3b36683
Remove now-unused Console::mStatus r=baku
https://hg.mozilla.org/integration/autoland/rev/25d1b2fe703d
change ShouldIncludeStackTrace from instance to class method r=baku
https://hg.mozilla.org/integration/autoland/rev/87aac4032d22
replace ArgumentsToValueList instance method with nsTArray::AppendElements() r=baku
https://hg.mozilla.org/integration/autoland/rev/bdbad2a40ec2
provide group stack parameter to PopulateConsoleNotificationInTheTargetScope() r=baku
https://hg.mozilla.org/integration/autoland/rev/84ab47891f80
change ProcessArguments from instance method to nonmember function with internal linkage r=baku
https://hg.mozilla.org/integration/autoland/rev/6427a6469f45
change CreateCounterOrResetCounterValue from instance method to nonmember function with internal linkage r=baku
https://hg.mozilla.org/integration/autoland/rev/7f6291f3e391
change CreateStartTimerValue and CreateLogOrEndTimerValue from instance to class method r=baku
https://hg.mozilla.org/integration/autoland/rev/6c87d6b52440
provide ID and Prefix on ConsoleCallData r=baku
https://hg.mozilla.org/integration/autoland/rev/59bebd739346
introduce a separate class to hold main-thread data associated with each Console r=baku
https://hg.mozilla.org/integration/autoland/rev/643c68ed31dd
make ConsoleCallData::mRefCnt thread-safe r=baku
https://hg.mozilla.org/integration/autoland/rev/4b2314fe45c7
assert that ConsoleStructuredCloneData::mGlobal is cleared before ConsoleRunnable destruction r=baku

Comment 17

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/97e12e488248
https://hg.mozilla.org/mozilla-central/rev/8c0bf4c5b259
https://hg.mozilla.org/mozilla-central/rev/b422b439d748
https://hg.mozilla.org/mozilla-central/rev/2ffea3b36683
https://hg.mozilla.org/mozilla-central/rev/25d1b2fe703d
https://hg.mozilla.org/mozilla-central/rev/87aac4032d22
https://hg.mozilla.org/mozilla-central/rev/bdbad2a40ec2
https://hg.mozilla.org/mozilla-central/rev/84ab47891f80
https://hg.mozilla.org/mozilla-central/rev/6427a6469f45
https://hg.mozilla.org/mozilla-central/rev/7f6291f3e391
https://hg.mozilla.org/mozilla-central/rev/6c87d6b52440
https://hg.mozilla.org/mozilla-central/rev/59bebd739346
https://hg.mozilla.org/mozilla-central/rev/643c68ed31dd
https://hg.mozilla.org/mozilla-central/rev/4b2314fe45c7

Comment 18

[Karl Tomlinson (:karlt)]

Attachment: Bug 1492011 uplift changes 97e12e488248..59bebd739346 to ESR68

consolidate StoreProfileData and StoreConsoleData into a single method with arguments parameter r=baku
store raw JS arguments on Console separately from ConsoleCallData r=baku
Remove Console::mCallDataStoragePending r=baku
Remove now-unused Console::mStatus r=baku
change ShouldIncludeStackTrace from instance to class method r=baku
replace ArgumentsToValueList instance method with nsTArray::AppendElements() r=baku
provide group stack parameter to PopulateConsoleNotificationInTheTargetScope() r=baku
change ProcessArguments from instance method to nonmember function with internal linkage r=baku
change CreateCounterOrResetCounterValue from instance method to nonmember function with internal linkage r=baku
change CreateStartTimerValue and CreateLogOrEndTimerValue from instance to class method r=baku
provide ID and Prefix on ConsoleCallData r=baku
introduce a separate class to hold main-thread data associated with each Console r=baku

Mechanical conflict resolution was required to address conflicts with these changes not on esr68:
https://hg.mozilla.org/mozilla-central/rev/11cda2e9615b
https://hg.mozilla.org/mozilla-central/rev/9919570a9843
https://hg.mozilla.org/mozilla-central/rev/c9c7f800aae6
https://hg.mozilla.org/mozilla-central/diff/30100f2c612832dc74831e4b1b36bec636d5b396/dom/console/Console.cpp

Comment 19

[Karl Tomlinson (:karlt)]

Comment on attachment 9141920 [details]
Bug 1492011 uplift changes 97e12e488248..59bebd739346 to ESR68

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: stability
• User impact if declined:
• Fix Landed on Version: 76
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): There is some risk due to the size of the patch, but it has had significant time on Beta. The ESR68 code is not substantially different in behavior.
• String or UUID changes made by this patch: None

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9141920 [details]
Bug 1492011 uplift changes 97e12e488248..59bebd739346 to ESR68

Fixes stability issues. Approved for 68.8esr.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/e388563552d6