763162 - crash in nsXULMenuitemAccessible::NativeInteractiveState @ nsMenuFrame::IsOnMenuBar

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Scoobidiver (away)]

It first appeared in 16.0a1/20120605 and 15.0a2/20120608. The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dd6ec482a85d&tochange=a7a905fd70d5
It's likely a regression from bug 756983.

Signature 	nsMenuFrame::IsOnMenuBar() More Reports Search
UUID	abf38ffb-009a-4414-92b5-f22e82120609
Date Processed	2012-06-09 07:33:34
Uptime	824
Last Crash	14.7 minutes before submission
Install Age	3.1 hours since version was first installed.
Install Time	2012-06-09 04:28:35
Product	Firefox
Version	16.0a1
Build ID	20120608081921
Release Channel	nightly
OS	Windows NT
OS Version	6.1.7600
Build Architecture	x86
Build Architecture Info	GenuineIntel family 6 model 23 stepping 10
Crash Reason	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address	0x80
App Notes 	
AdapterVendorID: 0x10de, AdapterDeviceID: 0x0a23, AdapterSubsysID: 34f21458, AdapterDriverVersion: 8.17.11.9646
D3D10 Layers? D3D10 Layers- D3D9 Layers? D3D9 Layers- 
EMCheckCompatibility	True	
Total Virtual Memory	2147352576
Available Virtual Memory	1694928896
System Memory Use Percentage	78
Available Page File	2474278912
Available Physical Memory	456781824

Frame 	Module 	Signature 	Source
0 	xul.dll 	nsMenuFrame::IsOnMenuBar 	layout/xul/base/src/nsMenuFrame.h:168
1 	xul.dll 	nsXULMenuitemAccessible::NativeInteractiveState 	accessible/src/xul/nsXULMenuAccessible.cpp:123
2 	xul.dll 	Accessible::NativeState 	accessible/src/generic/Accessible.cpp:665
3 	xul.dll 	nsXULMenuitemAccessible::NativeState 	accessible/src/xul/nsXULMenuAccessible.cpp:50
4 	xul.dll 	nsXULMenuSeparatorAccessible::NativeState 	accessible/src/xul/nsXULMenuAccessible.cpp:385
5 	xul.dll 	Accessible::State 	accessible/src/generic/Accessible.cpp:1512
6 	xul.dll 	AccessibleWrap::get_accState 	accessible/src/msaa/AccessibleWrap.cpp:443
7 	rpcrt4.dll 	Invoke 	
8 	rpcrt4.dll 	NdrStubCall2 	
9 	ole32.dll 	ChannelWrapper_QueryInterface 	
10 	oleaut32.dll 	CUnivStubWrapper::Invoke 	
11 	ole32.dll 	SyncStubInvoke 	
12 	ole32.dll 	StubInvoke 	
13 	ole32.dll 	CCtxComChnl::ContextInvoke 	
14 	ole32.dll 	MTAInvoke 	
15 	ole32.dll 	STAInvoke 	
16 	ole32.dll 	AppInvoke 	
17 	ole32.dll 	ComInvokeWithLockAndIPID 	
18 	ole32.dll 	ComInvoke 	
19 	ole32.dll 	ThreadDispatch 	
20 	ole32.dll 	ThreadWndProc 	
21 	user32.dll 	InternalCallWinProc 	
22 	user32.dll 	UserCallWinProcCheckWow 	
23 	user32.dll 	DispatchMessageWorker 	
24 	user32.dll 	DispatchMessageW 	
25 	xul.dll 	nsAppShell::ProcessNextNativeEvent 	widget/windows/nsAppShell.cpp:322
26 	nspr4.dll 	nspr4.dll@0x28df 	
27 	xul.dll 	nsBaseAppShell::OnProcessNextEvent 	widget/xpwidgets/nsBaseAppShell.cpp:298
28 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:586
...

More reports at:
https://crash-stats.mozilla.com/report/list?signature=nsMenuFrame%3A%3AIsOnMenuBar%28%29
https://crash-stats.mozilla.com/report/list?signature=nsXULMenuitemAccessible%3A%3ANativeInteractiveState

Comment 1

[alexander :surkov (:asurkov)]

Mark, would you like to take this? simple null check

Comment 2

[Mark Capella [:capella]]

Attachment: Patch (v1)

This builds and tests ok locally (No regressions) ... if it passes review do you want me to push to try first like I always do?

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 631645 [details] [diff] [review]
Patch (v1)

Review of attachment 631645 [details] [diff] [review]:
-----------------------------------------------------------------

thank you, r=me

