Closed Bug 1592288 Opened 4 months ago Closed 3 months ago

The messages.query API fails when GloDa returns bad data

Categories

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

defect
Not set

Tracking

(thunderbird_esr68 fixed, thunderbird71 fixed, thunderbird72 fixed)

RESOLVED FIXED
Thunderbird 72.0
Tracking Status
thunderbird_esr68 --- fixed
thunderbird71 --- fixed
thunderbird72 --- fixed

People

(Reporter: mihaicodrean, Assigned: darktrojan)

Details

Attachments

(1 file, 1 obsolete file)

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.

Flags: needinfo?(geoff)

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?

Flags: needinfo?(geoff) → needinfo?(mihaicodrean)

(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.

Flags: needinfo?(mihaicodrean)

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?

Flags: needinfo?(geoff)

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.

Flags: needinfo?(geoff)
Summary: The messages.query API fails when the expected result is smaller than messagesPerPage → The messages.query API fails when GloDa returns bad data

(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.

Flags: needinfo?(geoff)

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.

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.

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.

Flags: needinfo?(geoff)

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).

Assignee: nobody → geoff
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #9108868 - Flags: review?(mkmelin+mozilla)

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 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
Attachment #9108868 - Flags: review?(mkmelin+mozilla) → review+

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

(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.

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/97369b9ba582
Handle bad data in WebExt API message handling; r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 72.0
Attachment #9109364 - Flags: review+
Attachment #9109364 - Flags: approval-comm-esr68?
Attachment #9109364 - Flags: approval-comm-beta?
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?
Attachment #9109364 - Flags: approval-comm-beta? → approval-comm-beta+

Okay.

Attachment #9109364 - Flags: approval-comm-esr68? → approval-comm-esr68+
Attachment #9108868 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.