Closed Bug 1630655 Opened 4 months ago Closed 3 months ago

toolkit.shutdown.lateWriteChecksStage is not properly set on Nightly

Categories

(Core :: XPCOM, defect)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: dthayer, Assigned: dthayer)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [fxperf:p1])

Attachments

(4 files)

This should be set to 3 right now - instead it is 1. We will have to roll back fastShutdownStage back to 1 in order to properly collect the data we may have been missing from this. This is due to a misunderstanding on my part on how the preprocessor used by generate_static_pref_list.py works.

The preprocessor does not handle "# ifdef" like the C preprocessor,
and I failed to test this appropriately after restructuring the
#if... clauses. This also reverts fastShutdownStage back to 1, so
we can be confident in the data we collect.

Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f055b35804d7
Actually advance lateWriteChecksStage to 3 r=erahm

Hey njn, I'm wondering if you can provide some insight here. The bottom failure linked in the comment above occurs in PWRunnable. It's performing a late write, sometime during or after xpcom-shutdown. As far as I can tell, this is intended to not be possible, with the profile-before-change-telemetry observer in Preferences.cpp trying to ensure all writes have occurred and setting mProfileShutdown to guard against new writes after this point. The theory I have regarding how this is not working is that it looks like PreferencesWriter::Flush doesn't actually guarantee that we've written the prefs. All it does is dispatches a sync runnable to the same event target. That would be fine if it were a single thread consuming from that event target, but since the target is the stream transport service, it's backed by a thread pool, and thus we only guarantee that the preferences write has started, but not that it has completed. Is there something in place that I'm missing which makes Flush correct?

Flags: needinfo?(dothayer) → needinfo?(n.nethercote)

In practice this shouldn't substantially change the behavior, but
it gets around being UB, and it shouldn't really cost us in terms
of performance. The alternative would be adding it to the TSAN
whitelist, which feels worse.

Depends on D71190

I'm not positive that the intended behavior of PreferencesWriter::Flush
is to ensure that any pending writes to the preferences file are
completed, but that seems like what it ought to be for, and it does
not look water-tight. For one, adding a sync runnable to a thread pool
will not ensure that any previously submitted runnables have completed
by the time it returns. And two, the exchange on sPendingWriteData only
guarantees that the write has started.

This change simply ensures that the write to disk has been completed
before returning.

Depends on D72015

Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bfb074351550
Actually advance lateWriteChecksStage to 3 r=erahm
https://hg.mozilla.org/integration/autoland/rev/2e05e81cc858
Squash TSAN data race report in IOInterposer r=aklotz
https://hg.mozilla.org/integration/autoland/rev/939611f65438
Ensure PreferencesWriter::Flush actually flushes r=njn
Depends on: 1610664
Depends on: 1633063
Flags: needinfo?(dothayer)
Keywords: leave-open
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/54d5f4042cb1
Squash TSAN data race report in IOInterposer r=aklotz
https://hg.mozilla.org/integration/autoland/rev/b9e4c3d3eca0
Ensure PreferencesWriter::Flush actually flushes r=njn
Regressions: 1636925
Attachment #9141018 - Attachment description: Bug 1630655 - Actually advance lateWriteChecksStage to 3 r?erahm → Bug 1630655 - Actually advance lateWriteChecksStage to 3

As far as I've been able to find digging through source code and
adding logging on try - everything at least intends to do its
writeAtomic calls before xpcom-will-shutdown. So we may as well
make that more explicit.

This won't make OS.File writes air tight. We could still add writes
after this point and they will simply fail. However, that was still
possible before this change anyway.

Depends on D71190

Flags: needinfo?(n.nethercote)
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/972fcba11cb6
Actually advance lateWriteChecksStage to 3 r=erahm
https://hg.mozilla.org/integration/autoland/rev/c8e86127060f
Move OS.File shutdown forward r=froydnj
Status: NEW → RESOLVED
Closed: 3 months ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.