Closed
Bug 250092
Opened 21 years ago
Closed 21 years ago
make prefs use nsISafeFileOutputStream
Categories
(Core :: Preferences: Backend, defect)
Core
Preferences: Backend
Tracking
()
RESOLVED
FIXED
mozilla1.8beta1
People
(Reporter: mvl, Assigned: dwitte)
References
Details
(Keywords: fixed-aviary1.0)
Attachments
(1 file, 3 obsolete files)
8.37 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•21 years ago
|
||
i'm all over this one... if no one else wants it ;)
Assignee: prefs → dwitte
Target Milestone: --- → mozilla1.8beta
Assignee | ||
Comment 2•21 years ago
|
||
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>)
Assignee | ||
Comment 3•21 years ago
|
||
Comment on attachment 152811 [details] [diff] [review]
patch v1
(my r/sr request also includes the cvs rm of nsSafeSaveFile.*).
Attachment #152811 -
Flags: superreview?(darin)
Attachment #152811 -
Flags: review?(mvl)
Comment 4•21 years ago
|
||
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.
Attachment #152811 -
Flags: superreview?(darin) → superreview-
Assignee | ||
Comment 5•21 years ago
|
||
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•21 years ago
|
||
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.
Assignee | ||
Comment 7•21 years ago
|
||
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
}
Assignee | ||
Updated•21 years ago
|
Attachment #152811 -
Flags: review?(mvl)
Assignee | ||
Comment 8•21 years ago
|
||
now with Finish(), for added syntactic sugar.
Attachment #152811 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #153026 -
Flags: superreview?(darin)
Attachment #153026 -
Flags: review?(mvl)
Comment 9•21 years ago
|
||
Comment on attachment 153026 [details] [diff] [review]
v2
woo-hoo! looks great :-)
sr=darin
Attachment #153026 -
Flags: superreview?(darin) → superreview+
Assignee | ||
Comment 10•21 years ago
|
||
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.)
Depends on: 251091
Whiteboard: needed-aviary1.0
Comment 11•21 years ago
|
||
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?
Attachment #153026 -
Flags: review?(mvl) → review-
Comment 12•21 years ago
|
||
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.
Assignee | ||
Comment 13•21 years ago
|
||
updated with nsISafeOutputStream love.
Attachment #153026 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #153712 -
Flags: superreview?(darin)
Attachment #153712 -
Flags: review?(cbiesinger)
Comment 14•21 years ago
|
||
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
Attachment #153712 -
Flags: superreview?(darin) → superreview+
Comment 15•21 years ago
|
||
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
Attachment #153712 -
Flags: review?(cbiesinger) → review+
Assignee | ||
Comment 16•21 years ago
|
||
checked in. added 0600 permissions to the output stream, and noted in bug
59557.
thx!
Assignee | ||
Updated•21 years ago
|
Attachment #153712 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•21 years ago
|
Keywords: fixed-aviary1.0
Whiteboard: needed-aviary1.0
You need to log in
before you can comment on or make changes to this bug.
Description
•