Daily from 2017-09-28 re-downloads and indexes messages at every start - high CPU

RESOLVED FIXED in Thunderbird 58.0

Status

defect
--
major
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: Paenglab, Assigned: jorgk)

Tracking

({perf, regression})

Trunk
Thunderbird 58.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [regression:TB58])

Attachments

(1 attachment)

Starting with today Daily from 2017-09-28 at every start it downloads the messages from IMAP server and idexes them with high COU load (around 29% on a I7/2.2 GHz). Also the thread view is in a default state and not my configured view. Also Chat says there are new messages but they aren't show.

I waited until the download/indexing finished and restarted Daily and all began again.

When I go back to the Daily from 26. all is immediately shown as it should and no download/indexing.
I'd say Daily after 2017-09-28 is pretty broken as we have severe test failures in bug 1403771.

I'm currently bisecting the test failures starting before bug 1402888, so with M-C at cb604e7830ec and C-C at 3f2554599137. The next step would be to include bug 1402888, so move M-C to 7e992ffa004c and C-C to 523f36e9ebc5 and then 5b7dc947139e.

If it's not bug 1402888, then I don't know and we need to ask FRG ;-)
Severity: normal → major
Keywords: perf, regression
Summary: Daily from 2017-09-28 re-downloads and indexes messages at every start → Daily from 2017-09-28 re-downloads and indexes messages at every start - high CPU
Can you try today's Daily, maybe it was related to bug 1404045.
Tried today's Daily,

IMAP accounts still reindex. There doesn't seem to be high CPU usage for me.
Connected to chat and no topics or conversations show. Bug 1366255 back?
I could not connect to chat with yesterday's build.
I did not look at the state of my thread view.
Still reindex here and the CPU usage is normal now.
I found the cause: Bug 1404045 comment #6.
Depends on: 1404045
No longer depends on: 1404045
Sadly bug 1404045 was not the cause (although it fixes other bustage).

For me the problem is even worse. Every time I start, Daily re-establishes the folder list, so it seems like it has forgotten all the folders.

We still have some Necko bustage in the tree, see bug 1403771, so unless that gets fixed, I can't say for sure that we're accessing messages correctly.
During the regression range Daily 2017-09-26 to 2017-09-28 we had various bustages so that I wasn't able to have a TB build after each M-C merge.

So we need to bisect this. I'm still assuming bug 1402888 is at fault, but that's not 100% sure.

Here is the timeline going going backwards:

2017-09-28 Daily  M-C - 76a26ef7c493311c170ae83eb0c1d6592a - bad
2017-09-28 Tinder M-C - 5ebe2e8980c6fd3ede2b6617bbbc4073dd - not tried.
  https://archive.mozilla.org/pub/thunderbird/tinderbox-builds/comm-central-win64/1506560615/
2017-09-27 Tinder M-C - 70158e4e215d784d1391db5e517b18727f -seems bad, I tried.
                  C-C - 523f36e9ebc5
  https://archive.mozilla.org/pub/thunderbird/tinderbox-builds/comm-central-win64/1506490639/
2017-09-26 Daily  M-C - bc56729898954e32d3a3731d03d178ed78 - good.

That didn't get me anywhere.

So someone with some patience and a fast machine please do this:

1) M-C at cb604e7830ec and C-C at 3f2554599137 - working?
2) M-C to 7e992ffa004c and C-C at 523f36e9ebc5, but additionally apply all three patches from bug 1404045. Working?
3) M-C to 7e992ffa004c and C-C at 5b7dc947139e, but additionally apply all three patches from bug 1404045. Working?

If 1) works and 2) or 3) don't when we know we still have to look for Necko problems from bug 1402888. When you're at 2) or 3) and it's not working, also apply the patch from bug 1404489.

If 2) or 3) works, then we need to look for M-C changes later than 7e992ffa004c. In this case I can give further instructions.

Richard, do you have a few hours to get those tests done? I do slow debug builds on a relatively slow machine and I don't want to block my machine for other work. I wish I had that fast Linux box on which clobber builds take 12 minutes.

I'm sure that you know that you simply do
  hg up -r cb604e7830ec in the mozilla directory for example.
Flags: needinfo?(richard.marti)
(In reply to Jorg K (GMT+2) from comment #7)
> 
> 1) M-C at cb604e7830ec and C-C at 3f2554599137 - working?

Already not working.
Flags: needinfo?(richard.marti)
Are you 100% sure?? That would mean that the issue we're seeing here is pre-bug 1402888.

Which brings the range to:
Last seen working:
2017-09-26 Daily  M-C - bc56729898954e32d3a3731d03d178ed78
                  C-C - 8f431a95645c

Not working: M-C at cb604e7830ec and C-C at 3f2554599137.

