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

RESOLVED FIXED in Firefox 32

Status

()

defect
RESOLVED FIXED
5 years ago
2 months ago

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

(Blocks 1 bug)

unspecified
mozilla33
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox31 wontfix, firefox32 fixed, firefox33 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

No description provided.
Posted patch Exposing shutdown barriers (obsolete) — Splinter Review
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.
Assignee: nobody → dteller
Attachment #8441407 - Flags: review?(nfroyd)
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?
Attachment #8441407 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/integration/fx-team/rev/ed2985b53467
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/ed2985b53467
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla33
Blocks: 1034975
No longer depends on: 1034975
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.
Attachment #8441696 - Flags: approval-mozilla-beta?
Attachment #8441696 - Flags: approval-mozilla-aurora?
Comment on attachment 8441696 [details] [diff] [review]
Exposing shutdown barriers, v2

Test + fix a shutdown crash. Taking it for beta 8.
Attachment #8441696 - Flags: approval-mozilla-beta?
Attachment #8441696 - Flags: approval-mozilla-beta+
Attachment #8441696 - Flags: approval-mozilla-aurora?
Attachment #8441696 - Flags: approval-mozilla-aurora+
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.
(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.
We will need uplift once patch 1 of bug 985655 has been uplifted.
Attachment #8441696 - Flags: approval-mozilla-beta+

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?

Flags: needinfo?(ryanvm)
You need to log in before you can comment on or make changes to this bug.