1008944 - [OS.File] Expose a AsyncShutdown Barrier for OS.File shutdown

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Attachment: Exposing shutdown barriers

Here we go.
I also took the opportunity to:
- fix a trivial bug that made one of the fields of the AsyncShutdown state be useless (we forgot to increment the number of messages sent);
- get rid of some code designed only for testing, as the feature gives us a less invasive way of performing the same test;
- using one of the barriers in the CrashMonitor.

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8441407 [details] [diff] [review]
Exposing shutdown barriers

Review of attachment 8441407 [details] [diff] [review]:
-----------------------------------------------------------------

WFM with the bit below addressed.

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +600,5 @@
> +
> +    // Wait until all requests are complete and kill the worker.
> +    yield Scheduler.kill({reset: false, shutdown: true});
> +  }),
> +  function getDetails() {

Can we factor this out into a real function rather than copy-pasting it from profileBeforeChange's blocker?

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Attachment: Exposing shutdown barriers, v2

Applied feedback.
Try: https://tbpl.mozilla.org/?tree=Try&rev=6fa41d5de687

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/ed2985b53467

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ed2985b53467

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8441696 [details] [diff] [review]
Exposing shutdown barriers, v2

Approval Request Comment
[Feature/regressing bug #]: bug 888373
[User impact if declined]: shutdown timeout/crashes in CrashMonitor
[Describe test coverage new/current, TBPL]: This has been on m-c for ~2 weeks. According to Socorro, CrashMonitor crashes have disappeared.
[Risks and why]: None that I can think of.
[String/UUID change made/needed]: None.

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8441696 [details] [diff] [review]
Exposing shutdown barriers, v2

Test + fix a shutdown crash. Taking it for beta 8.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6a25989f0c6c
https://hg.mozilla.org/releases/mozilla-beta/rev/c63081c1f575

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Backed out from beta for suspicion of being the cause of mass-orange. Is it at all relevant that bug 985655 only landed on 32+?
https://hg.mozilla.org/releases/mozilla-beta/rev/17033248da97

The common thread in all the logs was the OS.File error:
https://tbpl.mozilla.org/php/getParsedLog.php?id=43268235&tree=Mozilla-Beta

11:59:09     INFO -  System JS : ERROR resource://gre/modules/osfile/osfile_async_front.jsm:1304 - TypeError: can't redefine non-configurable property 'creationDate'

AFAICT, that error is new starting with this push.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #9)
> Backed out from beta for suspicion of being the cause of mass-orange. Is it
> at all relevant that bug 985655 only landed on 32+?
> https://hg.mozilla.org/releases/mozilla-beta/rev/17033248da97

Oh. You are absolutely right. We would need to uplift the other one, too.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

We will need uplift once patch 1 of bug 985655 has been uplifted.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Ryan, I just received an e-mail informing me that you just reverted this patch.

https://phabricator.services.mozilla.com/rMOZILLACENTRALc63081c1f575d8ca4d93f960b0f0cfcb3d2bd18b

What is going on?

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Almost certainly fallout from https://groups.google.com/d/msg/mozilla.dev.platform/U1p13zL9EeY/rL-GX7NVAgAJ