816453 - crash in nsAccessibilityService::CreateHTMLObjectFrameAccessible

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Scoobidiver (away)]

It's #13 top crasher in 17.0 on Linux.

Signature 	nsAccessibilityService::CreateHTMLObjectFrameAccessible More Reports Search
UUID	07a7e8b2-b013-49ec-8c18-513512121128
Date Processed	2012-11-28 03:16:47
Uptime	3217
Install Age	4.3 days since version was first installed.
Install Time	2012-11-23 19:59:22
Product	Firefox
Version	17.0
Build ID	20121120063346
Release Channel	release
OS	Linux
OS Version	0.0.0 Linux 3.5.0-18-generic #29-Ubuntu SMP Fri Oct 19 10:27:31 UTC 2012 i686
Build Architecture	x86
Build Architecture Info	GenuineIntel family 6 model 37 stepping 5
Crash Reason	SIGSEGV
Crash Address	0xf0dea863
App Notes 	
OpenGL: ATI Technologies Inc. -- AMD Radeon HD 6300M Series  -- 4.2.11903 Compatibility Profile Context -- texture_from_pixmap
EMCheckCompatibility	True	
Accessibility	Active

Frame 	Module 	Signature 	Source
0 	libxul.so 	nsAccessibilityService::CreateHTMLObjectFrameAccessible 	nsIFrame.h:1073
1 	libxul.so 	nsObjectFrame::CreateAccessible 	nsObjectFrame.cpp:277
2 	libxul.so 	nsAccessibilityService::GetOrCreateAccessible 	nsAccessibilityService.cpp:1125
3 	libxul.so 	nsAccTreeWalker::NextChildInternal 	nsAccTreeWalker.cpp:82
4 	libxul.so 	DocAccessible::CacheChildren 	nsAccTreeWalker.h:35
5 	libxul.so 	Accessible::EnsureChildren 	Accessible.cpp:2964
6 	libxul.so 	DocAccessible::ProcessContentInserted 	DocAccessible.cpp:1826
7 	libxul.so 	NotificationController::ContentInsertion::Process 	NotificationController.cpp:838
8 	libxul.so 	NotificationController::WillRefresh 	NotificationController.cpp:230
9 	libxul.so 	nsRefreshDriver::Notify 	nsRefreshDriver.cpp:339
10 	libxul.so 	nsTimerImpl::Fire 	nsTimerImpl.cpp:476
11 	libxul.so 	nsTimerEvent::Run 	nsTimerImpl.cpp:556
12 	libxul.so 	nsThread::ProcessNextEvent 	nsThread.cpp:624
13 	libxul.so 	NS_ProcessNextEvent_P 	nsThreadUtils.cpp:220
14 	libxul.so 	mozilla::ipc::MessagePump::Run 	MessagePump.cpp:82
15 	libxul.so 	MessageLoop::RunInternal 	message_loop.cc:208
16 	libxul.so 	MessageLoop::Run 	message_loop.cc:201
17 	libxul.so 	nsBaseAppShell::Run 	nsBaseAppShell.cpp:163
18 	libxul.so 	nsAppStartup::Run 	nsAppStartup.cpp:273
19 	libxul.so 	XREMain::XRE_mainRun 	nsAppRunner.cpp:3812
20 	libxul.so 	XREMain::XRE_main 	nsAppRunner.cpp:3889
21 	libxul.so 	XRE_main 	nsAppRunner.cpp:3965 

More reports at:
https://crash-stats.mozilla.com/report/list?signature=nsAccessibilityService%3A%3ACreateHTMLObjectFrameAccessible

Comment 1

[alexander :surkov (:asurkov)]

related code has been changed but nevertheless the reason can be still valid. It claims it crashes at:

http://hg.mozilla.org/releases/mozilla-release/annotate/919435c6f654/layout/generic/nsIFrame.h#l1073

