686123 - Calling openURL on an uninitialized nsIMessenger crashes TB

Status: NEW

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

Description

[Ben Lerner]

Attachment: Click the menu item to crash TB

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.

Comment 1

[Jonathan Protzenko [:protz]]

Attachment: Tools > Crash TB

Previous xpi had a wrong structure.

Comment 2

[Wayne Mery (:wsmwk)]

(keyword + severity)

one of these? https://crash-stats.mozilla.com/query/query?product=Thunderbird&version=ALL%3AALL&range_value=3&range_unit=days&date=09%2F10%2F2011+08%3A29%3A23&query_search=signature&query_type=contains&query=URL&reason=&build_id=&process_type=any&hang_type=any&do_query=1

Comment 3

[Ben Lerner]

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.

Comment 4

[Wayne Mery (:wsmwk)]

Ben, like https://crash-stats.mozilla.com/report/index/6da366de-f9c3-4234-979f-640372110905 ?

Comment 5

[Ben Lerner]

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.

Comment 6

[Wayne Mery (:wsmwk)]

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

Comment 7

[Wayne Mery (:wsmwk)]

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

Comment 8

[Ludovic Hirlimann [:Usul]]

Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.

Comment 9

[Wayne Mery (:wsmwk)]

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

Comment 10

[Ben Lerner]

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

Comment 11

[Wayne Mery (:wsmwk)]

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?

Comment 12

[Wayne Mery (:wsmwk)]

The testcase addon won't install - manifest?

Comment 13

[John Bieling (:TbSync)]

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

Comment 14

[Wayne Mery (:wsmwk)]

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

Comment 15

[Ben 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!)

Comment 16

[John Bieling (:TbSync)]

Attachment: crashtb_v3.xpi

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

Comment 17

[John Bieling (:TbSync)]

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.