So ranges:
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=8f431a95645c&tochange=3f2554599137
So https://hg.mozilla.org/comm-central/rev/591d9466d703 could be at fault here. Let me back that out locally.

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=bc56729898954e32d3a3731d03d178ed78&tochange=cb604e7830ec
It seems that backing out https://hg.mozilla.org/comm-central/rev/591d9466d703 fixes the problem.

The only thing I don't understand? What broke it??

The only change in IMAP is:
-    leafName.Append(NS_LITERAL_STRING(FOLDER_SUFFIX));
+    leafName.AppendLiteral(FOLDER_SUFFIX);

and FOLDER_SUFFIX was made 16bit:
-#define FOLDER_SUFFIX ".sbd"
+#define FOLDER_SUFFIX u".sbd"
+#define FOLDER_SUFFIX8 ".sbd"
OK, problem found:
-#define SUMMARY_SUFFIX ".msf"
+#define SUMMARY_SUFFIX u".msf"
was OK, but it triggers a completely different RFind on:
https://dxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapMailFolder.cpp#255
 if (len > 4 && name.RFind(SUMMARY_SUFFIX, true) == len - 4)

Hmm, a little sad this. Patch coming.
So the team who broke it can fix it :-(
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8914009 - Flags: review?(acelists)
And let's not forget that Richard is the hero here. He descended into the dark dungeons of going back in time to bisect the problem. Thank you so much Richard, tomorrow's Daily will be usable again (unless, of course, there is more bustage over night).
Comment on attachment 8914009 [details] [diff] [review]
1404003-imap-download.patch (v1)

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

Thanks. It seems C++ is getting more JS-ish every day. A bool passed as int32_t should trigger a warning. It's not the old days in C where int was used as boolean.

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +251,5 @@
>  static bool
>  nsShouldIgnoreFile(nsString& name)
>  {
>    int32_t len = name.Length();
> +  // Warning: The first argument of RFind() needs to be a char string here to

'to' -> 'so' ?

@@ +254,5 @@
>    int32_t len = name.Length();
> +  // Warning: The first argument of RFind() needs to be a char string here to
> +  // that the second argument means 'ignorecase'. When passing a char16_t here,
> +  // a different overload is triggered and the argument is interpreted as offset.
> +  if (len > 4 && name.RFind(SUMMARY_SUFFIX8, true) == len - 4)

It would be nice if the 4 wasn't hardcoded. But I can do that in a followup.
Attachment #8914009 - Flags: review?(acelists) → review+
Thanks, I'll fix the typo before landing.

Yes, the 4 is plain silly. Why have a constant and then hard-code its name?
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/079662f36837
Bug 1402750 follow-up: char16_t triggers undesired RFind(). r=aceman
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 58.0
(In reply to :aceman from comment #14)
> @@ +254,5 @@
> >    int32_t len = name.Length();
> > +  // Warning: The first argument of RFind() needs to be a char string here to
> > +  // that the second argument means 'ignorecase'. When passing a char16_t here,
> > +  // a different overload is triggered and the argument is interpreted as offset.
> > +  if (len > 4 && name.RFind(SUMMARY_SUFFIX8, true) == len - 4)

Is this actually the same as StringEndsWith() ? Then we could avoid the ugliness.
Can that be case-insensitive? I guess. You also need to treat the
  name.SetLength(len-4); // truncate the string

Changes are, we'll never change the suffix, so there are more pressing issues :-(
Component: General → Networking: IMAP
OS: Unspecified → All
Product: Thunderbird → MailNews Core
Hardware: Unspecified → All
Version: unspecified → Trunk
Jörg, you are the hero to fix this. It wasn't so easy for you to find the culprit with this mass of bustages on different areas. 

Using now today Dily. Thank you very much!
That's true, considering the probable culprits were the *Stream interface changes and not a just some "trivial" string optimization patches.
Richard, you are the hero to find the problem. As you said, there was too much bustage to see the regression easily, so it needed a cool Swiss guy to do the dirty work bisecting the problem. I was the bad villain who broke it in the first place.

Daily is still broken with plaintext messages, so please be careful, see bug 1404581. Saved plaintext drafts are broken when editing the draft.
Tried Chat too and it still doesn't show any chat text. Should I file a new bug?
Yes, please.
Paenglab, you may want to check your local directory where the IMAP files for the account are stored on your disk. With the bug TB didn't see the existing data and .msf files and created new ones, downloading the messages into them. With the fix now TB is using the old files again. But the copies (even multiple ones) are left in the local directory so you may want to delete them now to reclaim the disk space.
Blocks: 1402750
Whiteboard: [regression:TB58]
You need to log in before you can comment on or make changes to this bug.