Closed Bug 973579 Opened 6 years ago Closed 6 years ago

TelemetryFile should fail silently when not overwriting an existing file

Categories

(Toolkit :: Telemetry, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: rvitillo, Assigned: rvitillo)

Details

Attachments

(1 file, 2 obsolete files)

philor
https://tbpl.mozilla.org/php/getParsedLog.php?id=34599338&tree=Mozilla-Inbound
Rev4 MacOSX Snow Leopard 10.6 mozilla-inbound talos dromaeojs on 2014-02-12 21:06:15
revision: d805f87989a2
slave: talos-r4-snow-126

[911] ###!!! ABORT: mismatched CxxStackFrame ctor/dtors: file /builds/slave/m-in-osx64-0000000000000000000/build/ipc/glue/MessageChannel.cpp, line 1710
[911] ###!!! ABORT: mismatched CxxStackFrame ctor/dtors: file /builds/slave/m-in-osx64-0000000000000000000/build/ipc/glue/MessageChannel.cpp, line 1710
PROCESS-CRASH | kraken | application crashed [@ mozalloc_abort(char const*)]
[911] ###!!! ABORT: mismatched CxxStackFrame ctor/dtors: file /builds/slave/m-in-osx64-0000000000000000000/build/ipc/glue/MessageChannel.cpp, line 1710
[911] ###!!! ABORT: mismatched CxxStackFrame ctor/dtors: file /builds/slave/m-in-osx64-0000000000000000000/build/ipc/glue/MessageChannel.cpp, line 1710
Traceback (most recent call last):
Return code: 2
# TBPL FAILURE #
Assignee: nobody → rvitillo
Summary: Unix error 17 during operation writeAtomic → Fail silently when not overwriting an existing file
This patch restores the original behavior of TelemetryFile.savePingToFile (pre Bug 839794) which ignored errors due to the file already existing.
Attachment #8377108 - Flags: review?(dteller)
Comment on attachment 8377108 [details] [diff] [review]
Fail silently when not overwriting an existing file, v1

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

Fine with me, with trivial changes.

::: toolkit/components/telemetry/TelemetryFile.jsm
@@ +64,5 @@
>     *
>     * @param {object} ping The content of the ping to save.
>     * @param {string} file The destination file.
>     * @param {bool} overwrite If |true|, the file will be overwritten
>     * if it exists.

Nit: Now you'll need to document |false|.

@@ +73,5 @@
> +      try {
> +        let pingString = JSON.stringify(ping);
> +        yield OS.File.writeAtomic(file, pingString, {tmpPath: file + ".tmp",
> +                                  noOverwrite: !overwrite});
> +      } catch(e) {

catch (e if e.becauseExists && !overwrite) {
  // Some comment explaining why we need to fail silently
}
Attachment #8377108 - Flags: review?(dteller) → review+
Summary: Fail silently when not overwriting an existing file → TelemetryFile should fail silently when not overwriting an existing file
Attachment #8377118 - Flags: review?(dteller)
Attachment #8377108 - Attachment is obsolete: true
Attachment #8377118 - Flags: review?(dteller)
Comment on attachment 8377118 [details] [diff] [review]
TelemetryFile should fail silently when not overwriting an existing file, v2

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

::: toolkit/components/telemetry/TelemetryFile.jsm
@@ +75,5 @@
> +        let pingString = JSON.stringify(ping);
> +        yield OS.File.writeAtomic(file, pingString, {tmpPath: file + ".tmp",
> +                                  noOverwrite: !overwrite});
> +      } catch(e) {
> +        if (overwrite || !e.becauseExists)

I'd still prefer
catch (e if ...) {
  // Here, we don't throw
}
// All other exceptions remain uncaught
Attachment #8377118 - Flags: review?(dteller) → review+
Attachment #8377118 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/5f87786ceab5
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/5f87786ceab5
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.