630486 - ASSERTION "Want to fire mutation events, but it's not safe" in nsContentUtils::HasMutationListeners triggered by a11y

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

When refresh observer calls into a11y (NotificationController::WillRefresh) then accessibility tree is created.  nsXULButtonAccessible that is created for XUL button with popup triggers child frames creation of associated popup, i.e. set "menugenerated" attribute on menupopup element.

The question is: are DOM mutations unsafe when refresh observer triggers, i.e. is it a problem of a11y code that starts frame generation at bad time or a problem of menupopup?

the stack is:

>	xul.dll!nsContentUtils::HasMutationListeners(nsINode * aNode, unsigned int aType, nsINode * aTargetForSubtreeModified)  Line 3630 + 0x48 bytes	C++
 	xul.dll!nsGenericElement::SetAttr(int aNamespaceID, nsIAtom * aName, nsIAtom * aPrefix, const nsAString_internal & aValue, int aNotify)  Line 4578 + 0x1b bytes	C++
 	xul.dll!nsGenericElement::SetAttr(int aNameSpaceID, nsIAtom * aName, const nsAString_internal & aValue, int aNotify)  Line 388	C++
 	xul.dll!nsGenericElement::SetAttribute(const nsAString_internal & aName, const nsAString_internal & aValue)  Line 2386 + 0x19 bytes	C++
 	xul.dll!nsXULElement::SetAttribute(const nsAString_internal & name, const nsAString_internal & value)  Line 561 + 0x14 bytes	C++
 	xul.dll!nsIDOMElement_SetAttribute(JSContext * cx, unsigned int argc, jsval_layout * vp)  Line 4853 + 0x23 bytes	C++
 	mozjs.dll!js::CallJSNative(JSContext * cx, int (JSContext *, unsigned int, js::Value *)* native, unsigned int argc, js::Value * vp)  Line 697 + 0xf bytes	C++
 	mozjs.dll!js::Interpret(JSContext * cx, JSStackFrame * entryFrame, unsigned int inlineCallCount, JSInterpMode interpMode)  Line 4780 + 0x21 bytes	C++
 	mozjs.dll!js::RunScript(JSContext * cx, JSScript * script, JSStackFrame * fp)  Line 657 + 0x11 bytes	C++
 	mozjs.dll!js::Invoke(JSContext * cx, const js::CallArgs & argsRef, unsigned int flags)  Line 737 + 0x11 bytes	C++
 	mozjs.dll!js::ExternalInvoke(JSContext * cx, const js::Value & thisv, const js::Value & fval, unsigned int argc, js::Value * argv, js::Value * rval)  Line 858 + 0xf bytes	C++
 	mozjs.dll!js::ExternalInvoke(JSContext * cx, JSObject * obj, const js::Value & fval, unsigned int argc, js::Value * argv, js::Value * rval)  Line 961 + 0x2a bytes	C++
 	mozjs.dll!JS_CallFunctionValue(JSContext * cx, JSObject * obj, jsval_layout fval, unsigned int argc, jsval_layout * argv, jsval_layout * rval)  Line 5075 + 0x38 bytes	C++
 	xul.dll!nsXBLProtoImplAnonymousMethod::Execute(nsIContent * aBoundElement)  Line 337 + 0x3c bytes	C++
 	xul.dll!nsXBLPrototypeBinding::BindingAttached(nsIContent * aBoundElement)  Line 486 + 0x12 bytes	C++
 	xul.dll!nsXBLBinding::ExecuteAttachedHandler()  Line 980	C++
 	xul.dll!nsBindingManager::ProcessAttachedQueue(unsigned int aSkipSize)  Line 1021	C++
 	xul.dll!nsCSSFrameConstructor::GenerateChildFrames(nsIFrame * aFrame)  Line 11769	C++
 	xul.dll!nsMenuPopupFrame::AttributeChanged(int aNameSpaceID, nsIAtom * aAttribute, int aModType)  Line 1781	C++
 	xul.dll!nsCSSFrameConstructor::AttributeChanged(mozilla::dom::Element * aElement, int aNameSpaceID, nsIAtom * aAttribute, int aModType)  Line 8273	C++
 	xul.dll!PresShell::AttributeChanged(nsIDocument * aDocument, mozilla::dom::Element * aElement, int aNameSpaceID, nsIAtom * aAttribute, int aModType)  Line 5053	C++
 	xul.dll!nsNodeUtils::AttributeChanged(mozilla::dom::Element * aElement, int aNameSpaceID, nsIAtom * aAttribute, int aModType)  Line 136 + 0xd0 bytes	C++
 	xul.dll!nsGenericElement::SetAttrAndNotify(int aNamespaceID, nsIAtom * aName, nsIAtom * aPrefix, const nsAString_internal & aOldValue, nsAttrValue & aParsedValue, unsigned char aModType, int aFireMutation, int aNotify, const nsAString_internal * aValueForAfterSetAttr)  Line 4686 + 0x19 bytes	C++
 	xul.dll!nsGenericElement::SetAttr(int aNamespaceID, nsIAtom * aName, nsIAtom * aPrefix, const nsAString_internal & aValue, int aNotify)  Line 4623 + 0x39 bytes	C++
 	xul.dll!nsIContent::SetAttr(int aNameSpaceID, nsIAtom * aName, const nsAString_internal & aValue, int aNotify)  Line 366	C++
 	xul.dll!nsCoreUtils::GeneratePopupTree(nsIContent * aContent, int aIsAnon)  Line 799 + 0x2a bytes	C++
 	xul.dll!nsXULButtonAccessible::Init()  Line 121 + 0x13 bytes	C++
 	xul.dll!nsDocAccessible::BindToDocument(nsAccessible * aAccessible, nsRoleMapEntry * aRoleMapEntry)  Line 1361 + 0xd bytes	C++
 	xul.dll!nsAccessibilityService::GetOrCreateAccessible(nsINode * aNode, nsIPresShell * aPresShell, nsIWeakReference * aWeakShell, bool * aIsSubtreeHidden)  Line 1146 + 0x15 bytes	C++
 	xul.dll!nsAccTreeWalker::GetNextChildInternal(int aNoWalkUp)  Line 119 + 0x2d bytes	C++
 	xul.dll!nsAccTreeWalker::GetNextChildInternal(int aNoWalkUp)  Line 129 + 0xe bytes	C++
 	xul.dll!nsAccTreeWalker::GetNextChildInternal(int aNoWalkUp)  Line 129 + 0xe bytes	C++
 	xul.dll!nsAccTreeWalker::GetNextChildInternal(int aNoWalkUp)  Line 129 + 0xe bytes	C++
 	xul.dll!nsAccTreeWalker::GetNextChildInternal(int aNoWalkUp)  Line 138 + 0x23 bytes	C++
 	xul.dll!nsAccTreeWalker::GetNextChild()  Line 65 + 0xe bytes	C++
 	xul.dll!nsDocAccessible::CacheChildren()  Line 1490 + 0xc bytes	C++
 	xul.dll!nsAccessible::EnsureChildren()  Line 3193	C++
 	xul.dll!nsDocAccessible::CacheChildrenInSubtree(nsAccessible * aRoot)  Line 1928	C++
 	xul.dll!NotificationController::WillRefresh(mozilla::TimeStamp aTime)  Line 224	C++
 	xul.dll!nsRefreshDriver::Notify(nsITimer * __formal)  Line 256	C++
 	xul.dll!nsTimerImpl::Fire()  Line 429	C++
 	xul.dll!nsTimerEvent::Run()  Line 519	C++

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Uh, nsMenuPopupFrame::AttributeChanged is doing something bad.
The call to GenerateChildFrames seems to trigger scripts to run :(

Regression from bug 558072, so fortunately 1.9.2 shouldn't have the problem
and on trunk XUL is disabled on web content.

Comment 2

[alexander :surkov (:asurkov)]

Thanks, Olli.

Boris, is there any problem that a11y triggers layout changes while refresh observer calls into a11y? For example, layout gets undone while rest registered refresh observers are going be called?

Comment 3

[Neil Deakin]

The call to GenerateChildFrames just needs to be moved into a nsContentUtils::AddScriptRunner.

Alternatively, get rid of the menugenerated attribute and just have some accessibility code that does the equivalent of GenerateChildFrames (in a script runner if needed).

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #3)
> The call to GenerateChildFrames just needs to be moved into a
> nsContentUtils::AddScriptRunner.

this sounds like a right fix

> Alternatively, get rid of the menugenerated attribute and just have some
> accessibility code that does the equivalent of GenerateChildFrames (in a script
> runner if needed).

script runner will help in a11y case but doesn't get right others if they are.

Comment 5

[Neil Deakin]

(In reply to comment #4)
> script runner will help in a11y case but doesn't get right others if they are.

The menugenerated attribute is a hack put in specifically for accessibility. It would be good to get rid of it. But that can be a separate bug.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #5)
> (In reply to comment #4)
> > script runner will help in a11y case but doesn't get right others if they are.
> 
> The menugenerated attribute is a hack put in specifically for accessibility. It
> would be good to get rid of it. But that can be a separate bug.

Ok, if we can query menupopup frame and it has virtual public methods do that then it's ok (we can always add it). Of course that's preferable way.

Neil, btw, don't you like to take this bug?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Boris, is there any problem that a11y triggers layout changes

No.  The refresh driver explicitly expects refresh observers to do evil things (esp. because some of those observers are by-design random web page script).

Comment 8

[Boris Zbarsky [:bzbarsky]]

That said...  the accessibility callback is running _after_ layout.  So if it triggers frame changes those won't be reflected while painting; this can lead to flicker.  Not sure whether it's an issue in practice.

Comment 9

[Daniel Veditz [:dveditz]]

sg:moderate based on the fact that XUL isn't allowed from web content, but there may be some way to force our chrome (or that of an add-on) to do this.

Comment 10

[alexander :surkov (:asurkov)]

I think we don't need anymore to force menupopup frame children creation and this part can be removed. Let's do it in Fx5 timeframe.

Comment 11

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

(In reply to comment #10)
> I think we don't need anymore to force menupopup frame children creation and
> this part can be removed. Let's do it in Fx5 timeframe.

I ripped this out.
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-c50e1ffddbc3/

MarcoZ reports no problems with popups. Our mochitests are unhappy though so I'll need to fix them (later).

Comment 12

[alexander :surkov (:asurkov)]

I wonder if bug 279703 comment #42 is still valid. Can you guess what onscreen keyboard should I try and what steps to reproduce could be?

Comment 13

[alexander :surkov (:asurkov)]

Attachment: patch

we need to decide how we should proceed with GOK onscreen keyboard and menus stuff.

Comment 14

[alexander :surkov (:asurkov)]

try server build - http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-83702c9fb07a/

Comment 15

[alexander :surkov (:asurkov)]

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

bz: for layout
gavin: for browser/components/feedback

Comment 16

[Boris Zbarsky [:bzbarsky]]

So what is this patch actually doing, and why does it help?

Comment 17

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

(In reply to comment #13)
> Created attachment 523303 [details] [diff] [review]
> patch
> 
> we need to decide how we should proceed with GOK onscreen keyboard and menus
> stuff.

If this the reason for the feedback request? Or do you want me to look at the patch details?

Comment 18

[:Gavin Sharp [email: [e-mail]]]

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

r=me on the browser/components/feeds/content/subscribe.xml change (regardless of what happens with the rest of the patch).

Comment 19

[alexander :surkov (:asurkov)]

(In reply to comment #16)
> So what is this patch actually doing, and why does it help?

A11y doesn't force popup's child frames creation, so a11y can't trigger the assertion anymore.

Comment 20

[alexander :surkov (:asurkov)]

(In reply to comment #17)

> If this the reason for the feedback request? Or do you want me to look at the
> patch details?

just thumb up on the patch approach.

Comment 21

[Fernando Herrera]

I have just confirmed that with this patch, GOK menu extraction is broken.

Comment 22

[alexander :surkov (:asurkov)]

Boris, ping

Comment 23

[Boris Zbarsky [:bzbarsky]]

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

r=me for the layout bits

Comment 24

[alexander :surkov (:asurkov)]

landed http://hg.mozilla.org/mozilla-central/rev/29ea31633ac6