1152193 - Sync/Readinglist log file directory not created when writing a log file.

Status: RESOLVED FIXED

(Cloud Services :: Firefox: Common, defect, P4)

Description

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-XXXXXXX-ensure-sync-readinglist-log-directory-ex.patch

Currently the LogManager attempts to create the "logs" directory under "weave", but not to first ensure "weave" exists.  The "weave" directory will often be created by Sync, but if sync is enabled or hasn't yet got around to it, creating a file log will fail.

This patch fixes this, and adds a test stolen from bug 1148980.

Comment 1

[Richard Newman [:rnewman]]

Comment on attachment 8589492 [details] [diff] [review]
0001-Bug-XXXXXXX-ensure-sync-readinglist-log-directory-ex.patch

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

::: services/common/logmanager.js
@@ +169,5 @@
> +    // We assume the profile directory exists, but not that the dirs under it do.
> +    let outputFile = FileUtils.getDir("ProfD", []);
> +    for (let sub of this._logFileSubDirectoryEntries) {
> +      outputFile.append(sub);
> +      yield OS.File.makeDir(outputFile.path, { ignoreExisting: true });

Don't bother with this. OS.File.makeDir will recursively create directories if you specify `from`, so just make the whole path and call it once with the profile directory as `from`. Buy some little efficiencies and some simplicity.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

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

Comment 3

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/47c6e3a311f1

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8589492 [details] [diff] [review]
0001-Bug-XXXXXXX-ensure-sync-readinglist-log-directory-ex.patch

Approval Request Comment
[Feature/regressing bug #]: ReadingList causing Sync regression
[User impact if declined]: Sync may not generate log files.
[Describe test coverage new/current, TreeHerder]: Has tests.
[Risks and why]: Low risk, limited to sync
[String/UUID change made/needed]: None

Note we still want this on 38 and 39 due to Sync.

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8589492 [details] [diff] [review]
0001-Bug-XXXXXXX-ensure-sync-readinglist-log-directory-ex.patch

Should be in 38 beta 6

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5d9a91678527

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fc98815acf5f