(used stack https://crash-stats.mozilla.com/report/index/1bf8dc54-c3bc-4862-b0b6-782ab2121124)

that should mean a11y code line:
nsIFrame* frame = aFrame->GetFirstPrincipalChild();

http://hg.mozilla.org/releases/mozilla-release/annotate/919435c6f654/accessible/src/base/nsAccessibilityService.cpp#l364

Comment 2

[Trevor Saunders (:tbsaunde)]

(In reply to Scoobidiver from comment #0)
> It's #13 top crasher in 17.0 on Linux.

linux only right?

Comment 3

[Scoobidiver (away)]

(In reply to Trevor Saunders (:tbsaunde) from comment #2)
> linux only right?
On Windows, there are one crash with a different stack trace, bp-95180176-a7c7-4c3c-a3e0-818a72121127 and two crashes from the same user in 16.0.2: https://crash-stats.mozilla.com/report/list?signature=nsAccessibilityService%3A%3ACreateHTMLObjectFrameAccessible%28nsObjectFrame*%2C+nsIContent*%2C+nsIPresShell*%29

Comment 4

[Trevor Saunders (:tbsaunde)]

So, I think what's happening is that nsPluginInstanceOwner::GetValueFromPlugin() is spinning the event loop or something that causes a reflow and the nsObjectFrame* gets fred.

Alex have thoughts on the nicest way to handle this?

Comment 5

[Trevor Saunders (:tbsaunde)]

Attachment: patch

actually bz tells me nsObjectFrame is only for plugins now so lets just get rid of this code since its dead.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #4)
> So, I think what's happening is that
> nsPluginInstanceOwner::GetValueFromPlugin() is spinning the event loop or
> something that causes a reflow and the nsObjectFrame* gets fred.

If this happens then it seems like a wrong thing. Can we get some body who can confirm that?

(In reply to Trevor Saunders (:tbsaunde) from comment #5)
> Created attachment 686885 [details] [diff] [review]
> patch
> 
> actually bz tells me nsObjectFrame is only for plugins now so lets just get
> rid of this code since its dead.

ok in that case we regressed somewhere earlier since HTML object element can be used as it described in our code (also see http://www.w3.org/TR/html401/struct/objects.html). So it doesn't look very right to remove that code (it should be changed instead).

Comment 7

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #6)
> (In reply to Trevor Saunders (:tbsaunde) from comment #4)
> > So, I think what's happening is that
> > nsPluginInstanceOwner::GetValueFromPlugin() is spinning the event loop or
> > something that causes a reflow and the nsObjectFrame* gets fred.
> 
> If this happens then it seems like a wrong thing. Can we get some body who
> can confirm that?

why? it's calling into the plugin, which is third party code, so can do whatever random thing it likes.

> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> > Created attachment 686885 [details] [diff] [review]
> > patch
> > 
> > actually bz tells me nsObjectFrame is only for plugins now so lets just get
> > rid of this code since its dead.
> 
> ok in that case we regressed somewhere earlier since HTML object element can
> be used as it described in our code (also see
> http://www.w3.org/TR/html401/struct/objects.html). So it doesn't look very
> right to remove that code (it should be changed instead).

that's the elemnt, the spec doesn't say anything about frames, so it seems perfectly fine that layout should give the HTMLObjectElement a different frame type if the element is for something other than a plugin.

Comment 8

[Boris Zbarsky [:bzbarsky]]

HTMLObjectElement gets an nsSubDocumentFrame, an nsImageFrame, or an nsObjectFrame depending on what it's doing (subdocument, image, plugin respectively).

And yes, any time you call into the plug-ins's code it can do random crap.  :(

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

r=me.  Might be good to add a comment at the bottom of the method that aFrame might be dead at that point.

Comment 10

[alexander :surkov (:asurkov)]

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

> > > So, I think what's happening is that
> > > nsPluginInstanceOwner::GetValueFromPlugin() is spinning the event loop or
> > > something that causes a reflow and the nsObjectFrame* gets fred.
> > 
> > If this happens then it seems like a wrong thing. Can we get some body who
> > can confirm that?
> 
> why? it's calling into the plugin, which is third party code, so can do
> whatever random thing it likes.

right, we cannot trust them. Just ID getting stuff shouldn't call back into the browser, at least I don't see a reason. It's like we don't have null checks on MSAA side and we don't crash.

but that's a problem in general, if they flush then we create an accessible under wrong assumptions and we may crash somewhere else.

> > > actually bz tells me nsObjectFrame is only for plugins now so lets just get
> > > rid of this code since its dead.
> > 
> > ok in that case we regressed somewhere earlier since HTML object element can
> > be used as it described in our code (also see
> > http://www.w3.org/TR/html401/struct/objects.html). So it doesn't look very
> > right to remove that code (it should be changed instead).
> 
> that's the elemnt, the spec doesn't say anything about frames, so it seems
> perfectly fine that layout should give the HTMLObjectElement a different
> frame type if the element is for something other than a plugin.

it's true, my point was that previously this code worked to handle different kinds (natures) of object elements. Now per bz comment it doesn't work so we *likely* have a problem and *possibly* we still need this code but it should not be running under nsObjectFrame assumption. Do you agree?

So I would suggest to add few mochitests for different kinds of object elements and see if it works. If you don't want to do that here then please file follow up and it'd be great if you pick it up.

Comment 11

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #10)
> (In reply to Trevor Saunders (:tbsaunde) from comment #7)
> 
> > > > So, I think what's happening is that
> > > > nsPluginInstanceOwner::GetValueFromPlugin() is spinning the event loop or
> > > > something that causes a reflow and the nsObjectFrame* gets fred.
> > > 
> > > If this happens then it seems like a wrong thing. Can we get some body who
> > > can confirm that?
> > 
> > why? it's calling into the plugin, which is third party code, so can do
> > whatever random thing it likes.
> 
> right, we cannot trust them. Just ID getting stuff shouldn't call back into
> the browser, at least I don't see a reason. It's like we don't have null
> checks on MSAA side and we don't crash.
> 
> but that's a problem in general, if they flush then we create an accessible
> under wrong assumptions and we may crash somewhere else.

true, but I'm not sure what you think we can do about it, other than work around it when problems come up.

> it's true, my point was that previously this code worked to handle different
> kinds (natures) of object elements. Now per bz comment it doesn't work so we
> *likely* have a problem and *possibly* we still need this code but it should
> not be running under nsObjectFrame assumption. Do you agree?
> 
> So I would suggest to add few mochitests for different kinds of object
> elements and see if it works. If you don't want to do that here then please
> file follow up and it'd be great if you pick it up.

so, I took a quick look and I think everything should actually work because
<object> for image -> nsImageFrame and then we call get AccessibleType() on the frame and get the right accessible type
<object> for sub doc -> nsSudocumentFrame and again AccessibleType() dtrt for us.

I'll file follow up for tests though.

Comment 12

[alexander :surkov (:asurkov)]

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

> > but that's a problem in general, if they flush then we create an accessible
> > under wrong assumptions and we may crash somewhere else.
> 
> true, but I'm not sure what you think we can do about it, other than work
> around it when problems come up.

as Boris said add a comment :)

> so, I took a quick look and I think everything should actually work because
> <object> for image -> nsImageFrame and then we call get AccessibleType() on
> the frame and get the right accessible type
> <object> for sub doc -> nsSudocumentFrame and again AccessibleType() dtrt
> for us.

object element has own "features", if we rely on DOM in those accessible classes then we should fail somewhere.

> I'll file follow up for tests though.

ok, sounds good.

Comment 13

[Boris Zbarsky [:bzbarsky]]

If you rely on DOM in those accessibles you have a problem no matter what.

You can get an nsImageFrame for an <html:object>, <html:input>, <html:img>, <svg:image>, <svg:feImage> and the element class we create for CSS generated content images.

You can get an nsSubDocumentFrame for <html:object>, <html:embed>, <html:iframe>, <html:frame>, <xul:iframe>, and <xul:browser>.

Comment 14

[alexander :surkov (:asurkov)]

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

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

r=me

Comment 15

[Trevor Saunders (:tbsaunde)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/40e050424563

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/40e050424563

Comment 17

[Trevor Saunders (:tbsaunde)]

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

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version:
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): probably from when we introduced plugin accessibility for linux around firefox 5 or 6.
User impact if declined:  crashes (possibly exploitable)
Testing completed (on m-c, etc.):  yes
Risk to taking this patch (and alternatives if risky):  low just removes code  that is dead anyway.
String or UUID changes made by this patch:  none

Comment 18

[Trevor Saunders (:tbsaunde)]

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

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version:
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

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

Go ahead with uplift to Aurora/Beta for this low risk crash fix, but for ESR17 this doesn't meet the criteria for that branch so we won't be taking it there.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5725b445e2c3
https://hg.mozilla.org/releases/mozilla-beta/rev/db56aa86d643

Comment 21

[Paul Silaghi, QA [:pauly]]

I see no crashes on FF 18. Verified fixed based on crash stats.

Comment 22

[Virgil Dicu [:virgil] [QA]]

No crashes with this signature in any build newer than 17.

https://crash-stats.mozilla.com/report/list?signature=nsAccessibilityService%3A%3ACreateHTMLObjectFrameAccessible