1592288 - The messages.query API fails when GloDa returns bad data

Status: RESOLVED FIXED

(Thunderbird :: Add-Ons: Extensions API, defect)

Description

[Mihai]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:70.0) Gecko/20100101 Firefox/70.0

Steps to reproduce:

Called the WebExtensions "messages.query" API with the "folder" & "fromDate" filters.

Actual results:

The operation fails with:

messageList.getNext(...) is null ext-mail.js:1503
_getNextPage chrome://messenger/content/parent/ext-mail.js:1503
startList chrome://messenger/content/parent/ext-mail.js:1442
query chrome://messenger/content/parent/ext-messages.js:212
AsyncFunctionNext self-hosted:689

It seems that this is happening when the expected result is smaller than messagesPerPage (100).

Expected results:

Return the results.

Comment 1

[Geoff Lankow (:darktrojan)]

This works for me, and our tests, so I don't think it's anything to do with the number of messages. I think it's more likely that one (or more) particular message is causing a problem, like if glodaMsg for some reason didn't have a folderMessage property here. Mihai, could you narrow it down to any particular message?

Comment 2

[Mihai]

(In reply to Geoff Lankow (:darktrojan) from comment #1)

Mihai, could you narrow it down to any particular message?

I have tried with the "query" API instead of "list" and adjusted the "fromDate" parameter in a divide & conquer manner until narrowing it down to one day, for which I have a single message in that folder. However, I see nothing special about that message, although I should also check the DB for it.

I'm now rebuilding global-messages-db.sqlite, just to see if that's it - this is a lengthy process, because its size was 1.8 GB.

Comment 3

[Mihai]

So you were right, it was Gloda related - rebuilding global-messages-db.sqlite did the trick. Thanks!

Do you think it's worth investigating any further (I still have the old global-messages-db.sqlite backed up), or just put it on DB corruption?

Comment 4

[Geoff Lankow (:darktrojan)]

It's probably a bug that Gloda should never be in this state, and that might be worth investigating but I doubt it. This API should be fixed to handle the faulty data though. I suspect the only thing it can do is ignore the message.

Comment 5

[Mihai]

(In reply to Geoff Lankow (:darktrojan) from comment #4)

It's probably a bug that Gloda should never be in this state, and that might be worth investigating but I doubt it.

Can provide me with the necessary SQLite query or relevant schema to check, based on the identified message (in the TB UI, I don't know it's DB ID), I will do it.

This API should be fixed to handle the faulty data though. I suspect the only thing it can do is ignore the message.

Makes sense to me.

Comment 6

[Mihai]

Mihai> I have tried with the "query" API instead of "list" and adjusted the "fromDate" parameter in a divide & conquer manner until narrowing it down to one day, for which I have a single message in that folder. However, I see nothing special about that message, although I should also check the DB for it.

I have checked Gloda and I was able to locate the corresponding message by the Message-Id, in the following tables:

• messages
• messagesText_content
• conversations (by the conversationID from messages)
• conversationsText_content
• folderLocations.

But all looks normal to me. I don't know how to interpret the contents of messages.jsonAttributes though.

Geoff, shall I dig any further? If yes, please provide the steps.

Comment 7

[Geoff Lankow (:darktrojan)]

You probably now know more about GloDa than I do. I haven't had time to look further into this yet, but I'll leave the NI? here to remind me to do so.

Comment 8

[Geoff Lankow (:darktrojan)]

It looks like there's lots of reasons why folderMessage could be null, so it definitely should be handled better. I suspect the reason your message is problematic is that it's somehow got disconnected from the folder it's really in, but I'm not going to investigate further.

Comment 9

[Geoff Lankow (:darktrojan)]

Attachment: 1592288-message-handle-bad-data-1.diff

This patch handles the situation two ways: it ignores null messages in a message list, and it prevents them being there in the first place (at least from this method).

Comment 10

[Mihai]

Thanks. Shouldn't there be some error logging in place, so that one can be aware that an incomplete set of messages was returned? Or is that the Gloda API's responsibility?

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9108868 [details] [diff] [review]
1592288-message-handle-bad-data-1.diff

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

r=mkmelin with the changes below

::: mail/components/extensions/parent/ext-messages.js
@@ +199,5 @@
>                onQueryCompleted(collection) {
> +                for (let glodaMsg of collection.items) {
> +                  if (glodaMsg.folderMessage) {
> +                    collectionArray.push(glodaMsg.folderMessage);
> +                  }

or perhaps still map but filter first?

@@ +201,5 @@
> +                  if (glodaMsg.folderMessage) {
> +                    collectionArray.push(glodaMsg.folderMessage);
> +                  }
> +                }
> +                resolve();

You're not resolving with a value now. I think that was nicer

Comment 12

[Geoff Lankow (:darktrojan)]

Attachment: 1592288-message-handle-bad-data-2.diff

How about this? (I blame Philipp for the use of Boolean!)

Comment 13

[Geoff Lankow (:darktrojan)]

(In reply to Mihai from comment #10)

Thanks. Shouldn't there be some error logging in place, so that one can be aware that an incomplete set of messages was returned? Or is that the Gloda API's responsibility?

IMO that's up to Gloda. It either shouldn't be broken or it should tell you when it is.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/97369b9ba582
Handle bad data in WebExt API message handling; r=mkmelin

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 9109364 [details] [diff] [review]
1592288-message-handle-bad-data-2.diff

Will put into TB 71 beta 4 and TB 68.3, OK?

Comment 16

[Geoff Lankow (:darktrojan)]

Okay.

Comment 17

[Jorg K (CEST = GMT+2)]

TB 71 beta 4:
https://hg.mozilla.org/releases/comm-beta/rev/004fc2991ce3d1e1b6f7c85e4540f69e9812e4ab

Comment 18

[Geoff Lankow (:darktrojan)]

68.3.1:
https://hg.mozilla.org/releases/comm-esr68/rev/a548a194c7abe5ce95751f08b2b25e8f19287614