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

RESOLVED FIXED in Firefox 38

Status

defect
P4
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: markh, Assigned: markh)

Tracking

(Blocks 1 bug)

unspecified
mozilla40
Points:
1
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox38 fixed, firefox39 fixed, firefox40 fixed)

Details

Attachments

(1 attachment)

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.
Attachment #8589492 - Flags: review?(rnewman)
Flags: qe-verify-
Flags: firefox-backlog+
Assignee: nobody → mhammond
Iteration: --- → 40.1 - 13 Apr
Blocks: 1132074
Status: NEW → ASSIGNED
Priority: -- → P4
Iteration: 40.1 - 13 Apr → 40.2 - 27 Apr
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.
Attachment #8589492 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/mozilla-central/rev/47c6e3a311f1
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
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.
Attachment #8589492 - Flags: approval-mozilla-beta?
Attachment #8589492 - Flags: approval-mozilla-aurora?
Comment on attachment 8589492 [details] [diff] [review]
0001-Bug-XXXXXXX-ensure-sync-readinglist-log-directory-ex.patch

Should be in 38 beta 6
Attachment #8589492 - Flags: approval-mozilla-beta?
Attachment #8589492 - Flags: approval-mozilla-beta+
Attachment #8589492 - Flags: approval-mozilla-aurora?
Attachment #8589492 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.