toolkit.shutdown.lateWriteChecksStage is not properly set on Nightly
Categories
(Core :: XPCOM, defect)
Tracking
()
People
(Reporter: alexical, Assigned: alexical)
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.
|
Assignee | |
Comment 1•4 years ago
|
||
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
|
||
Comment 3•4 years ago
|
||
Backed out for failures regarding /IOInterposer
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=325e655b6024492c976ba22bf2335ad5d2229036&selectedJob=298043860
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=298043860&repo=autoland&lineNumber=1998
and
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=298044106&repo=autoland&lineNumber=6235
Backout: https://hg.mozilla.org/integration/autoland/rev/d6a4c108cd4038f7b5516c1b3ff916f5c0d98b17
|
|
Assignee | |
Comment 4•4 years ago
|
||
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?
|
|
Assignee | |
Comment 5•4 years ago
|
||
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
|
|
Assignee | |
Comment 6•4 years ago
|
||
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
|
||
Comment 8•4 years ago
|
||
Backed out 3 changesets for causing failures in test_headless_screenshot.html
Backout link: https://hg.mozilla.org/integration/autoland/rev/05dd19d0ac177958c450b7e11cf47f49b987cb74
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=299331150&repo=autoland&lineNumber=1485
|
|
Assignee | |
Updated•4 years ago
|
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
|
||
Comment 10•4 years ago
|
||
| bugherder | ||
|
||
Updated•4 years ago
|
|
|
Assignee | |
Comment 11•4 years ago
|
||
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
|
||
Updated•4 years ago
|
|
||
Comment 12•4 years ago
|
||
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
|
|
||
Comment 13•4 years ago
|
||
| bugherder | ||
|
|
Assignee | |
Updated•4 years ago
|
Description
•