Closed Bug 816453 Opened 8 years ago Closed 8 years ago

crash in nsAccessibilityService::CreateHTMLObjectFrameAccessible

Categories

(Core :: Disability Access APIs, defect)

17 Branch
All
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla20
Tracking Status
firefox18 --- verified
firefox19 --- verified
firefox20 --- verified

People

(Reporter: scoobidiver, Assigned: tbsaunde)

Details

(Keywords: crash, verifyme)

Crash Data

Attachments

(1 file)

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
(In reply to Scoobidiver from comment #0)
> It's #13 top crasher in 17.0 on Linux.

linux only right?
(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
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?
Attached patch patchSplinter Review
actually bz tells me nsObjectFrame is only for plugins now so lets just get rid of this code since its dead.
Attachment #686885 - Flags: review?(surkov.alexander)
Attachment #686885 - Flags: review?(bzbarsky)
(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).
(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.
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 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.
Attachment #686885 - Flags: review?(bzbarsky) → review+
(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.
(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.
(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.
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 on attachment 686885 [details] [diff] [review]
patch

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

r=me
Attachment #686885 - Flags: review?(surkov.alexander) → review+
https://hg.mozilla.org/mozilla-central/rev/40e050424563
Assignee: nobody → trev.saunders
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
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
Attachment #686885 - Flags: approval-mozilla-esr10?
Attachment #686885 - Flags: approval-mozilla-beta?
Attachment #686885 - Flags: approval-mozilla-aurora?
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.
Attachment #686885 - Flags: approval-mozilla-esr10? → approval-mozilla-esr17?
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.
Attachment #686885 - Flags: approval-mozilla-esr17?
Attachment #686885 - Flags: approval-mozilla-esr17-
Attachment #686885 - Flags: approval-mozilla-beta?
Attachment #686885 - Flags: approval-mozilla-beta+
Attachment #686885 - Flags: approval-mozilla-aurora?
Attachment #686885 - Flags: approval-mozilla-aurora+
I see no crashes on FF 18. Verified fixed based on crash stats.
QA Contact: paul.silaghi
You need to log in before you can comment on or make changes to this bug.