Potential crash while folder compaction

RESOLVED FIXED in Thunderbird 25.0

Status

MailNews Core
Backend
--
critical
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: hiro, Assigned: hiro)

Tracking

({crash})

Trunk
Thunderbird 25.0
crash

Thunderbird Tracking Flags

(thunderbird24+ fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 656240 [details] [diff] [review]
Fix with an xpcshell test

I found that Release() is unnecessary in nsFolderCompactState::Init() because the nsFolderCompactState instance is not AddRef() yet.
Attachment #656240 - Flags: review?(mbanner)
Comment on attachment 656240 [details] [diff] [review]
Fix with an xpcshell test

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

Sorry for the delay, r=me with the comments fixed.

::: mailnews/base/test/unit/test_compactFailure.js
@@ +1,1 @@
> +const TARGET_FOLDER_NAME = "Target"

You're only using this once, so I don't think this is necessary as a const.

@@ +1,3 @@
> +const TARGET_FOLDER_NAME = "Target"
> +const NS_LOCALFILEOUTPUTSTREAM_CONTRACTID = "@mozilla.org/network/file-output-stream;1";
> +const REGISTRAR = Components.manager.QueryInterface(Ci.nsIComponentRegistrar);

I'd prefer kRegistrar for this.

@@ +89,5 @@
> +  });
> +  try {
> +    compactor.compact(gTargetFolder, false, listener, null);
> +  } catch(ex) {
> +    do_check_eq(Cr.NS_ERROR_FILE_IS_LOCKED, ex.result);

I think you should also add a check in to check that compactor.compact actually fails, as if it succeeds we're not going to notice as it stands.
Attachment #656240 - Flags: review?(mbanner) → review+
Assignee: nobody → hiikezoe
(In reply to Mark Banner (:standard8) from comment #1)
> I think you should also add a check in to check that compactor.compact
> actually fails, as if it succeeds we're not going to notice as it stands.

so this would crash @ nsFolderCompactState::Init ?

(FWIW, found nothing close in crash-stats)
Flags: needinfo?(hiikezoe)
this may have bitrotted also
rkent, could you dust/finish off this patch?

Comment 5

4 years ago
(In reply to Wayne Mery (:wsmwk) from comment #4)
> rkent, could you dust/finish off this patch?

OK I'll try to get to this. I'll add my name to Assigned so that I see it, but anyone who is motivated is welcome to finish it off as well.
Assignee: hiikezoe → kent
(don't have signature but it seems reasonable that we'd see this as a crash in the wild)

If kent can't finish, can we find someone else?
Severity: normal → critical
Flags: needinfo?(kent)
Keywords: crash

Comment 7

4 years ago
I think Hiro can finish it now that he is back with us :)
Assignee: kent → hiikezoe
(Assignee)

Comment 8

4 years ago
Created attachment 780128 [details] [diff] [review]
Fix_bug786487.patch

Addressed comment#1.
Attachment #656240 - Attachment is obsolete: true
Attachment #780128 - Flags: review+
Flags: needinfo?(hiikezoe)
(Assignee)

Updated

4 years ago
Flags: needinfo?(kent)
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/aa2e90a554cc
Status: NEW → RESOLVED
Last Resolved: 4 years ago
tracking-thunderbird24: --- → +
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 25.0
Comment on attachment 780128 [details] [diff] [review]
Fix_bug786487.patch

[Triage Comment]
I think we should take this on 24 there to help any potential issues.
Attachment #780128 - Flags: approval-comm-aurora+
Comment on attachment 780128 [details] [diff] [review]
Fix_bug786487.patch

[Triage Comment]
Attachment #780128 - Flags: approval-comm-aurora+ → approval-comm-beta+
https://hg.mozilla.org/releases/comm-beta/rev/a9fc41ede3f4
status-thunderbird24: --- → fixed
found only one crash in 12 weeks bp-8ba90450-bf8c-4b81-bea2-c70052130717 17.0.3esr.
so crash rate too low to determine from crash-stats whether this patch impacts crashes in the field.
See Also: → bug 673904
You need to log in before you can comment on or make changes to this bug.