::: accessible/src/xul/XULMenuAccessible.cpp
@@ +119,5 @@
>    if (NativelyUnavailable()) {
>      // Note: keep in sinc with nsXULPopupManager::IsValidMenuItem() logic.
>      bool skipNavigatingDisabledMenuItem = true;
>      nsMenuFrame* menuFrame = do_QueryFrame(GetFrame());
> +    if (!menuFrame || !menuFrame->IsOnMenuBar()) {

if (menuFrame && !menuFrame->IsOnMenuBar()) is more correct since in this case you don't expose focusable | selectable states.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #2)
> Created attachment 631645 [details] [diff] [review]
> Patch (v1)
> 
> This builds and tests ok locally (No regressions) ... if it passes review do
> you want me to push to try first like I always do?

up to you (try server build is optional thing)

Comment 5

[Mark Capella [:capella]]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=6ec11aada392

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6ec11aada392

Comment 7

[Robert Kaiser]

This has exploded to #12 in yesterday's data.

Comment 8

[Scoobidiver (away)]

(In reply to Robert Kaiser (:[e-mail]) from comment #7)
> This has exploded to #12 in yesterday's data.
There's a pully user that is responsible of 47% of these crashes.

Comment 9

[Alex Keybl [:akeybl]]

(In reply to Scoobidiver from comment #8)
> (In reply to Robert Kaiser (:[e-mail]) from comment #7)
> > This has exploded to #12 in yesterday's data.
> There's a pully user that is responsible of 47% of these crashes.

Regardless, since this is just a null check, let's take the low risk fix and play it safe.

Mark - can you nominate for Aurora 15 approval? Thanks!

Comment 10

[Mark Capella [:capella]]

I spoke with tbsaunde today. He is handling this for the a11y team.

fwiw, I mentioned that it looks like the patch I originally pushed didn't include a last minute change as directed by comment # 3, and simply promoting it may help but not be the complete solution here.

>> +    if (!menuFrame || !menuFrame->IsOnMenuBar()) {
>if (menuFrame && !menuFrame->IsOnMenuBar()) is more correct

So for now I'll defer to him on what he wants to do next.

Comment 11

[Alex Keybl [:akeybl]]

Should this be reopened as well?

Comment 12

[Trevor Saunders (:tbsaunde)]

Comment on attachment 631645 [details] [diff] [review]
Patch (v1)

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
User impact if declined: crashes
Testing completed (on m-c, etc.): yes
Risk to taking this patch (and alternatives if risky): just null check
String or UUID changes made by this patch: no

Comment 13

[Trevor Saunders (:tbsaunde)]

(In reply to Alex Keybl [:akeybl] from comment #11)
> Should this be reopened as well?

no, we should just take the patch, backporting if needed (but I doubt it is).

I thought we'd already asked for approval :(

Comment 14

[Alex Keybl [:akeybl]]

(In reply to Trevor Saunders (:tbsaunde) from comment #13)
> (In reply to Alex Keybl [:akeybl] from comment #11)
> > Should this be reopened as well?
> 
> no, we should just take the patch, backporting if needed (but I doubt it is).
> 
> I thought we'd already asked for approval :(

Any concerns around "simply promoting it may help but not be the complete solution here" in comment 10?

Comment 15

[Trevor Saunders (:tbsaunde)]

(In reply to Alex Keybl [:akeybl] from comment #14)
> (In reply to Trevor Saunders (:tbsaunde) from comment #13)
> > (In reply to Alex Keybl [:akeybl] from comment #11)
> > > Should this be reopened as well?
> > 
> > no, we should just take the patch, backporting if needed (but I doubt it is).
> > 
> > I thought we'd already asked for approval :(
> 
> Any concerns around "simply promoting it may help but not be the complete
> solution here" in comment 10?

well, ideally comment 3 would be addressed since we'd be a little more correct in an odd case, but other than that no.

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 631645 [details] [diff] [review]
Patch (v1)

low risk null check, let's get this in.

Comment 17

[Trevor Saunders (:tbsaunde)]

landed on beta https://hg.mozilla.org/releases/mozilla-beta/rev/052e3c00e5e3

Comment 18

[Ioana (away)]

According to the Socorro stats, this crash stopped reproducing after the 24th of July:
https://crash-stats.mozilla.com/report/list?query_search=signature&query_type=contains&reason_type=contains&range_value=4&range_unit=weeks&hang_type=any&process_type=any&signature=nsMenuFrame%3A%3AIsOnMenuBar%28%29

https://crash-stats.mozilla.com/report/list?query_search=signature&query_type=contains&reason_type=contains&range_value=4&range_unit=weeks&hang_type=any&process_type=any&signature=nsXULMenuitemAccessible%3A%3ANativeInteractiveState