345006 - Enabling an extension then switching to Themes tab crashes Minefield [@ nsXULDocument::ExecuteOnBroadcastHandlerFor][@ 0x02a1ff9a] [@ nsCOMPtr<nsIContent>::nsCOMPtr<nsIContent>]

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Jason Goble]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060714 Minefield/3.0a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060714 Minefield/3.0a1

When I open the Add-ons manager from the Tools menu and choose to enable the Talkback plugin and then try to select the Themes tab, Minefield crashes.  After program restart the Talkback plugin is enabled.  I can the go back to Add-Ins and move between the Extensions and Themes tabs freely.  I can then disable the Talkback plugin (no restart yet) and move between the Extensions and Themes tabs freely.  After restart, if I attempt to begin the sequence again, it will occur again.

Reproducible: Always

Steps to Reproduce:
Initial state:
Talkback disabled, default themes
1. Open Tools | Add-ons (presented with Extensions tab)
2. Choose to Enable Talkback Extension
3. Attempt to navigate to the Themes tab


Actual Results:  
Software crashes completely, however on restart of the application the talkback extension is marked as enabled.  I can freely navigate between the Extensions and Themes tabs, even after disabling the plugin once again (without restarting).

Expected Results:  
Expect to be able to freely navigate between the Extensions and Themes tabs.

Comment 1

[Robert Strong (they/them - no direct email)]

Try to reproduce in safe mode (e.g. command line argument of -safe-mode or the start menu shortcut). Can you reproduce this with other extensions? If so, please supply a talkback incident id.

Comment 2

[Jason Goble]

Attachment: Windows Stack Trace

Here is the stack trace as the application failed.

Comment 3

[Robert Strong (they/them - no direct email)]

This is wfm using today's nightly.

What other extensions do you have installed? Also, try to reproduce in safe mode, etc. per comment #1. Thanks

Comment 4

[Jason Goble]

