Closed Bug 1152193 Opened 5 years ago Closed 5 years ago

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

Categories

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

defect
Points:
1

Tracking

(firefox38 fixed, firefox39 fixed, firefox40 fixed)

RESOLVED FIXED
mozilla40
Iteration:
40.2 - 27 Apr
Tracking Status
firefox38 --- fixed
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: markh, Assigned: markh)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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: 5 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.