Last Comment Bug 786487 - Potential crash while folder compaction
: Potential crash while folder compaction
Status: RESOLVED FIXED
: crash
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: -- critical (vote)
: Thunderbird 25.0
Assigned To: Hiroyuki Ikezoe (:hiro)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-28 16:01 PDT by Hiroyuki Ikezoe (:hiro)
Modified: 2013-10-21 15:50 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed


Attachments
Fix with an xpcshell test (5.41 KB, patch)
2012-08-28 16:01 PDT, Hiroyuki Ikezoe (:hiro)
standard8: review+
Details | Diff | Splinter Review
Fix_bug786487.patch (5.44 KB, patch)
2013-07-23 18:56 PDT, Hiroyuki Ikezoe (:hiro)
hiikezoe: review+
standard8: approval‑comm‑beta+
Details | Diff | Splinter Review

Description Hiroyuki Ikezoe (:hiro) 2012-08-28 16:01:47 PDT
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.
Comment 1 Mark Banner (:standard8) 2012-10-04 08:06:50 PDT
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.
Comment 2 Wayne Mery (:wsmwk, NI for questions) 2012-11-13 05:54:40 PST
(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)
Comment 3 Wayne Mery (:wsmwk, NI for questions) 2012-12-21 12:58:27 PST
this may have bitrotted also
Comment 4 Wayne Mery (:wsmwk, NI for questions) 2013-02-03 22:05:23 PST
rkent, could you dust/finish off this patch?
Comment 5 Kent James (:rkent) 2013-03-26 10:04:00 PDT
(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.
Comment 6 Wayne Mery (:wsmwk, NI for questions) 2013-07-18 12:16:40 PDT
(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?
Comment 7 :aceman 2013-07-18 12:30:51 PDT
I think Hiro can finish it now that he is back with us :)
Comment 8 Hiroyuki Ikezoe (:hiro) 2013-07-23 18:56:18 PDT
Created attachment 780128 [details] [diff] [review]
Fix_bug786487.patch

Addressed comment#1.
Comment 9 Mark Banner (:standard8) 2013-08-05 01:59:54 PDT
https://hg.mozilla.org/comm-central/rev/aa2e90a554cc
Comment 10 Mark Banner (:standard8) 2013-08-12 03:37:31 PDT
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.
Comment 11 Mark Banner (:standard8) 2013-08-27 02:40:56 PDT
Comment on attachment 780128 [details] [diff] [review]
Fix_bug786487.patch

[Triage Comment]
Comment 12 Mark Banner (:standard8) 2013-08-27 02:48:35 PDT
https://hg.mozilla.org/releases/comm-beta/rev/a9fc41ede3f4
Comment 13 Wayne Mery (:wsmwk, NI for questions) 2013-09-21 08:41:06 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.