Closed Bug 630486 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla6

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access, regression, Whiteboard: [sg:moderate])

Attachments

(1 file)

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++
Group: core-security
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.
Blocks: 558072
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?
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).
(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.
(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.
(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?
> 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).
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.
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.
Keywords: regression
Whiteboard: [sg:moderate]
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.
Blocks: a11ynext
Target Milestone: --- → mozilla2.0b11
(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/dbolter@mozilla.com-c50e1ffddbc3/

MarcoZ reports no problems with popups. Our mochitests are unhappy though so I'll need to fix them (later).
Assignee: nobody → bolterbugz
Depends on: 646369
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?
Attached patch patchSplinter Review
we need to decide how we should proceed with GOK onscreen keyboard and menus stuff.
Assignee: bolterbugz → surkov.alexander
Status: NEW → ASSIGNED
Attachment #523303 - Flags: review?(marco.zehe)
Comment on attachment 523303 [details] [diff] [review]
patch

bz: for layout
gavin: for browser/components/feedback
Attachment #523303 - Flags: review?(gavin.sharp)
Attachment #523303 - Flags: review?(bzbarsky)
Attachment #523303 - Flags: feedback?(bolterbugz)
So what is this patch actually doing, and why does it help?
(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 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).
Attachment #523303 - Flags: review?(gavin.sharp) → review+
(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.
(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.
Attachment #523303 - Flags: feedback?(bolterbugz) → feedback+
I have just confirmed that with this patch, GOK menu extraction is broken.
Attachment #523303 - Flags: review?(marco.zehe) → review+
Boris, ping
Comment on attachment 523303 [details] [diff] [review]
patch

r=me for the layout bits
Attachment #523303 - Flags: review?(bzbarsky) → review+
landed http://hg.mozilla.org/mozilla-central/rev/29ea31633ac6
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: mozilla2.0b11 → mozilla6
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.