1996263 - LlamaStreamSource Worker Lifecycle needs improvement

Status: RESOLVED FIXED

(Core :: Machine Learning: On Device, defect)

Description

[Matthew Gaudet (he/him) [:mgaudet]]

Opening a bug to make sure it gets tracked. See Bug 1973711 Comment 15:

It seems like there may missing support for properly cleaning up on workers which can result in crashes on worker shutdown; I made a brief comment on https://phabricator.services.mozilla.com/D254808#9316979 about this because of :mgaudet's observed https://crash-stats.mozilla.org/report/index/3c1d6468-493c-4072-978b-b9b710251020.

CC'ing both asuth and smaug who flagged the issues here.

Comment 1

[BugBot [:suhaib / :marco]]

The severity field is not set for this bug.
:tarek, could you have a look please?

For more information, please visit BugBot documentation.

Comment 3

[Matthew Gaudet (he/him) [:mgaudet]]

(redirecting the Aristide ni from Bug 2001950)

Comment 4

[David Parks [:handyman]]

Just got this crash. It looks like the SpinEventLoopUntil during garbage collection is the culprit -- that does sound dangerous. Without knowing the code, if the shutdown doesn't need to be synchronous, nsIThread::AsyncShutdown wouldn't call SELU.

Comment 6

[BugBot [:suhaib / :marco]]

The bug is linked to a topcrash signature, which matches the following criterion:

• Top 10 desktop browser crashes on nightly

For more information, please visit BugBot documentation.

Comment 7

[Paul Adenot (:padenot)]

https://treeherder.mozilla.org/jobs?repo=try&landoCommitID=172399

Comment 8

[Paul Adenot (:padenot)]

Attachment: Bug 1996263 - Rework shutdown of LlamaStreamSource in Worker. r?asuth

This is inspired from BodyConsumer.

Comment 9

[Paul Adenot (:padenot)]

CI is busted because of bug 2010162, I'll see if I land this now or later.

Comment 10

[Paul Adenot (:padenot)]

I am told by jcristau to land this, because we anticipate that the CI is going to be broken for some time, it isn't tier-1.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/2f3d39a61c42
https://hg.mozilla.org/integration/autoland/rev/2fc2c2131e51
Rework shutdown of LlamaStreamSource in Worker. r=asuth,ai-ondevice-reviewers

Comment 12

[agoloman]

https://hg.mozilla.org/mozilla-central/rev/2fc2c2131e51

Comment 13

[BugBot [:suhaib / :marco]]

The patch landed in nightly and beta is affected.
:padenot, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox148 to wontfix.

For more information, please visit BugBot documentation.

Comment 14

[Donal Meehan [:dmeehan]]

:padenot, re: https://bugzilla.mozilla.org/show_bug.cgi?id=2018023#c5 do you want to add a release uplift request here?
We could take it in a later Fx148 dot release.

Comment 15

[Paul Adenot (:padenot)]

Attachment: Bug 1996263 - Rework shutdown of LlamaStreamSource in Worker.

This is inspired from BodyConsumer.

Original Revision: https://phabricator.services.mozilla.com/D278645

Comment 16

[Phabricator Automation]

firefox-release Uplift Approval Request

• User impact if declined: Top crash. This was crashing in CI and crashes are gone, and is effectively tested.
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: This has baked on beta and nightly for some time.
• String changes made/needed: none
• Is Android affected?: no

Comment 18

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/4663f6e62d5a
https://hg.mozilla.org/releases/mozilla-release/rev/0f3f0595b582

Comment 20

[BugBot [:suhaib / :marco]]

Copying crash signatures from duplicate bugs.