nsIMsgDBService::openFolderDB unusable from JavaScript

RESOLVED FIXED in Thunderbird 3.0b2

Status

P2
major
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: jcranmer, Assigned: jcranmer)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
Thunderbird 3.0b2
Dependency tree / graph
Bug Flags:
blocking-thunderbird3.0a2 -
wanted-thunderbird3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

11 years ago
The definition of this function is as follows:
nsIMsgDatabase openFolderDB(in nsIMsgFolder aFolder, in boolean aCreate, in boolean aLeaveInvalidDB);

However, if the database file doesn't exist, or is corrupted, the database is returned and the function throws NS_MSG_ERROR_FOLDER_SUMMARY_MISSING or the corresponding corrupted error. The missing folder summary error is generated if either the file doesn't exist, or it has zero length.

If we request that the database create the file through the aCreate parameter, it will create a new file with zero length and then return the database so that the caller can prefill it, as well as returning the error.

However, from a Javascript extension, the error is translated to a throw before the return value can be intercepted, which means that, without resorting to crude hackery (manually manipulating the creation of databases), a database cannot be created.

This can be fixed by the simple addition of an outparam that indicates whether or not the database needs to be created.
Flags: blocking-thunderbird3.0a2?
(Assignee)

Comment 1

11 years ago
Created attachment 324223 [details] [diff] [review]
Patch to fix the problem

Fix to the problem. In short, the old style of throwing an error can be emulated by setting the outparam to be null, but, if one expects this type of error, it is set and returns NS_OK.

Verified that it handles ok from the JavaScript end and for IMAP folders.
Attachment #324223 - Flags: superreview?(bienvenu)
Attachment #324223 - Flags: review?(bienvenu)

Comment 2

11 years ago
Joshua, I agree that the method is awkward to use, and not just from JS. It's definitely worth fixing.  I need to think a little bit about whether this is the best way to fix it or not...
Doesn't need to block 3.0a2.
Flags: wanted-thunderbird3+
Flags: blocking-thunderbird3.0a2?
Flags: blocking-thunderbird3.0a2-

Comment 4

11 years ago
Joshua, would this work for you? If the caller said to create a new db, and not leave an invalid one, then creating a new db isn't an error (otherwise, it still is). And we add a new boolean out parameter, aNewDBCreated for the caller to detect this situation. I'm not sure if that handles all the cases, but it seems slightly cleaner.

quick comment about the patch:
   NS_ENSURE_ARG(aFolder);
+  if (aInvalidReason) *aInvalidReason = NS_OK;

and then later on, you do an NS_ENSURE_ARG on aInvalidReason in the error case. I think it would be much better to just do NS_ENSURE_ARG_POINTER() at the very top, to shake out any callers not doing the right thing, before they actually run into an error.

Comment 5

11 years ago
oh, and just to be clear, I'm proposing the new boolean out parameter instead of the boolean out aInvalidReason.
(Assignee)

Comment 6

11 years ago
Created attachment 325289 [details] [diff] [review]
Updated patch, with improved documentation

Updated, taking into account bienvenu's comments and some discussion over IRC.

I also took the time to document the interface fully, and add in some module documentation.
Attachment #324223 - Attachment is obsolete: true
Attachment #325289 - Flags: superreview?(bienvenu)
Attachment #325289 - Flags: review?(bienvenu)
Attachment #324223 - Flags: superreview?(bienvenu)
Attachment #324223 - Flags: review?(bienvenu)

Comment 7

11 years ago
Comment on attachment 325289 [details] [diff] [review]
Updated patch, with improved documentation

OK, you're going to hate me, but looking at the code, aDBCreated isn't the right name at all - it should be aInvalidDBLeft. Which almost makes me want to say it should be an in-out parameter, but that's a pain for the callers. I guess in some situations, we do expect it to create a new db, if the previous call to OpenFolderDB did delete the db...

+    PRBool created = PR_FALSE;
+    rv = msgDBService->OpenFolderDB(this, PR_TRUE, PR_FALSE, &created,
+                                    getter_AddRefs(mDatabase));
+
+    if (NS_FAILED(rv))
+      rv = msgDBService->OpenFolderDB(this, PR_TRUE, PR_TRUE, &created,
+                                      getter_AddRefs(mDatabase));

for the first call, we should pass in nsnull instead of &created, since we've passed in PR_FALSE for aLeaveInvalidDB.

and some nits:

+   * This method should only be used if the caller does not have a folder
+   * instance, since it does not perform additional folder operations. For this

Instead of "since it does not..." I would say "because the resulting db and msg hdrs retrieved from the db would not know their owning folder, which limits their usefulness."

+   * Unlike nsIMsgDBService::openFolderDB, this method does not have a parameter
+   * that is used to tell whether or not the database was created. In this case,
+   * the method will return normally.

That makes sense in the context of the bug, but not the context of someone reading the idl since they won't know what "normally" means. Perhaps "returns NS_MSG_ERROR_FOLDER_SUMMARY_MISSING/OUT_OF_DATE if those errors are encountered.

+   * caller has an actual nsIMsgFolder around. If the database detects that it
+   * is unreadable, it will destroy itself and prepare to be rebuilt, unless 
+   * aLeaveInvalidDB is true.

Instead of "will destroy itself", how about "will delete the .msf file". 

+   * Since the message database will likely be opened and closed many times, by
+   * registering using this method, one will be guaranteed to see all subsequent
+
Not "will likely be open and closed many times" - "may be opened and closed".

