Tyson Smith [:tsmith] Reporter
	Description • 8 years ago

I found this with e10s disabled using the fuzzPriv extension (https://github.com/MozillaSecurity/funfuzz/tree/master/dom/extension)

Assertion failure: aChild->Parent() == mParent, at /home/worker/workspace/build/src/accessible/base/EventTree.cpp:78

==10737==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f5bbfffe5e7 bp 0x7ffff0772b30 sp 0x7ffff0772a80 T0)
    #0 0x7f5bbfffe5e6 in mozilla::a11y::TreeMutation::BeforeRemoval(mozilla::a11y::Accessible*, bool) /home/worker/workspace/build/src/accessible/base/EventTree.cpp:78:3
    #1 0x7f5bc005b7bd in mozilla::a11y::DocAccessible::UpdateTreeOnRemoval(mozilla::a11y::Accessible*, nsIContent*) /home/worker/workspace/build/src/accessible/generic/DocAccessible.cpp:1952:5
    #2 0x7f5bc005c5bf in mozilla::a11y::DocAccessible::RecreateAccessible(nsIContent*) /home/worker/workspace/build/src/accessible/generic/DocAccessible.cpp:1368:3
    #3 0x7f5bc0059178 in mozilla::a11y::DocAccessible::UpdateAccessibleOnAttrChange(mozilla::dom::Element*, nsIAtom*) /home/worker/workspace/build/src/accessible/generic/DocAccessible.cpp:1678:5
    #4 0x7f5bc0058eb8 in mozilla::a11y::DocAccessible::AttributeChanged(nsIDocument*, mozilla::dom::Element*, int, nsIAtom*, int, nsAttrValue const*) /home/worker/workspace/build/src/accessible/generic/DocAccessible.cpp:755:7
    #5 0x7f5bc005a140 in non-virtual thunk to mozilla::a11y::DocAccessible::AttributeChanged(nsIDocument*, mozilla::dom::Element*, int, nsIAtom*, int, nsAttrValue const*) /home/worker/workspace/build/src/accessible/generic/DocAccessible.cpp:744:16
    #6 0x7f5bbc0eea38 in nsNodeUtils::AttributeChanged(mozilla::dom::Element*, int, nsIAtom*, int, nsAttrValue const*) /home/worker/workspace/build/src/dom/base/nsNodeUtils.cpp:145:3
    #7 0x7f5bbbe91752 in mozilla::dom::Element::SetAttrAndNotify(int, nsIAtom*, nsIAtom*, nsAttrValue const&, nsAttrValue&, unsigned char, bool, bool, bool) /home/worker/workspace/build/src/dom/base/Element.cpp:2509:5
    #8 0x7f5bbbe90c90 in mozilla::dom::Element::SetAttr(int, nsIAtom*, nsIAtom*, nsAString_internal const&, bool) /home/worker/workspace/build/src/dom/base/Element.cpp:2371:10
    #9 0x7f5bbdd0f48b in nsGenericHTMLElement::SetAttr(int, nsIAtom*, nsIAtom*, nsAString_internal const&, bool) /home/worker/workspace/build/src/dom/html/nsGenericHTMLElement.cpp:824:17
    #10 0x7f5bbbe8b221 in mozilla::dom::Element::SetAttribute(nsAString_internal const&, nsAString_internal const&, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/base/Element.cpp:1242:14
    #11 0x7f5bbd26d055 in mozilla::dom::ElementBinding::setAttribute(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Element*, JSJitMethodCallArgs const&) /home/worker/workspace/build/src/obj-firefox/dom/bindings/ElementBinding.cpp:725:3
...
see log.txt

	alexander :surkov (:asurkov) Assignee
	Comment 2 • 8 years ago