(In reply to comment #3)
> This is wfm using today's nightly.
> 
> What other extensions do you have installed? Also, try to reproduce in safe
> mode, etc. per comment #1. Thanks
> 

I have executed the steps while in safe mode, the bug is still reproducible.  It does not generate a talkback incident ID or gather any information ... it just bombs.

I have no other extensions installed.  This is a fairly clean setup.  I DO have IE 7 Beta 2 installed, though that (I would think) would not have any effect on this.  I mention it only because it is the other "non-release" product I have installed currently.

Are there other extensions you would like me to try against this scenario?

Comment 5

[Robert Strong (they/them - no direct email)]

Try duplicating this by selecting disable instead of enable with talkback already enabled and then selecting themes. If that doesn't work try selecting enable then disable with talkback already enabled and then selecting themes.

Comment 6

[Robert Strong (they/them - no direct email)]

Also, you could try the same steps with DOM Inspector installed and talkback enabled. What we need to get is a talkback incident id.

Comment 7

[Jason Goble]

(In reply to comment #5)
> Try duplicating this by selecting disable instead of enable with talkback
> already enabled and then selecting themes. If that doesn't work try selecting
> enable then disable with talkback already enabled and then selecting themes.
> 

Attempted both scenarios, neither caused a crash.

Comment 8

[Jason Goble]

(In reply to comment #6)
> Also, you could try the same steps with DOM Inspector installed and talkback
> enabled. What we need to get is a talkback incident id.
> 

Can you give me instructions on the DOM Inspector, tried to install DOM Inspector 1.8 (https://addons.mozilla.org/firefox/1806/) and it reported incompatible with Minefield.  I'm turning in (I'm ET) and will check it out tomorrow if you can point me in the right direction. Thanks!

Comment 9

[Robert Strong (they/them - no direct email)]

DOM Inspector comes with the latest trunk nightly installer. Select Custom and on the optional components page select Developer Tools.

Comment 10

[Martijn Wargers (dead)]

It happened to me also (the crash), when trying the steps to reproduce.
But after that first time, it didn't happen afterwards anymore.
Using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060717 Minefield/3.0a1

Comment 11

[Robert Strong (they/them - no direct email)]

Can this be reproduced on the 1.8.1 branch?

Comment 12

[Robert Strong (they/them - no direct email)]

Also, a talkback incident id would be appreciated.

Comment 13

[Martijn Wargers (dead)]

That's kinda hard when talkback is disabled.

Comment 14

[Robert Strong (they/them - no direct email)]

True :P

The thing is if it is talkback specific then chances are great that the crash is outside the EM. If it only happens sporadically with talkback then it may also occur with a different extension.

Comment 15

[Jackie Liu]

This seems to happen when "enabling any extension then switching to the Themes tab".  Because of that, I am able to get a Talkback ID: TB22660032Y

Using "Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060829 Minefield/3.0a1".

Comment 16

[Adam Guthrie]

This is probably one of the weirdest stacks I've seen.

Incident ID: 22660032
Stack Signature	0x02a1ff9a 778d2d75
Product ID	FirefoxTrunk
Build ID	2006082905
Trigger Time	2006-08-29 13:27:17.0
Platform	Win32
Operating System	Windows NT 5.1 build 2600
Module	
URL visited	
User Comments	Enabling any extension then switching to Themes tab crashed Minefield. See Bug 345006.
Since Last Crash	90 sec
Total Uptime	90 sec
Trigger Reason	Access violation
Source File, Line No.	N/A
Stack Trace 	
0x02a1ff9a
nsCOMPtr<nsIContent>::nsCOMPtr<nsIContent>  [/mozilla\dist\include\xpcom\nscomptr.h, line 646]
0x029e1ac4
GkAtoms_info
nsDebugImpl::AddRef  [/mozilla\xpcom\base\nsdebugimpl.cpp, line 125]
0x68560c4d

Comment 17

[Robert Strong (they/them - no direct email)]

Has anyone seen this on the 1.8.1 branch?

Comment 18

[Jackie Liu]

I am also able to reproduce this bug in 'Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b2) Gecko/20060830 BonEcho/2.0b2".  Talkback ID: TB22701322M

This bug doesn't seem to only apply for Talkback.  In fact, when enabling any extension then going to the themes tab, the browser crashes.  Quite serious; nominating this to block Firefox 2. 

Comment 19

[Martijn Wargers (dead)]

That seems like a more useful stack to me.
From talkback ID: TB22701322M
0x026171e7
nsXULDocument::ExecuteOnBroadcastHandlerFor  [mozilla/content/xul/document/src/nsXULDocument.cpp, line 1017]
nsXULDocument::AttributeChanged  [mozilla/content/xul/document/src/nsXULDocument.cpp, line 1123]
nsXULElement::SetAttrAndNotify  [mozilla/content/xul/content/src/nsXULElement.cpp, line 1519]
nsXULElement::SetAttr  [mozilla/content/xul/content/src/nsXULElement.cpp, line 1440]
nsGenericElement::SetAttribute  [mozilla/content/base/src/nsGenericElement.cpp, line 1493]
XPTC_InvokeByIndex  [mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp, line 102]
XPCWrappedNative::CallMethod  [mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 2169]
XPC_WN_CallMethod  [mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp, line 1449]
js_Invoke  [mozilla/js/src/jsinterp.c, line 1350]
js_Interpret  [mozilla/js/src/jsinterp.c, line 4049]
js_Invoke  [mozilla/js/src/jsinterp.c, line 1369]
js_InternalInvoke  [mozilla/js/src/jsinterp.c, line 1448]
JS_CallFunctionValue  [mozilla/js/src/jsapi.c, line 4419]
nsJSContext::CallEventHandler  [mozilla/dom/src/base/nsJSEnvironment.cpp, line 1448]
nsJSEventListener::HandleEvent  [mozilla/dom/src/events/nsJSEventListener.cpp, line 195]
nsEventListenerManager::HandleEventSubType  [mozilla/content/events/src/nsEventListenerManager.cpp, line 1655]
nsEventListenerManager::HandleEvent  [mozilla/content/events/src/nsEventListenerManager.cpp, line 1762]
nsXULElement::HandleDOMEvent  [mozilla/content/xul/content/src/nsXULElement.cpp, line 2229]
nsXULCommandDispatcher::UpdateCommands  [mozilla/content/xul/document/src/nsXULCommandDispatcher.cpp, line 415]
XPTC_InvokeByIndex  [mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp, line 102]
XPCWrappedNative::CallMethod  [mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 2169]
XPC_WN_CallMethod  [mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp, line 1449]
js_Invoke  [mozilla/js/src/jsinterp.c, line 1350]
js_Interpret  [mozilla/js/src/jsinterp.c, line 4049]
js_Invoke  [mozilla/js/src/jsinterp.c, line 1369]
js_InternalInvoke  [mozilla/js/src/jsinterp.c, line 1448]
js_InternalGetOrSet  [mozilla/js/src/jsinterp.c, line 1508]
js_SetProperty  [mozilla/js/src/jsobj.c, line 3539]
js_Interpret  [mozilla/js/src/jsinterp.c, line 3799]
js_Invoke  [mozilla/js/src/jsinterp.c, line 1369]
js_InternalInvoke  [mozilla/js/src/jsinterp.c, line 1448]
JS_CallFunctionValue  [mozilla/js/src/jsapi.c, line 4419]
nsJSContext::CallEventHandler  [mozilla/dom/src/base/nsJSEnvironment.cpp, line 1448]
nsJSEventListener::HandleEvent  [mozilla/dom/src/events/nsJSEventListener.cpp, line 195]
nsEventListenerManager::HandleEventSubType  [mozilla/content/events/src/nsEventListenerManager.cpp, line 1655]
nsEventListenerManager::HandleEvent  [mozilla/content/events/src/nsEventListenerManager.cpp, line 1762]
nsXULElement::HandleDOMEvent  [mozilla/content/xul/content/src/nsXULElement.cpp, line 2229]
PresShell::HandleDOMEventWithTarget  [mozilla/layout/base/nsPresShell.cpp, line 6522]
nsButtonBoxFrame::DoMouseClick  [mozilla/layout/xul/base/src/nsButtonBoxFrame.cpp, line 182]
nsButtonBoxFrame::MouseClicked  [mozilla/layout/xul/base/src/nsButtonBoxFrame.h, line 61]
PresShell::HandleEventInternal  [mozilla/layout/base/nsPresShell.cpp, line 6464]
PresShell::HandleEventWithTarget  [mozilla/layout/base/nsPresShell.cpp, line 6321]
nsEventStateManager::CheckForAndDispatchClick  [mozilla/content/events/src/nsEventStateManager.cpp, line 3207]
nsEventStateManager::PostHandleEvent  [mozilla/content/events/src/nsEventStateManager.cpp, line 2170]
PresShell::HandleEventInternal  [mozilla/layout/base/nsPresShell.cpp, line 6495]
PresShell::HandleEvent  [mozilla/layout/base/nsPresShell.cpp, line 6259]
nsViewManager::HandleEvent  [mozilla/view/src/nsViewManager.cpp, line 2559]
nsViewManager::DispatchEvent  [mozilla/view/src/nsViewManager.cpp, line 2246]
HandleEvent  [mozilla/view/src/nsView.cpp, line 174]
nsWindow::DispatchEvent  [mozilla/widget/src/windows/nsWindow.cpp, line 1377]
nsWindow::DispatchMouseEvent  [mozilla/widget/src/windows/nsWindow.cpp, line 6336]
ChildWindow::DispatchMouseEvent  [mozilla/widget/src/windows/nsWindow.cpp, line 6583]
nsWindow::WindowProc  [mozilla/widget/src/windows/nsWindow.cpp, line 1565]
USER32.dll + 0x8734 (0x77d48734)
USER32.dll + 0x8816 (0x77d48816)
USER32.dll + 0x89cd (0x77d489cd)
USER32.dll + 0x8a10 (0x77d48a10)
nsAppShell::Run  [mozilla/widget/src/windows/nsAppShell.cpp, line 159]
nsAppStartup::Run  [mozilla/toolkit/components/startup/src/nsAppStartup.cpp, line 152]
main  [mozilla/browser/app/nsBrowserApp.cpp, line 61]
kernel32.dll + 0x16fd7 (0x7c816fd7)

Comment 20

[Adam Guthrie]

SWAG for the cause of the crash: in showview() in extensions.js we try to setAttribute on an element that somehow disappears out from under us.

Jackie, it would be very helpful if you could find when this crash started happening. Older builds can be found on http://archive.mozilla.org/pub/.

Comment 21

[Robert Strong (they/them - no direct email)]

I am still unable to reproduce. :(

Comment 22

[Robert Strong (they/them - no direct email)]

Jackie, could you please also try reproducing in safe mode?

Comment 23

[Jackie Liu]

I can reproduce this bug, safe mode or not, in "Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b2) Gecko/20060830 BonEcho/2.0b2" and "Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060830 Minefield/3.0a1".  

Could someone sufficiently empowered change the summary to say something along the lines of "Enabling an extension the switching to Themes tab crashes the browser", as the bug occurs both on the 2.0 branch and trunk using ANY extension, not just Talkback.

Comment 24

[Martijn Wargers (dead)]

Ok, done. Jackie, could you find a regression window for this bug?
You can find older builds here: http://archive.mozilla.org/pub/firefox/nightly/
That would be most helpful, since most of us can't reproduce this bug.

Comment 25

[Robert Strong (they/them - no direct email)]

I haven't been able to reproduce this on linux, win2k, winxp using latest trunk and 1.8.1 branch. Has anyone else reproduced this on branch?

Comment 26

[Jackie Liu]

Attachment: Crash in Minefield "Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060830 Minefield/3.0a1"

Comment 27

[Jackie Liu]

Crash in Bon Echo "Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b2) Gecko/20060830 BonEcho/2.0b2"

Screenshot: http://img176.imageshack.us/my.php?image=bonechotu8.png

Comment 28

[Mike Connor [:mconnor]]

not going to block on this unless we can get wider ability to reproduce.

Comment 29

[Adam Guthrie]

Jackie, could you please focus on getting a regression range? Screenshots like that really don't help much. :(

CCing Ria, who might be able to help us get a regression range if she can reproduce.

Comment 30

[Robert Strong (they/them - no direct email)]

note: after talking with Jackie via irc it appears to be limited to one system out of the two Jackie has.

btw: I asked for the screenshot in case it might provide a clue as to what stage it actually crashed.

Comment 31

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1b2) Gecko/20060831 BonEcho/2.0b2 ID:2006083103
Win2k sp4, AthlonXP2400

Well, I can accurately repo this bug. Just to be clear, here are the steps I use:

1. New profile, start firefox
2. Tools > Add-ons
3. Left click on Dom Inspector, Left click the 'Disable' button.
4. Dismiss addons manager with the window's [x] close button
5. Dismiss firefox with the window's [x] button
6. Relaunch firefox with the same profile
7. Tools > Add-ons
8. Left click on the Dom Inspector's 'Enable' button.
9. Left click on the Themes tab icon in the addons manager

This results in a crash for me. I looked for a regression range, and get

Works
- Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8) Gecko/20060502 BonEcho/2.0a1 ID:2006050203

Crash
- Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8) Gecko/20060503 BonEcho/2.0a1 ID:2006050304

The difference between these dates is that 20060502 doesn't have the new Add-ons UI, and 20060503 does.

My crashes for regression range find were as follows: TB22734329E TB22738087E TB22738286Y TB22738471W TB22738710X TB22738852X TB22739054Z TB22739318Y TB22739769H TB22740038Z You can see that I always crash with the same repo steps throughout this date range (20060831 --> 20060503)

Comment 32

[Jackie Liu]

I have reason to *suspect* the bug has been there ever since the "Extensions" and "Themes" dialogs were replaced with "Add-ons", which I believe is Build 2006050304

Comment 33

[Robert Strong (they/them - no direct email)]

I suspect the new ui exposed the bug but the bug has always been there.

Steve, I tried your steps 3 times without being able to reproduce.

Comment 34

[Ria Klaassen (not reading all bugmail)]

I see it in the 1.9a1_2006042620 build for the first time, but that's because the extensions in the AM have an Enable button. Previously there was no Enable button, only an option in the context-menu which does not crash the application. 

Comment 35

[Robert Strong (they/them - no direct email)]

OK... I have finally been able to reproduce it though why this occurs with some profiles and not others I haven't figured out yet.

-> taking

Comment 36

[Robert Strong (they/them - no direct email)]

The reason it is difficult to reproduce is that after the last selected view has been stored in localstore.rdf the bug goes away. This should be relatively easy and safe to fix.

re-requesting blocking‑firefox2

Comment 37

[Robert Strong (they/them - no direct email)]

we are crashing in richlistbox's clearSelection method

Comment 38

[Jackie Liu]

(In reply to comment #36)
> The reason it is difficult to reproduce is that after the last selected view
> has been stored in localstore.rdf the bug goes away. This should be relatively
> easy and safe to fix.
> 
> re-requesting blocking‑firefox2
> 

I second re-request for blocking firefox 2. 

Comment 39

[Mike Beltzner [:beltzner, not reading bugmail]]

Not blocking, since it's so hard to reproduce, but would take a patch.

Comment 40

[Robert Strong (they/them - no direct email)]

Attachment: patch - only ensure element is visible when there is a richlistbox last-selected attribute

Comment 41

[Robert Strong (they/them - no direct email)]

Attachment: better patch - if there isn't a selectedItem return early

ensureElementIsVisible will crash if we no longer have a selected list item so if the richlistbox doesn't have a selectedItem return early.

Comment 42

[(not reading, please use [e-mail] instead)]

robert, your patch seems reasonable (since it will prevent the crash), but what about the underlying bug (which has security implications)?

When we crash, is it because in nsXULDocument::ExecuteOnBroadcastHandlerFor() we are passing in garbage to do_QueryInterface()?  

Looking at the stack, we call nsXULDocument::ExecuteOnBroadcastHandlerFor() from nsXULDocument::AttributeChanged(), and in nsXULDocument::AttributeChanged() we do the same do_QueryInterface() on the same nsIDOMElement pointer.  But before we call nsXULDocument::ExecuteOnBroadcastHandlerFor(), we call listener->SetAttr() or listener->UnsetAttr().  

Could one of those methods have a side effect of releasing the nsIDOMElement that bl (or bl->mListener) was pointing to?

From nsXULDocument.cpp:

348 struct BroadcastListener {
349     nsIDOMElement*    mListener; // [WEAK] XXXwaterson crash waiting to happen!
350     nsCOMPtr<nsIAtom> mAttribute;
351 };

Comment 43

[Robert Strong (they/them - no direct email)]

from a quick glance that makes sense. For this bug I think this is the right thing to do and to open a new bug for the "XXXwaterson crash waiting to happen!"

Comment 44

[(not reading, please use [e-mail] instead)]

Comment on attachment 236669 [details] [diff] [review]
better patch - if there isn't a selectedItem return early

oops, sorry robert, I forgot the r=sspitzer on your patch.

can you log a new bug on the problem with the underlying crash?

Comment 45

[(not reading, please use [e-mail] instead)]

robert, would the EM in 1.5.0.x crash in this scenario too?  (If so, and it fixes it, what about checking your one liner to the branch for 1.5.0.7?)

Comment 46

[Adam Guthrie]

Seth, I don't think it would. See comment 34.

Comment 47

[Robert Strong (they/them - no direct email)]

Filed Bug 351347.

Comment 48

[Robert Strong (they/them - no direct email)]

(In reply to comment #45)
> robert, would the EM in 1.5.0.x crash in this scenario too?  (If so, and it
> fixes it, what about checking your one liner to the branch for 1.5.0.7?)
It doesn't since this code was added to handle the case where the list item extends below the bottom due to a status message being displayed which increases the height of the list item... so, this only applies to the new add-ons mgr

Comment 49

[(not reading, please use [e-mail] instead)]

>> robert, would the EM in 1.5.0.x crash in this scenario too?
> It doesn't...

Thanks for clarifying and for logging the spin off bug.

Comment 50

[Robert Strong (they/them - no direct email)]

Checked in to trunk

Comment 51

[Robert Strong (they/them - no direct email)]

Comment on attachment 236669 [details] [diff] [review]
better patch - if there isn't a selectedItem return early

Safe and simple fix for this crasher. The patch just returns early we no longer have a selectedItem so we don't call ensureElementIsVisible on an element that no longer exists

Comment 52

[Mike Schroepfer]

Comment on attachment 236669 [details] [diff] [review]
better patch - if there isn't a selectedItem return early

a=schrep for drivers for crash fix.

Is there a separate bug filed about ensureElementIsVisible not crashing on an empty element?

Comment 53

[Robert Strong (they/them - no direct email)]

Should be covered by Bug 351347.

Comment 54

[Robert Strong (they/them - no direct email)]

Checked in to MOZILLA_1_8_BRANCH

Comment 55

[Stephen Donner [:stephend] Not actively reading bugmail]

https://litmus.mozilla.org/show_test.cgi?id=5004

in-litmus+ (not sure if this testcase covers it 100%, but it's a good testcase in general)

Comment 56

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008011005 Minefield/3.0b3pre