+    // JavaScript code trying to use this method. For this reason, we set the
+    // argument, but only if the argument is nonnull (i.e., the caller is

instead of "argument", "out parameter aDBCreated" - argument is vague...

Comment 8

11 years ago
Comment on attachment 325289 [details] [diff] [review]
Updated patch, with improved documentation

minusing based on comments.
Attachment #325289 - Flags: superreview?(bienvenu)
Attachment #325289 - Flags: superreview-
Attachment #325289 - Flags: review?(bienvenu)
Attachment #325289 - Flags: review-
(Assignee)

Comment 9

11 years ago
In response to some IRC conversations (one in #maildev last Monday and one in #developers this morning), I looked at how people tackled the SUMMARY_MISSING error.

NNTP and IMAP consumers both treated it as effectively equal to NS_OK. The nsLocalMailFolder.cpp tended to pass the error on or patch it up (do a little thing to make it valid) before continuing. Where it passed the error on, it was either treated as an NS_OK again or went back to the folder to update it.

OUT_OF_DATE is even more hackish; NNTP and IMAP both ignore it, while non-local mail consumers treat as identical to SUMMARY_MISSING and force an update, with local folders also doing more or less the same thing.

Right now, SUMMARY_MISSING is the more pressing matter for this bug. bsmedberg and others have expressed disapproval of the making it a successful return; in lieu of such an event, I think what makes the most sense and least hackishness  would be to have a callback to the folder to have it handle a new or out-of-date database.

bienvenu, thoughts?

Comment 10

11 years ago
The reason NNTP and IMAP can treat summary missing as OK is that they know they're going to resync with the server in the normal course of things. 

OUT_OF_DATE doesn't apply to IMAP or NNTP by its very nature. OUT_OF_DATE means the local mailbox was changed from outside Thunderbird. Local Folders do something very special with out of date - they force a reparse of the folder.

> I think what makes the most sense and least hackishness 
>would be to have a callback to the folder to have it handle a new or
>out-of-date database.

I don't know what you're proposing exactly. Currently in the mailnews backends, it's the folders that open the db's and handle the various errors.
(Assignee)

Comment 11

11 years ago
Created attachment 325631 [details] [diff] [review]
Patch

Third try's a charm?

Anyways, this implements the latest decision of how to go about this (once again over IRC).
Attachment #325289 - Attachment is obsolete: true
Attachment #325631 - Flags: superreview?(bienvenu)
Attachment #325631 - Flags: review?(bienvenu)
Product: Core → MailNews Core
(Assignee)

Updated

10 years ago
Blocks: 449226
Retriaging according to new policy for flags.
https://wiki.mozilla.org/Thunderbird:Release_Driving
(bugs marked wanted- don't indicate we wouldn't accept patches, but that they're not going to be the focus for release drivers)
Priority: -- → P2
Target Milestone: --- → Thunderbird 3.0b2
(Assignee)

Updated

10 years ago
Attachment #325631 - Flags: superreview?(bienvenu)
Attachment #325631 - Flags: review?(bienvenu)
(Assignee)

Comment 13

10 years ago
Comment on attachment 325631 [details] [diff] [review]
Patch

This patch has now bitrotted to the point where it's causing tests to fail locally. Clearing reviews.

Updated

10 years ago
Target Milestone: Thunderbird 3.0b1 → Thunderbird 3.0b2

Comment 14

10 years ago
Comment on attachment 325631 [details] [diff] [review]
Patch

I think this comment isn't right anymore:
+   * Unlike nsIMsgDBService::openFolderDB, this method does not have a parameter
+   * that is used to tell whether or not the database was created. In this case,
+   * the method will return the errors as detailed below.
+   *
(Assignee)

Comment 15

10 years ago
Created attachment 353719 [details] [diff] [review]
patch

6 months to the day later, an unbitrotted version that passes all tests. With better documentation.
Attachment #325631 - Attachment is obsolete: true
Attachment #353719 - Flags: superreview?(bienvenu)
Attachment #353719 - Flags: review?(bienvenu)

Comment 16

10 years ago
make check doesn't work for me...trying an other tree w/o the db patch.

Comment 17

10 years ago
make check is now working, but I'm having an issue loading my saved searches. I'll have to figure that out first.

Comment 18

10 years ago
Comment on attachment 353719 [details] [diff] [review]
patch

all my issues were because of our horked build system.
Attachment #353719 - Flags: superreview?(bienvenu)
Attachment #353719 - Flags: superreview+
Attachment #353719 - Flags: review?(bienvenu)
Attachment #353719 - Flags: review+
(Assignee)

Comment 19

10 years ago
Pushed as changeset e10a3baac8d7, with a very slight documentation modification (the CID wasn't popping out right, so I added a \ to make it do so).
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 20

10 years ago
A problem has come up with these changes.

Please review Bug 471130 for me.  Also that bug needs confirmation from windows user with oexpress (outlook express)
Since Bugzilla doesn't currently have a clean way to indicate "regression caused by", we overload the "depends on" field for that purpose; thanks for figuring this out, Phil!
Depends on: 471130

Updated

10 years ago
Depends on: 471307

Updated

10 years ago
Depends on: 471682

Updated

10 years ago
Depends on: 472446

Updated

10 years ago
Depends on: 472628
You need to log in before you can comment on or make changes to this bug.