973579 - TelemetryFile should fail silently when not overwriting an existing file

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Roberto Agostino Vitillo (:rvitillo)]

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 #

Comment 1

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: Fail silently when not overwriting an existing file, v1

This patch restores the original behavior of TelemetryFile.savePingToFile (pre Bug 839794) which ignored errors due to the file already existing.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

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
}

Comment 3

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: TelemetryFile should fail silently when not overwriting an existing file, v2

Comment 4

[David Teller [:Yoric] - still alive but not very active]

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

Comment 5

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: TelemetryFile should fail silently when not overwriting an existing file, v3

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/5f87786ceab5

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5f87786ceab5