Closed Bug 686123 Opened 13 years ago Closed 8 months ago

Calling openURL on an uninitialized nsIMessenger crashes TB

Categories

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

x86_64
Windows 7
defect

Tracking

(Not tracked)

RESOLVED INVALID

People

(Reporter: benjamin.lerner, Unassigned)

Details

(Keywords: crash, reproducible, testcase, Whiteboard: [rare])

Crash Data

Attachments

(2 files, 2 obsolete files)

Attached file Click the menu item to crash TB (obsolete) —
While working on an extension, using only scriptable methods from JS I managed to crash TB hard, despite a JS try/catch block. If you uncomment the setWindow call in overlay.js, the crash is avoided.
Attachment #559691 - Attachment mime type: application/octet-stream → application/x-xpinstall
Component: General → Database
Product: Thunderbird → MailNews Core
QA Contact: general → database
Attached file Tools > Crash TB (obsolete) —
Previous xpi had a wrong structure.
Attachment #559691 - Attachment is obsolete: true
Status: UNCONFIRMED → NEW
Ever confirmed: true
I don't think it has anything to do with parsing URLs -- the intial crash I saw was with a completely valid URL, obtained by nsIMsgDBView::GetURIForViewIndex(some number). And when I uncomment the setWindow line in the xpi, the crash goes away, even with the invalid "mailbox://anything" URL that's currently there. I think the crash has to do with the fact that in http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMessenger.cpp#518, nsMessenger::OpenURL, it calls AddMsgUrlToNavigateHistory, which dereferences mMsgWindow, which has never been set and is therefore null.
Possibly, yeah -- that crash is from an older version of TB than I used (I tested on 5 or 6, whichever was current a few weeks ago, and that crash is from TB3.0.4), so the line numbers are a bit different. Also, the crash I create in my test extension doesn't require stack frames 2-4 of this trace; it just jumps straight to the call to OpenURL, and then into AddMsgToUrlNavigateHistory, and then into oblivion when it dereferences mMsgWindow. So it's probably crashing on the same bug in this trace as in attachment 559692 [details] above, but I suspect that there's another bug lurking in that trace: somehow, it must have created an incomplete nsIMessenger. Reading through nsMsgDBView, the messenger being used on line 1025 comes from a weak ref stored in the nsMsgDBView, which was initialized in either Init or CopyDBView...but since neither of those are on the stack trace, I can't tell where it came from. It looks like both of those initializers are ok in and of themselves, but there's not enough information to tell whether the messenger that was passed in was improperly initialized.
nsMessenger::AddMsgUrlToNavigateHistory(nsACString_internal const&) bp-7df65635-3e86-4caf-94ab-d02902111006 EXCEPTION_ACCESS_VIOLATION_READ 0x0 0 xul.dll nsMessenger::AddMsgUrlToNavigateHistory mailnews/base/src/nsMessenger.cpp:503 1 xul.dll nsMessenger::OpenURL mailnews/base/src/nsMessenger.cpp:529 2 xul.dll NS_InvokeByIndex_P xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:102 3 xul.dll XPCWrappedNative::CallMethod js/src/xpconnect/src/xpcwrappednative.cpp:2334 4 xul.dll XPC_WN_CallMethod js/src/xpconnect/src/xpcwrappednativejsops.cpp:1629 5 mozjs.dll js::InvokeKernel js/src/jsinterp.cpp:630 only 3 other examples in crash-stats from last two months. bp-04e7f018-0786-4250-a3b9-82f5a2110831 bp-7e8b9069-5624-4d03-8a0f-cec042110826 bp-2bc7fde7-649d-43e4-94d2-6bc2c2110813
Crash Signature: [@ nsMessenger::AddMsgUrlToNavigateHistory(nsACString_internal const&) ]
Keywords: stackwanted
retested. resulting crash bp-0e465a52-912d-424f-aa92-676832120708 somewhat rare. all these examples have only testpilot installed bp-9c2569fb-fe46-4783-ad80-688472120614 bp-d732ca6c-2d47-4977-a05b-4211a2120707 bp-cf3c9769-8516-44c3-8575-994842120616 startup bp-999247a4-545f-4426-8fc0-bf1302120622 no correlations, no comments
Whiteboard: [rare]
Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.
Crash Signature: [@ nsMessenger::AddMsgUrlToNavigateHistory(nsACString_internal const&) ] → [@ nsMessenger::AddMsgUrlToNavigateHistory(nsACString_internal const&) ] [@ nsMessenger::AddMsgUrlToNavigateHistory ]

Ben, can you still reproduce or still have the addon which caused this?

Still rare, average only one per week

bp-51e724f2-db60-403e-a968-262a10190506

0 xul.dll nsMessenger::AddMsgUrlToNavigateHistory(nsTSubstring<char> const&) comm/mailnews/base/src/nsMessenger.cpp:442 context
1 xul.dll nsMessenger::OpenURL(nsTSubstring<char> const&) comm/mailnews/base/src/nsMessenger.cpp:471 cfi
2 xul.dll nsMsgDBView::LoadMessageByViewIndex(unsigned int) comm/mailnews/base/src/nsMsgDBView.cpp:1228 cfi
3 xul.dll nsMsgGroupView::LoadMessageByViewIndex(unsigned int) comm/mailnews/base/src/nsMsgGroupView.cpp:1056 cfi
4 xul.dll nsMsgDBView::SelectionChanged() comm/mailnews/base/src/nsMsgDBView.cpp:1321 cfi
5 xul.dll NS_InvokeByIndex xpcom/reflect/xptcall/md/win32/xptcinvoke_asm_x86_msvc.asm:54 cfi

Flags: needinfo?(benjamin.lerner)

Yes -- :protz' addon attached above instantly crashes TB60.6.1. I just installed it, ran it, and submitted a crash trace.

Flags: needinfo?(benjamin.lerner)

Thanks. So bp-2bb4ac11-dc15-4380-a7b8-4504d0190512 2019-05-12 15:39:22 and bp-9786f305-2d9e-44d3-9272-23bf60190512

I missed that we have the add-on attached - and so a testcase!
Does this put the finger on the cause?

Flags: needinfo?(benc)
Severity: critical → normal
Flags: needinfo?(benc)

The testcase addon won't install - manifest?

Attached file crashtb_v2.xpi

Updated version of the crasher add-on. It adds a browser_action_button "Crash Me" to Thunderbirds main toolbar.

You should restart TB once after installing the add-on, because crashing TB directly removed the button from the toolbar for me (maybe some xulstore has not finished saving the new icon before the crash happend).

Attachment #559692 - Attachment is obsolete: true
Severity: normal → S3

Does this still crash?
There are no reports on crash-stats.

Flags: needinfo?(benjamin.lerner)

Is there an updated version of the extension to the new manifest format? (I haven't kept up with how to write extensions any more, sorry!)

Flags: needinfo?(benjamin.lerner)
Component: Database → Add-Ons: Extensions API
Product: MailNews Core → Thunderbird
Attached file crashtb_v3.xpi

Updated version (v3) runs in TB115+ and does not crash.

Wayne, how should we move this forward? We usually do not add tests for Experiment code, and Thunderbird itself does not cause this crash.

Thunderbird does not even use this function anymore:
https://searchfox.org/comm-central/search?q=.openUrl%28&path=&case=false&regexp=false

For me, this entire bug is invalid by now. We provide all the different ways to open tabs/windows/messages through WebExtension APIs.

Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: