250092 - make prefs use nsISafeFileOutputStream

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[Michiel van Leeuwen (email: mvl+moz@)]

prefs should use the new nsISafeFileOutputStream code (bug 246675) instead of
it's own nsSafeSaveFile (bug 132517). If nothing else, it will reduce codesize.
But prefs uses some guessing at wether the safe was successfull, while
nsISafeFileOutputStream knows.

Comment 1

[[e-mail]]

i'm all over this one... if no one else wants it ;)

Comment 2

[[e-mail]]

Attachment: patch v1

so this ditches nsSafeSaveFile. mostly straightforward; the only real
difference in the impl here is that nsSafeSaveFile used to make a backup copy
if the new file was < 1/2 the size of the old file ("dataloss"), whereas
nsISafeFileOutputStream won't. i don't think that'll be an issue. also, the
rv-checking fu at the very end of WritePrefFile() is a tad ugly, because afaict
we need to check that (a) the writes succeeded, and also (b) Close() succeeded,
since we're dealing with a buffered output stream that may not write data
immediately. comments welcome.

(i also switched some raw ptrs over to nsCOMPtrs... <shudder>)

Comment 3

[[e-mail]]

Comment on attachment 152811 [details] [diff] [review]
patch v1

(my r/sr request also includes the cvs rm of nsSafeSaveFile.*).

Comment 4

[Darin Fisher]

Comment on attachment 152811 [details] [diff] [review]
patch v1

>Index: modules/libpref/src/nsPrefService.cpp

>+  // close the stream, and make sure all the writes succeeded
>+  rv = outStream->Close();
>+  PRBool succeeded;
>+  safeStream->GetWriteSucceeded(&succeeded);
>+  if (!succeeded)
>+    rv = NS_ERROR_FAILURE;

hmm.. you should not have to call GetWriteSucceeded after calling
Close.	it should be enough to check the return value of the Close
method.  i know this therefore depends on that other bug fix, but
i think that's fine.

Comment 5

[[e-mail]]

okay, i can go along with that. mvl and i actually discussed whether Close
should propagate mInternalWriteSucceeded failures, and decided no. the logic
being that Close should only do two things; (a) close the channel and (b)
perform cleanup, so rv's should only correspond to those two things. but what
you say also sounds reasonable, so we can do that...

Comment 6

[Darin Fisher]

Failure returned by nsIOutputStream::Close often indicates that data could not
be flushed to some lower-level stream.  Given that it makes sense for Close to
reflect Write failures.

Comment 7

[[e-mail]]

oh, hmm... i was thinking of something slightly different (my patch in bug
246675 just makes the forwarded Close() rv propagate).

so, just to be clear... you want something like this, in our safe Close() impl?

nsresult rv = nsFileOutputStream::Close();

if (!mWriteSucceeded || !mInternalWriteSucceeded) {
    mTempFile->Remove(PR_FALSE);
    return NS_ERROR_FAILURE; // we closed the stream, but some writes failed
}

Comment 8

[[e-mail]]

Attachment: v2

now with Finish(), for added syntactic sugar.

Comment 9

[Darin Fisher]

Comment on attachment 153026 [details] [diff] [review]
v2

woo-hoo!  looks great :-)

sr=darin

Comment 10

[[e-mail]]

brendan/shaver said this should definitely be in for aviary1.0 - it's a nice
feature and we'll have testing time in 1.8a2. adding keyword. (bookmarks too, if
someone gets around to it.)

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 153026 [details] [diff] [review]
v2

+  nsCOMPtr<nsISafeFileOutputStream> safeStream = do_QueryInterface(outStream);

outStream is an bufferedOutputStream. this QI will fail. thus, finish will
never be called, and the data will be thrown away in all cases.


let's assume you'd write outStreamSink here. how do you guarantee that the
bufferedstream has already written all the data to the fileoutputstream?

can you remove nsSafeSaveFile now?

Comment 12

[Darin Fisher]

good catch biesi!  call outStream->Close first and check the return value? 
(that would work as a quick fix at least.)  seems like it would be cleaner if we
could have Finish do that somehow.  hmm... and making nsBufferedOutputStream
support nsISafeFileOutputStream seems bad.

Comment 13

[[e-mail]]

Attachment: v3

updated with nsISafeOutputStream love.

Comment 14

[Darin Fisher]

Comment on attachment 153712 [details] [diff] [review]
v3

>Index: modules/libpref/src/nsPrefService.cpp

>+  nsCOMPtr<nsISafeOutputStream> safeStream = do_QueryInterface(outStream);
>+  if (safeStream) {
>+    rv = safeStream->Finish();
>+    if (NS_FAILED(rv)) {
>+      NS_WARNING("failed to save prefs file! possible dataloss");
>+      return rv;
>+    }
>+  }

seems to me that you might want to assert that the QI doesn't fail.

sr=darin

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 153712 [details] [diff] [review]
v3

+  rv = NS_NewSafeLocalFileOutputStream(getter_AddRefs(outStreamSink), aFile);

should prefs use 0600 as mode, maybe?


when checking this into any branch, remember to also land bug 251648

Comment 16

[[e-mail]]

Attachment: v4

checked in. added 0600 permissions to the output stream, and noted in bug
59557.

thx!