252053 - make bookmarks use nsISafeOutputStream

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[[e-mail]]

convert another consumer over to using our new safe-save functionality (see bug
246675).

doing so will fix the following open bugs (there are both seamonkey and fx bugs
in this list, but it's all really the same thing):

157152 (by not leaving the tempfile around in the failure case)
220159 (maybe... not sure what's going on here)
206567 (once i land a fix to the safe-save impl, see bug 252050)

i'll be doing this for seamonkey too.

Comment 1

[[e-mail]]

Attachment: le patch

Comment 2

[[e-mail]]

Comment on attachment 153710 [details] [diff] [review]
le patch

vlad, what do you think?

(see nsCookieService::Write() for an example of an existing nsISafeOutputStream
consumer.)

Comment 3

[[e-mail]]

just as a note, using nsISafeOutputStream makes all the |rv| checks of
|stream->Write()| redundant. there are a lot of these checks scattered through
the bookmarks write code, but i didn't touch any of them since they are
intertwined with other fu (rdf etc) and i didn't want to risk breaking anything.

if your opinion differs, let me know :)

Comment 4

[[e-mail]]

filed bug 252193 for seamonkey.

also, in this patch, i clobbered the 0600 permissions when creating the safe
output stream. that was a mistake, just pretend they're there :)

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Looks good, two questions though:

>+    // All went ok. Maybe except for problems in Write(), but the stream detects
>+    // that for us
>+    nsCOMPtr<nsISafeOutputStream> safeStream = do_QueryInterface(strm);

I believe you need either 'out' instead of 'strm' here, or you need to get the
Sink() from strm.  (Great variable names, eh?)

>+    if (NS_SUCCEEDED(rv) && safeStream)
>+        rv = safeStream->Finish();

You're layering the BufferedOutputStream on top of the safeStream, right?  Does
this need a strm->Flush() before Finish()?

Comment 6

[[e-mail]]

re both your comments: actually, nsBufferedOutputStream has magical machinery to
deal with nsISafeOutputStream. see bug 251648 for the gory details, but suffice
to say this "just works" :/

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 153710 [details] [diff] [review]
le patch

Ahh, I didn't see the changes in BufferedOutputStream.	Looks good then,
r=vladimir.

Setting aviary1.0?, ben might want this on aviary for 1.0 -- there's a number
of bookmarks-getting-clobbered bugs against firefox.

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Forgot to mention -- please add in a comment when you commit regarding
BufferedOutputStream having magic to handle SafeOutputStream..

Comment 9

[[e-mail]]

adding needed-aviary1.0 per brendan/shaver.

thanks for review vlad, i'll add the comment and commit with the seamonkey patch
(in case reviewers have comments).

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

patch is in on branch and trunk firefox, -> FIXED

Comment 11

[Mike Connor [:mconnor]]

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change