(In reply to Tyson Smith [:tsmith] from comment #0)

> I found this with e10s disabled using the fuzzPriv extension
> (https://github.com/MozillaSecurity/funfuzz/tree/master/dom/extension)

what the steps are to install the extension?

	Tyson Smith [:tsmith] Reporter
	Comment 3 • 8 years ago

(In reply to alexander :surkov from comment #2)
> (In reply to Tyson Smith [:tsmith] from comment #0)
> 
> > I found this with e10s disabled using the fuzzPriv extension
> > (https://github.com/MozillaSecurity/funfuzz/tree/master/dom/extension)
> 
> what the steps are to install the extension?

It is only being used to enabled accessibility so it may not be needed (and you should remove the 'fuzzPriv.enableAccessibility();' line from the test case) 

Just in case it is needed here are the instructions:

Copy or symlink the whole 'extension' folder from the repo into the 'extensions' folder in your firefox profile, and rename it 'domfuzz@squarefree.com'

Set these prefs in your 
user_pref("extensions.enabledScopes", 5);
user_pref("extensions.autoDisableScopes", 0);
user_pref("xpinstall.signatures.required", false); // doesn't work in beta/release

	alexander :surkov (:asurkov) Assignee
	Comment 4 • 8 years ago

The problem here is we operate on a wrong parent of HTML:option: HTML:option is a child of combobox list, but we feed a combobox itself. Syncing up the insertion and removal logic.

are you ok with review?

	Eitan Isaacson [:eeejay]
	Comment 5 • 8 years ago

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

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

This looks good as a fix for a crasher.

It looks strange to me that a benign attribute that has no effect on rendering or the accessible tree would destroy/create the accessible. I'm not familiar with the underlying design, so I won't push further :)

::: accessible/generic/DocAccessible.h
@@ +351,5 @@
>      UpdateTreeOnRemoval((aContainer ? aContainer : this), aChildNode);
>    }
>    void ContentRemoved(nsIContent* aContainerNode, nsIContent* aChildNode)
>    {
> +    ContentRemoved(AccessibleOrTrueContainer(aContainerNode), aChildNode);

This definitely fixes the crash. Is there a reason why setting 'href' should destroy/create the accessible in the first place? It doesn't trigger a restyle or change the accessible attributes in any way.

::: accessible/tests/mochitest/treeupdate/test_select.html
@@ +91,5 @@
>        }
>      }
>  
> +    /**
> +     * Setting @href on option makes the accessible to recreate.

Suggested rewording: "Settings @href on the option element destroys the accessible and recreates it"

Is the new practice not to reference the bugs that the test is for? I'm fine with that, it gets recursive..

@@ +111,5 @@
> +          { COMBOBOX: [
> +            { COMBOBOX_LIST: [
> +              { COMBOBOX_OPTION: [ ] }
> +            ] }
> +          ] };

You missed actually calling testAccessibleTree here.

	alexander :surkov (:asurkov) Assignee
	Comment 6 • 8 years ago

(In reply to Eitan Isaacson [:eeejay] from comment #5)

> This looks good as a fix for a crasher.
> 
> It looks strange to me that a benign attribute that has no effect on
> rendering or the accessible tree would destroy/create the accessible. I'm
> not familiar with the underlying design, so I won't push further :)

> This definitely fixes the crash. Is there a reason why setting 'href' should
> destroy/create the accessible in the first place? It doesn't trigger a
> restyle or change the accessible attributes in any way.

It's a good point, it's blast from the past I think, probably we wanted to support xlinks or something besides ordinal HTML:a, or just didn't care about checking a tagname, since the attribute doesn't make any sense on non HTML:a elements, and thus no point to use it in real life. I guess that we don't need that code anymore even for HTML:a, since @href would mean restyling, and thus accessible tree update, but it feels like a separate change. I'll file a bug.

> > +    /**
> > +     * Setting @href on option makes the accessible to recreate.
> 
> Suggested rewording: "Settings @href on the option element destroys the
> accessible and recreates it"
> 
> Is the new practice not to reference the bugs that the test is for? I'm fine
> with that, it gets recursive..
> 
> @@ +111,5 @@
> > +          { COMBOBOX: [
> > +            { COMBOBOX_LIST: [
> > +              { COMBOBOX_OPTION: [ ] }
> > +            ] }
> > +          ] };
> 
> You missed actually calling testAccessibleTree here.

thanks :)

	alexander :surkov (:asurkov) Assignee
	Comment 7 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/1781d98423dbf8a6cfd5602913d4a2cc40f5452c
Bug 1331269 - Assertion failure: aChild->Parent() == mParent, r=eitan

	Phil Ringnalda (:philor)
	Comment 8 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/1781d98423db