Closed Bug 1663859 Opened 4 years ago Closed 3 years ago

Crash when opening an PDF attachment from IMAP [@ nsXPCWrappedJS::CallMethod ]

Categories

(Thunderbird :: General, defect, P1)

Thunderbird 82
Unspecified
All

Tracking

(thunderbird91+ fixed)

RESOLVED FIXED
92 Branch
Tracking Status
thunderbird91 + fixed

People

(Reporter: mkmelin, Assigned: lasana)

References

Details

(Keywords: crash, topcrash-thunderbird)

Crash Data

Attachments

(3 files)

Got a recurring crash which seems to be related to having a tab show a pdf attachment from an imap mail, when that tab is being restored at startup.

https://crash-stats.mozilla.org/report/index/bee0c084-bfc6-45bf-b823-587740200909
MOZ_RELEASE_ASSERT(NS_IsMainThread()) (nsXPCWrappedJS::CallMethod called off main thread)

https://searchfox.org/comm-central/source/mailnews/imap/src/nsImapService.cpp#2367 ->
https://searchfox.org/comm-central/rev/1b97640106e37ba9a0a208f937b8396de9ac89d9/mailnews/base/src/nsMsgAccountManager.cpp#1659

? Related to Bug 1175168 - crash nsXPCWrappedJS::CallMethod - need to proxy URL release (JavaScript's XPCOM object) to the main thread in the IMAP protocol (Not import related)

Keywords: crash
Version: unspecified → Thunderbird 82

This bug has been affecting me for several months, i'm using thunderbird 84 on windows.

This crashes occur very frequently when the session is reopening multiple pdf documents of the previous session. Deleting session.json is a manual workaround to get out of the restarting-crash-restarting-crash-... loop

PDF attachment viewing is a great improvement to Thunderbird, but it comes with this recurring downside...

(In reply to germinolegrand from comment #2)

This bug has been affecting me for several months, i'm using thunderbird 84 on windows.

What are your crash IDs?

Flags: needinfo?(germinolegrand)

(In reply to Wayne Mery (:wsmwk) from comment #3)

(In reply to germinolegrand from comment #2)

This bug has been affecting me for several months, i'm using thunderbird 84 on windows.

What are your crash IDs?

Here they are :

bp-4747248b-9bb6-4414-8e77-d9bd00201229 29/12/2020 à 10:14
bp-a11068fa-4fe3-452b-8b9b-03e930201229 29/12/2020 à 10:13
bp-180ee1bd-1641-4c1c-b9e9-ce4ca0201202 02/12/2020 à 17:19
bp-c3abced2-128a-4790-8676-6e9520201202 02/12/2020 à 17:18
bp-ae28d9c9-057c-4fa6-9b4b-13d6f0201202 02/12/2020 à 17:17
bp-49b99b5f-e31e-48d2-8d19-c5e860201202 02/12/2020 à 17:17
bp-546992f3-8ee7-41f5-a905-9498d0201202 02/12/2020 à 17:17
bp-8ce0c0af-1e8a-4ef1-bc84-69ccb0201202 02/12/2020 à 17:16
bp-5cfd5cba-6776-42bf-9e67-39c840201001 01/10/2020 à 23:06
bp-299108d1-93c4-4fd1-8fad-8060c0201001 01/10/2020 à 22:50
bp-37cccc5e-12e0-4f47-ac33-414750201001 01/10/2020 à 22:50
bp-9c40cf76-0534-4fd1-9a9c-060770201001 01/10/2020 à 22:49
bp-8ad608b4-ee13-4c81-b364-1fc0f0201001 01/10/2020 à 22:49
bp-8c9a8287-5992-4703-a009-63d3e0201001 01/10/2020 à 22:49
bp-fd32eaf1-ad94-4971-bb7d-3def90201001 01/10/2020 à 22:49
bp-80015257-2452-4393-bffb-25db80201001 01/10/2020 à 22:49
bp-737c6741-8892-4ddc-aa8a-3c93d0201001 01/10/2020 à 22:48
bp-b8d433de-c6a2-470a-b7f9-a5e270201001 01/10/2020 à 22:47
bp-50e05486-d231-43ec-ab04-49a790201001 01/10/2020 à 22:47
bp-337ddfb5-2bf4-43b6-8041-fb2230201001 01/10/2020 à 22:47
bp-63bc1ab5-278c-49d1-937a-117ba0201001 01/10/2020 à 22:47
bp-de96e617-c3d5-4c2c-b4dd-d696f0201001 01/10/2020 à 22:40
bp-9b93ae57-bd67-42f6-bb8c-fcadb0201001 01/10/2020 à 22:39
bp-0c9ad86d-3480-424f-86ff-a24960201001 01/10/2020 à 22:39
bp-3fe3d480-647d-4584-b211-0dde00201001 01/10/2020 à 22:37
bp-4cc7fba4-9efa-46bf-a68d-5df690201001 01/10/2020 à 22:37
bp-3fcd25e6-2627-4c6e-a76d-9d0cd0201001 01/10/2020 à 22:30

Flags: needinfo?(germinolegrand)

This bug is at least somewhat reproducible. Unless there are other ideas, maybe we shouldn't restore pdf tabs, and thus avoiding the problem.

Priority: -- → P2

From the looks of the stacks, the assertions are happening on the DOM worker thread. I don't know exactly what that is, but here's what I guess is happening:

This is mainly based on bp-bee0c084-bfc6-45bf-b823-587740200909

At that stage, we're probably trying to access something from javascript, but because we're in the DOM worker thread, we fail.

Interestingly, later in NS_NewMailnewsURI, we do some main thread checking and throw across to the main thread if necessary: https://searchfox.org/comm-central/rev/bd4526e24041709d3b988ecaca782e4a19982dd9/mailnews/base/src/nsNewMailnewsURI.cpp#111-117

Maybe something like that would work for the whole call?

It might be worth checking my analysis with someone from the gecko team, they might understand why this is happening on the DOM worker thread, and the correct way to handle this.

Crash Signature: [@ nsXPCWrappedJS::CallMethod ]
Summary: crash [@ nsXPCWrappedJS::CallMethod ] on startup restoring tab with pdf attachment from imap → Thuderbird crash [@ nsXPCWrappedJS::CallMethod ] on startup restoring tab with pdf attachment from imap

Hello, now i get this crash absolutely every time i open a pdf attachment since 89.0b3 or 89.0b4. Still here in 90.0b1. Very easy to reproduce on my side, but i can't open any pdf within thunderbird...

(In reply to germinolegrand from comment #8)

Hello, now i get this crash absolutely every time i open a pdf attachment since 89.0b3 or 89.0b4. Still here in 90.0b1. Very easy to reproduce on my side, but i can't open any pdf within thunderbird...

Before you ask :
https://crash-stats.mozilla.org/report/index/e4852fe1-9600-4e99-afdc-492c80210604
https://crash-stats.mozilla.org/report/index/fd6e33ed-d852-41da-8c02-d52ec0210604

Yeah I think this bug morphed, and it's no longer only for restore. Basically all pdf opening crash for me nowadays.

MOZ_RELEASE_ASSERT(NS_IsMainThread()) (nsXPCWrappedJS::CallMethod called off main thread)

Priority: P2 → P1

(In reply to Magnus Melin [:mkmelin] from comment #10)

Basically all pdf opening crash for me nowadays.

Is it as simple as opening any/every old pdf? Or are steps to reproduce slightly more complicated?

I don't crash when opening a pdf attachment on Mac.* I wonder why.

Flags: needinfo?(mkmelin+mozilla)

(In reply to Mark Banner (:standard8) from comment #7)

...
It might be worth checking my analysis with someone from the gecko team, they might understand why this is happening on the DOM worker thread, and the correct way to handle this.

Who is the person to ask?

(In reply to Wayne Mery (:wsmwk) from comment #12)

(In reply to Magnus Melin [:mkmelin] from comment #10)

Basically all pdf opening crash for me nowadays.

Is it as simple as opening any/every old pdf? Or are steps to reproduce slightly more complicated?

I don't crash when opening a pdf attachment on Mac.* I wonder why.

Any pdf of any sort for me on windows. Just opening it. Always.

The STR are to open any PDF that's attached to an email on IMAP. Doesn't crash if it's from a local folder.

Flags: needinfo?(mkmelin+mozilla)
Attached file attached pdf.eml (deleted) —

Simple test case. Copy to IMAP, try to open the PDF. Crash.

Possibly what we should do is for IMAP and maybe NNTP, get the messages and put it into a data URI before passing it off to get opened.
fetch() on the imap url could work.

Code should be around here: https://searchfox.org/comm-central/rev/b972d9ae1bd20536044be70c5bf3670e70b40720/mail/base/content/msgHdrView.js#1931

Attached file WIP: bug1663859_pdf_dataurl.patch (deleted) —

WIP - the basic idea seems ok (pdfs do open for local messages), but needs more work.

Lasana, can you take it from here?

Assignee: nobody → lasana

Might this cause an issue with our session restore code, especially for large PDFs? I'm guessing that would require saving the full data url in the restore file, which would then need to be loaded on startup potentially causing performance issues if we're doing it in a blocking fashion.

Could the real issue be that IMAP hasn't fully started by the time we're trying to restore the tab, and we should wait on something before we do that?

It used to crash for session restore, but it's mutated to being always for imap. (Updating the summary now.)

Session restore for a data: url could be somewhat problematic. OTOH, it's probably loading faster in total than trying to restore a large PDF over IMAP.

Summary: Thuderbird crash [@ nsXPCWrappedJS::CallMethod ] on startup restoring tab with pdf attachment from imap → Thuderbird crash [@ nsXPCWrappedJS::CallMethod ] when opening pdf attachment from imap

Was this fixed already? I can open pdf files without crashing. Or is it not happening on Linux?

Flags: needinfo?(mkmelin+mozilla)

I tried it earlier today on linux, with the test case above copied to a gmail account on imap. Crashed instantly. Of course pdfjs needs to be enabled (which it is by default).

Flags: needinfo?(mkmelin+mozilla)

Comment on attachment 9226773 [details]
WIP: bug1663859_pdf_dataurl.patch

Fwiw, i'm seeing the crash too on OpenBSD with 90.b1 (and since some betas) when opening pdfs attached to mails.

With this patch it no longer crashes but doesnt open the file, and i have this in the error console:

Uncaught (in promise) TypeError: Window.fetch: imap://user@server:143/fetch%3EUID%3E/INBOX%3E36273?part=1.2&filename=Acc%C3%A8s%20T%C3%A9l%C3%A9com%20CRAIG%20V1.2%20%28TVA%29.pdf is an url with embedded credentials.
    open chrome://messenger/content/msgHdrView.js:1942
msgHdrView.js:1942:23

Yep it's wip and not expected to work for imap messages.

I still can't reproduce this, I tried with a debug build, with some of the extensions I saw installed but to no avail yet.

We also see PDF crashes, and from what I understood: The PDF renderer should be running in a content process (not IMAP thread, and not main thread, not main process), but for that to work, the data for the URL needs to be send over the process boundary.

After some debugging we followed Mark's analysis from comment #7 and came up with this. It fixes the crash:
https://github.com/Betterbird/thunderbird-patches/blob/main/91/bugs/1663859-imap-uri-main-tread-crash-fix.patch

Great! I'm changing the assignee since you already got something.

Assignee: lasana → zoe

Thanks for the honour, but there seems to be a misunderstanding. Betterbird is a Thunderbird fork and we're giving back to upstream (as any good downstream project should do). We're not planning to be involved in your project above that (and we'd also like to keep comments to a minimum, ideally one per bug, so it doesn't look like we'd be advertising our stuff). If you like the patch, please take it, and we'd appreciate if you preserved the authorship like we have acknowledged Mark's analysis/idea in the commit message.

Assignee: zoe → nobody

Thanks, Zoe! This is the biggest TB 91 blocker right now. I appreciate you looking into it and fixing it.

The fix seems to be the same code as Neil did for Owl in bug 1555633 at the end of the same file.

Minor code NITs (doesn't change functionality at all, just easier to read code):

  • Either move the inline function into the else block, where it's used, or use the function NewURI also in the if block, like: NewURI(); return rv;

@Lasana: Can you take this bug back, please?

Severity: -- → S2
Flags: needinfo?(lasana)
Summary: Thuderbird crash [@ nsXPCWrappedJS::CallMethod ] when opening pdf attachment from imap → Crash when opening an PDF attachment from IMAP [@ nsXPCWrappedJS::CallMethod ]

Minor code NITs ...

Copied what was there (wondering why it was done like that and marvelling at the C++ syntax).

(In reply to Zoe Martin from comment #30)

Thanks for the honour, but there seems to be a misunderstanding. Betterbird is a Thunderbird fork and we're giving back to upstream (as any good downstream project should do). We're not planning to be involved in your project above that ...

Thanks for letting me know.

@Lasana: Can you take this bug back, please?

I have not been able to reproduce this bug for some reason so I can't test if the patch works or not. My cpp is still basic so I really can't tell if it's doing the right thing or not by just looking.

Flags: needinfo?(lasana)

I'm not 100% sure my testing is right, but porting https://github.com/Betterbird/thunderbird-patches/blob/main/91/bugs/1663859-imap-uri-main-tread-crash-fix.patch on top of 90.0b3 self built on OpenBSD allows me to open PDF attachments in a thunderbird tab, from a mail on an IMAP connection (eg the tab URL is imap://landry@localhost:1143/fetch%3EUID%3E.INBOX%3E27962?part=1.2&filename=Proc%C3%A9dure%20d%27inscription.pdf&type=application/pdf). That's not on the computer when i was seeing thunderbird crashing (with 90.0b3 unpatched) when opening PDF attachments, where i'll be able to test monday.

OS: Unspecified → All

tested with 90.0b3 on my work laptop (Also OpenBSD) where i've had crashes opening PDFs since some betas, can now definitely open PDF attachments from mails stored on IMAP. Definitely worth improvement, especially if that's a blocker for 91 !

Assignee: nobody → lasana
Status: NEW → ASSIGNED
Attachment #9230776 - Attachment description: Bug 1663859 - Move all IMAP URI creation onto the main thread (analysis by Mark Banner). r=mkmelin → Bug 1663859 - Move all IMAP URI creation onto the main thread. r=mkmelin
Target Milestone: --- → 92 Branch

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/76ece33df0c5
Move all IMAP URI creation onto the main thread. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED

Thank you!

Pushed by geoff@darktrojan.net: https://hg.mozilla.org/comm-central/rev/0ae34b27f11a follow-up - Add a missing line to unbreak the build. rs=bustage-fix

#1 crash for 91.0b1

Comment on attachment 9230776 [details]
Bug 1663859 - Move all IMAP URI creation onto the main thread. r=mkmelin

[Triage Comment]
Approved for beta. Has had light testing.

Attachment #9230776 - Flags: approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: