706784 - Firefox 8.0.1 Crash Report [@ nsLinkableAccessible::GetValue(nsAString_internal&) ] (mainly correlated to WebRoot Secure Anywhere)

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Carsten Book [:Tomcat]]

noticed this crash on the top changer report for Firefox 8.0.1. Crash  [@ nsLinkableAccessible::GetValue(nsAString_internal&) ] - example crash report -> https://crash-stats.mozilla.com/report/index/d0216e0f-4bd2-449e-8b1a-317032111201 and general overview -> https://crash-stats.mozilla.com/report/list?range_value=3&range_unit=days&signature=nsLinkableAccessible%3A%3AGetValue%28nsAString_internal%26%29&version=Firefox%3A8.0.1

seems its there since at least Firefox 7.0.1

stack:

Crashing Thread
Frame 	Module 	Signature [Expand] 	Source
0 	xul.dll 	nsLinkableAccessible::GetValue 	accessible/src/base/nsBaseWidgetAccessible.cpp:137
1 	xul.dll 	nsAccessibleWrap::get_accValue 	accessible/src/msaa/nsAccessibleWrap.cpp:322
2 	rpcrt4.dll 	Invoke 	
3 	rpcrt4.dll 	NdrStubCall2 	
4 	ole32.dll 	NdrpCreateStub 	
5 	oleaut32.dll 	oleaut32.dll@0xffd2 	
6 	ole32.dll 	SyncStubInvoke 	
7 	ole32.dll 	StubInvoke 	
8 	ole32.dll 	CCtxComChnl::ContextInvoke 	
9 	ole32.dll 	MTAInvoke 	
10 	ole32.dll 	STAInvoke 	
11 	ole32.dll 	AppInvoke 	
12 	ole32.dll 	ComInvokeWithLockAndIPID 	
13 	ole32.dll 	ComInvoke 	
14 	ole32.dll 	ThreadDispatch 	
15 	ole32.dll 	ThreadWndProc 	
16 	user32.dll 	InternalCallWinProc 	
17 	user32.dll 	UserCallWinProcCheckWow 	
18 	user32.dll 	DispatchMessageWorker 	
19 	user32.dll 	DispatchMessageW 	
20 	xul.dll 	nsAppShell::ProcessNextNativeEvent 	widget/src/windows/nsAppShell.cpp:346
21 	xul.dll 	nsBaseAppShell::OnProcessNextEvent 	widget/src/xpwidgets/nsBaseAppShell.cpp:324
22 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:595
23 	xul.dll 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:134
24 	xul.dll 	xul.dll@0xba37ab 	
25 	xul.dll 	MessageLoop::RunHandler 	ipc/chromium/src/base/message_loop.cc:205
26 	xul.dll 	xul.dll@0x30056f 	
27 	xul.dll 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:179
28 	xul.dll 	mozilla::storage::AsyncExecuteStatements::AsyncExecuteStatements 	storage/src/mozStorageAsyncStatementExecution.cpp:239
29 	xul.dll 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:189
30 	dui70.dll 	DirectUI::ValueProvider::ValueProvider 	
31 	xul.dll 	nsAppStartup::Run 	toolkit/components/startup/nsAppStartup.cpp:224
32 	xul.dll 	XRE_main 	toolkit/xre/nsAppRunner.cpp:3544
33 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:107
34 	firefox.exe 	firefox.exe@0x4043 	
35 	firefox.exe 	_RTC_Initialize 	
36 	mozcrt19.dll 	_initterm 	obj-firefox/memory/jemalloc/crtsrc/crt0dat.c:852
37 	firefox.exe 	__security_init_cookie 	obj-firefox/memory/jemalloc/crtsrc/gs_support.c:139
38 	kernel32.dll 	BaseThreadInitThunk 	
39 	ntdll.dll 	WinSqmSetIfMaxDWORD 	
40 	ntdll.dll 	_RtlUserThreadStart 	
41 	firefox.exe 	firefox.exe@0x1cef 	
42 	firefox.exe 	firefox.exe@0x1cef

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[Marco Zehe (:MarcoZ)]

Comment on attachment 578219 [details] [diff] [review]
patch

r=me thanks for the quick fix!

Comment 3

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/768c2a7a320e

Comment 4

[Scoobidiver (away)]

There are still crashes with 64-bit builds:
https://crash-stats.mozilla.com/report/list?version=Firefox:11.0a1&range_value=30&range_unit=days&signature=nsLinkableAccessible%3A%3AGetValue%28nsAString_internal%26%29

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

Trevor, I was looking at the fix over on bug 653584. It seems that nsLinkableAccessible::BindToParent can exit without assigning mActionAcc (leaving it null).

Should we be null checking in GetValue,DoAction,GetKeyboardShortcut,GetAnchorURI?

Also, I didn't dive too deep but is there any danger mActionAcc could equal /this/? (GetValue could infinitely recurse)?

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

OK as per IRC I see mIsLink should only be true when we have an mActionAcc. Smells a bit funny though.

I have no idea why these crashes are only happening on amd64.

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

Users are still getting crashes.

Comment 8

[Scoobidiver (away)]

It's #47 top browser crasher in 9.0.1 and #46 in 10.0b2.
After the patch landing, it's #82 top browser crasher in 11.0a2 and #288 in 12.0a1.

Comment 9

[Scoobidiver (away)]

There's a high correlation in 9.0.1 (before the patch) with WebRoot Secure Anywhere:
94% (240/255) vs.   1% (555/101230) WRusr.dll

Comment 10

[Marcia Knous [:marcia]]

This is showing up in Firefox 10 crash data as the #32 overall crash. Adding Kev to see if he has a contact at that company - http://www.webroot.com/En_US/consumer-products-secureanywhere-complete.html.

Comment 11

[Scoobidiver (away)]

Before the patch, it's #35 top crasher in 10.0.
After the patch, it's #87 top crasher in 11.0b1, #61 in 12.0a2, and #146 in 13.0a1.

There are still correlations to WebRoot SecureAnywhere in 11.0:
nsLinkableAccessible::GetValue(nsAString_internal&)|EXCEPTION_ACCESS_VIOLATION_READ (30 crashes)
     90% (27/30) vs.   0% (69/26008) WRusr.dll
          0% (0/30) vs.   0% (2/26008) 8.0.1.42
         90% (27/30) vs.   0% (67/26008) 8.0.1.95

Comment 12

[Sheila Mooney]

Marcia, did we try and reproduce this?

Comment 13

[Scoobidiver (away)]

It's #18 top browser crasher in 11.0, #24 in 12.0b1.

It still happens with the latest WebRoot SecureAnywhere version:
     95% (20/21) vs.   0% (39/13377) WRusr.dll
          0% (0/21) vs.   0% (1/13377) 8.0.1.40
         95% (20/21) vs.   0% (38/13377) 8.0.1.154

Comment 14

[David Bolter [:davidb] (NeedInfo me for attention)]

Alexander, any ideas?

Comment 15

[Trevor Saunders (:tbsaunde)]

(In reply to David Bolter [:davidb] from comment #14)
> Alexander, any ideas?

I still can't think of anything, but I'd be interested to see if msvc can get us a stack trace with a little more info, such as local vars or exactly which instruction we crash on and the local disassembly

Comment 16

[alexander :surkov (:asurkov)]

a crash link pls?

Comment 17

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to alexander :surkov from comment #16)
> a crash link pls?

http://bit.ly/GCu1gq

Comment 18

[alexander :surkov (:asurkov)]

Attachment: patch: second round

Comment 19

[Trevor Saunders (:tbsaunde)]

Comment on attachment 607832 [details] [diff] [review]
patch: second round

>+nsLinkableAccessible::UnbindFromParent()
>+{
>+  mActionAcc = nsnull;
>+  mIsLink = false;
>+  mIsOnclick = nsnull;

false?

Comment 20

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #19)

> >+  mIsOnclick = nsnull;
> 
> false?

sure, I noticed that after I filed the patch :)

Comment 21

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #18)
> Created attachment 607832 [details] [diff] [review]
> patch: second round

https://hg.mozilla.org/integration/mozilla-inbound/rev/21fabfdbeaab

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/21fabfdbeaab

Comment 23

[Scoobidiver (away)]

It's #25 top browser crasher in 12.0b1 and 13.0a2.
That qualifies it for Aurora and Beta approvals.

Comment 24

[alexander :surkov (:asurkov)]

(In reply to Scoobidiver from comment #23)
> It's #25 top browser crasher in 12.0b1 and 13.0a2.
> That qualifies it for Aurora and Beta approvals.

this patch is a reasonable guess about the crash cause. What is the best way to proceed, should we get some results from Firefox 14 before nomination for back porting or should we proceed immediately?

Comment 25

[Scoobidiver (away)]

(In reply to alexander :surkov from comment #24)
> should we get some results from Firefox 14 before nomination for
> back porting or should we proceed immediately?
Before back porting it, let's monitor it in the trunk during a few days with this link:
https://crash-stats.mozilla.com/report/list?query_search=signature&version=Firefox:14.0a1&query_type=contains&reason_type=contains&range_value=4&range_unit=weeks&hang_type=any&process_type=any&signature=nsLinkableAccessible%3A%3AGetValue%28nsAString_internal%26%29

Comment 26

[alexander :surkov (:asurkov)]

it sounds like it improves things: two crashes for 25 and 28 March builds. Makes sense to back port this patch.

Comment 27

[Marco Zehe (:MarcoZ)]

Comment on attachment 607832 [details] [diff] [review]
patch: second round

[Approval Request Comment]
Regression caused by (bug #): unknown
User impact if declined: frequent crashes
Testing completed (on m-c, etc.): Yes, see comment #26
Risk to taking this patch (and alternatives if risky):None, just nulling out some members.
String changes made by this patch:None

Comment 28

[Johnathan Nightingale [:johnath]]

Comment on attachment 607832 [details] [diff] [review]
patch: second round

Discussed and approved in triage, land it soon please!

Comment 29

[Marco Zehe (:MarcoZ)]

Landed on Alexander's behalf:
http://hg.mozilla.org/releases/mozilla-aurora/rev/fdce25e870da
http://hg.mozilla.org/releases/mozilla-beta/rev/833d79ffbf39

Comment 30

[Michael Lefevre]

(In reply to alexander :surkov from comment #20)
> (In reply to Trevor Saunders (:tbsaunde) from comment #19)
> > >+  mIsOnclick = nsnull;
> > 
> > false?
> 
> sure, I noticed that after I filed the patch :)

I guess it doesn't matter, but the push to aurora and beta doesn't seem to have this change from nsnull to false, which was made between attaching the patch and pushing to trunk.

Comment 31

[Marco Zehe (:MarcoZ)]

Alex, did you make a change before pushing to trunk? If so, please make these changes to the aurora and beta source files and push as bustage fixes.

Comment 32

[alexander :surkov (:asurkov)]

(In reply to Marco Zehe (:MarcoZ) from comment #31)
> Alex, did you make a change before pushing to trunk?

yes, sometimes I do that. So the best way is to import path from trunk and then back port it to aurora or beta. To be on safe side.

> If so, please make
> these changes to the aurora and beta source files and push as bustage fixes.

I don't think it's big deal. nsnull is casted to false.

Comment 33

[u279076]

Verify fix by checking crashstats after the March 29th landing.

Comment 34

[Virgil Dicu [:virgil] [QA]]

http://bit.ly/HPSf4c

Since the fix landed on 12 beta, a lot less crashes than F11, but still a few:
12 crashes on 12b4
5 crashes on 12b5

Are these expected? Would it be enough to call it verified?

Comment 35

[Robert Kaiser]

(In reply to Virgil Dicu [:virgil] [QA] from comment #34)
> Are these expected? Would it be enough to call it verified?

I would think so. From the experience I have dealing with crash stats every day, I'd guess the main issue we saw here has been fixed and there is a different issue just merely triggering something with the same signature. For this other issue, a different bug should be filed if it's of high enough frequency (which your numbers don't seem to imply).

Comment 36

[Virgil Dicu [:virgil] [QA]]

Thank you for the clarification, Robert. Marking this as verified on Firefox 12 based on crash-stats data.

Comment 37

[Virgil Dicu [:virgil] [QA]]

http://bit.ly/HPSf4c

Only one crash in F13beta after 2 weeks after merge. Marking as verified in F13 as well, as the main issue is resolved.