1602326 - GetOfflineFileStream() can fail to return a stream even upon success.

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Ben Campbell]

In particular, nsMsgDBFolder::GetOfflineFileStream.
Right at the top, there are some cases where a failure to access the database will result in a return code of NS_OK but the return value aFileStream being left untouched.

This seems unintuitive. If I called GetOfflineFileStream() and got NS_OK as the result code, then I'd expect to have a valid stream too.
Indeed, a lot of uses of GetOfflineFileStream() either check both the result code and the returned stream, or just ignore the code completely.

If NS_OK and no stream is indeed an acceptable return condition, then at least it should be documented as such in nsIMsgFolder.idl.

Comment 1

[Ben Campbell]

Just a note to myself from looking at Bug 1596036:
Seems pretty obvious to me that GetOfflineFileStream() should always return a non-null stream upon success (and failure should always return a fail code). The work is in making sure all the callers of GetOfflineFileStream() conform to that.

Comment 2

[Addons]

OK. Please don't forget the patch in attachment 9204913 [details] [diff] [review] of bug 1596036. There is some clean-up to do.

Comment 3

[Ben Campbell]

Attachment: Bug 1602326 - Make sure nsIMsgFolder.getOfflineFileStream() returns a stream or an error code. r=mkmelin

Comment 4

[Addons]

So where to you intend to address comment #1 and comment #2?

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e40d2042c6b7
Make sure nsIMsgFolder.getOfflineFileStream() returns a stream or an error code. r=mkmelin

Comment 6

[Ben Campbell]

Attachment: 1602326-tidy-up-nsIMsgFolder-getOfflineFileStream-callers-1.patch

Just going though the call sites of getOfflineFileStream() and giving them a freshen-up now it's return conditions have been straightened out.

Comment 7

[Ben Campbell]

Doh, didn't realise I was using phab on this bug!

Comment 8

[Addons]

Nice, but you should reopen the bug.

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9208548 [details] [diff] [review]
1602326-tidy-up-nsIMsgFolder-getOfflineFileStream-callers-1.patch

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

LGTM, r=mkmelin

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9d888b16cc38
Tidy up error handling for callers of nsIMsgFolder.getOfflineFileStream(). r=mkmelin