Closed Bug 1492011 Opened 2 years ago Closed 4 months ago

ConsoleWorkletRunnable thread shutdown incompatibility

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla76
Tracking Status
firefox-esr68 --- fixed
firefox64 --- wontfix
firefox76 --- fixed

People

(Reporter: karlt, Assigned: karlt)

References

(Blocks 1 open bug)

Details

Attachments

(15 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
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.
Component: DOM → DOM: Core & HTML
Blocks: 1616723
Blocks: 1581896
Assignee: nobody → karlt
Status: NEW → ASSIGNED

(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

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

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

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

Depends on D67989

Depends on D67990

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

Depends on D67993

to remove Console instance access from PopulateConsoleNotificationInTheTargetScope().

Depends on D67997

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

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

Pushed by ktomlinson@mozilla.com:
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

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 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
Attachment #9141920 - Flags: approval-mozilla-esr68?

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

Fixes stability issues. Approved for 68.8esr.

Attachment #9141920 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
You need to log in before you can comment on or make changes to this bug.