Closed Bug 1829660 Opened 11 months ago Closed 10 months ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
115 Branch
Tracking Status
firefox115 --- fixed

People

(Reporter: mbrodesser-Igalia, Assigned: mbrodesser-Igalia)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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

See Also: → 1828652
Severity: -- → S3

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)
Status: NEW → ASSIGNED

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.

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)?

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?

Flags: needinfo?(smaug)
Flags: needinfo?(emilio)

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

Flags: needinfo?(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.

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)

Flags: needinfo?(smaug)
Depends on: 1831042
See Also: → 1828502
Blocks: 1831081

(In reply to Olli Pettay [:smaug][bugs@pettay.fi] 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.

Pushed by mbrodesser@igalia.com:
https://hg.mozilla.org/integration/autoland/rev/c64635691eee
pass `true` for `aFireEvents` and defer hiding the popover and updating the popover states. r=emilio
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → 115 Branch
You need to log in before you can comment on or make changes to this bug.