1829660 - Pass `true` as `fireEvents` when `popover`'s "attribute change steps" "run the hide popover algorithm

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mirko Brodesser (:mbrodesser-Igalia), offline until April 25h]

To match the spec (https://html.spec.whatwg.org/#the-popover-attribute):

The following attribute change steps are used for all HTML elements:
    [...] run the hide popover algorithm given element, true, true, and false.

Relevant code, which might need other adaptation too: https://searchfox.org/mozilla-central/rev/26790fecfcda622dab234b28859da721b80f3a35/dom/html/nsGenericHTMLElement.cpp#687-688

Comment 1

[Mirko Brodesser (:mbrodesser-Igalia), offline until April 25h]

Changing this leads to a crash for a WPT at https://searchfox.org/mozilla-central/rev/1f4f99a8f331cce8467a50742178b6d46914ab89/dom/events/EventDispatcher.cpp#834:

mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*)::$_0::operator()(bool) const (aIsSystem=<optimized out>, this=<optimized out>) at /home/mirko/work/code/gecko/dom/events/EventDispatcher.cpp:819
819	        MOZ_CRASH("This is unsafe! Fix the caller!");
(rr) bt
#0  mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*)::$_0::operator()(bool) const (aIsSystem=<optimized out>, this=<optimized out>) at /home/mirko/work/code/gecko/dom/events/EventDispatcher.cpp:819
#1  mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) (aTarget=<optimized out>, aPresContext=0x0, aEvent=0x7f6e9e074b80, aDOMEvent=0x7f6ea17ddc80, aEventStatus=0x0, aCallback=aCallback@entry=0x0, aTargets=0x0)
    at /home/mirko/work/code/gecko/dom/events/EventDispatcher.cpp:834
#2  0x00007f6eac4aa37c in mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsPresContext*, nsEventStatus*)
    (aTarget=0x7f6e9df143a0, aEvent=<optimized out>, aDOMEvent=0x7f6ea17ddc80, aPresContext=0x0, aEventStatus=0x0) at /home/mirko/work/code/gecko/dom/events/EventDispatcher.cpp:1264
#3  0x00007f6eac6fe8aa in nsGenericHTMLElement::FireToggleEvent(mozilla::dom::PopoverVisibilityState, mozilla::dom::PopoverVisibilityState, nsTSubstring<char16_t> const&)
     (this=0x7f6e9df143a0, aOldState=<optimized out>, aNewState=<optimized out>, aType=u"beforetoggle") at /home/mirko/work/code/gecko/dom/html/nsGenericHTMLElement.cpp:3242
#4  0x00007f6eaafef1ea in mozilla::dom::Document::HidePopover(mozilla::dom::Element&, bool, bool, mozilla::ErrorResult&)
     (this=<optimized out>, aPopover=..., aFocusPreviousElement=true, aFireEvents=true, aRv=...) at /home/mirko/work/code/gecko/dom/base/Document.cpp:15017
#5  0x00007f6eac6f71ec in nsGenericHTMLElement::AfterSetAttr(int, nsAtom*, nsAttrValue const*, nsAttrValue const*, nsIPrincipal*, bool)

Comment 2

[Mirko Brodesser (:mbrodesser-Igalia), offline until April 25h]

It seems the script-blocker added in https://searchfox.org/mozilla-central/rev/1f4f99a8f331cce8467a50742178b6d46914ab89/dom/base/Element.cpp#2485-2488 is one necessary condition for the crash.

Comment 3

[Mirko Brodesser (:mbrodesser-Igalia), offline until April 25h]

In nsGenericHTMLElement::AfterSetAttr, HidePopoverInternal is called. Now, with aFireEvents = true, FireToggleEvent (https://searchfox.org/mozilla-central/rev/1f4f99a8f331cce8467a50742178b6d46914ab89/dom/base/Document.cpp#15017) will be called.

Perhaps for that call of AfterSetAttr, firing and hence dispatching the event should be deferred via a ScriptRunner (https://searchfox.org/mozilla-central/rev/1f4f99a8f331cce8467a50742178b6d46914ab89/dom/base/nsContentUtils.h#2172-2173)?

Comment 4

[Mirko Brodesser (:mbrodesser-Igalia), offline until April 25h]

The idea of above suggestion is to prevent EventDispatcher::Dispatch (https://searchfox.org/mozilla-central/rev/1f4f99a8f331cce8467a50742178b6d46914ab89/dom/events/EventDispatcher.cpp#814,819,834) from crashing due to being in a state where it's unsafe to run script.

Smaug, Emilio: does that approach make sense or if not, any pointers how to fix this?

Comment 5

[Mirko Brodesser (:mbrodesser-Igalia), offline until April 25h]

For the record, pasting Emilio's chat response:

Yeah, that's what script runners are for :)
Part of the issue is that we probably need to defer all that work altogether tho
As beforetoggle is expected to run before the state is updated

Comment 6

[Mirko Brodesser (:mbrodesser-Igalia), offline until April 25h]

Attachment: Bug 1829660: pass `true` for `aFireEvents` and defer hiding the popover and updating the popover states. r=emilio

Pass true for aFireEvents to match the spec.
Defer hiding the popover because it may run script, which is forbidden,
because SetAttrAndNotify runs with a script blocker
(mozAutoDocUpdate).

This fixes some sub-tests of <popover-attribute-basic.html> which
unfortunately currently aren't executed, because of earlier failing
sub-tests. When commenting the latter tests out, some failures like:
Fail Changing a popover from auto to manual (via attr), and then auto during 'beforetoggle' works assert_equals: Content attribute expected "auto" but got "manual"
are fixed.

Comment 7

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

Yes, script runner is supposed to be used when one wants to dispatch a DOM event at a time when running scripts isn't safe.
This is the reason for AsyncEventdispatcher.
(but looks like the most recent patch does also something else)

Comment 8

[Mirko Brodesser (:mbrodesser-Igalia), offline until April 25h]

(In reply to Olli Pettay [:smaug][[e-mail]] from comment #7)

Yes, script runner is supposed to be used when one wants to dispatch a DOM event at a time when running scripts isn't safe.
This is the reason for AsyncEventdispatcher.
(but looks like the most recent patch does also something else)

Thanks for the remark. Wasn't aware of AsyncEventDispatcher. In this case, the current patch seems more appropriate.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c64635691eee
pass `true` for `aFireEvents` and defer hiding the popover and updating the popover states. r=emilio

Comment 10

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/c64635691eee