1834972 - Investigate violated assertion: `Top layer element should be in doc: 'element->IsInComposedDoc()', file /home/mirko/work/code/gecko/dom/base/Document.cpp`

Status: RESOLVED FIXED

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

Description

[Mirko Brodesser (:mbrodesser-Igalia)]

Triggered by e.g.:

./mach wpt --debug-test ./testing/web-platform/tests/html/semantics/popovers/popover-attribute-basic.html

Comment 1

[Mirko Brodesser (:mbrodesser-Igalia)]

Call stack of the assertion's first violation:

(rr) bt
#0  mozilla::dom::Document::GetTopLayerTop() (this=this@entry=0x7fc5da50d000) at /home/mirko/work/code/gecko/dom/base/Document.cpp:14946
#1  0x00007fc5e18c8def in mozilla::dom::Document::TopLayerPop(mozilla::FunctionRef<bool (mozilla::dom::Element*)>) (this=0x7fc5da50d000, aPredicate=...)
    at /home/mirko/work/code/gecko/dom/base/Document.cpp:14789
#2  0x00007fc5e18c90d2 in mozilla::dom::Document::TopLayerPop(mozilla::dom::Element&) (this=0x7fc5da50d000, aElement=...) at /home/mirko/work/code/gecko/dom/base/Document.cpp:14831
#3  mozilla::dom::Document::TopLayerPush(mozilla::dom::Element&) (this=0x7fc5ed95fa60 <_IO_stdfile_2_lock>, aElement=...) at /home/mirko/work/code/gecko/dom/base/Document.cpp:14723
#4  0x00007fc5e2f847c6 in nsGenericHTMLElement::ShowPopover(mozilla::ErrorResult&) (this=0x7fc5d372a310, aRv=<optimized out>) at /home/mirko/work/code/gecko/dom/html/nsGenericHTMLElement.cpp:3391
#5  0x00007fc5e2872288 in mozilla::dom::HTMLElement_Binding::showPopover(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&)Traceback (most recent call last):

It's first violated by sub-test "Changing a popover from auto to manual (via attr), and then auto during 'beforetoggle' works":

 0:08.50 pid:138439 console.log: "TEST START" "Changing a popover from auto to manual (via attr), and then auto during 'beforetoggle' works"
 0:08.50 pid:138439 console.debug: "TEST STEP" "Changing a popover from auto to manual (via attr), and then auto during 'beforetoggle' works"
 0:08.50 pid:138439 Assertion failure: element->IsInComposedDoc() (Top layer element should be in doc), at /home/mirko/work/code/gecko/dom/base/Document.cpp:14947

The corresponding JS call is from the first call of popover.showPopover() (https://searchfox.org/mozilla-central/rev/0d9d9d644b06d039e119242f6f12af21d763e4eb/testing/web-platform/tests/html/semantics/popovers/popover-attribute-basic.html#305):

mozilla::dom::Document::GetTopLayerTop (this=this@entry=0x7fc5da50d000) at /home/mirko/work/code/gecko/dom/base/Document.cpp:14946
14946	  MOZ_ASSERT(element->IsInComposedDoc(),
Traceback (most recent call last):
  File "/home/mirko/work/code/gecko/js/src/gdb/mozilla/asmjs.py", line 33, in on_stop
    sigaction_fn(SIGSEGV, 0, buf)
gdb.error: Cannot call functions in reverse mode.
(rr) call DumpJSStack()
0 window.onload/</</</</<(t = "[object Object]") ["http://web-platform.test:8000/html/semantics/popovers/popover-attribute-basic.html":305:20]
1 Test.prototype.step(func = [function], this_obj = "[object Object]", "[object Object]") ["http://web-platform.test:8000/resources/testharness.js":2599:24]
    this = [object Object]

The same assertion isn't violated by the previous test:

 0:08.50 pid:138439 console.log: "TEST START" "Changing a popover from auto to auto (via attr), and then undefined during 'beforetoggle' works"
 0:08.50 pid:138439 console.debug: "TEST STEP" "Changing a popover from auto to auto (via attr), and then undefined during 'beforetoggle' works"
 0:08.50 pid:138439 console.debug: "ASSERT" "assert_true" "Changing a popover from auto to auto (via attr), and then undefined during 'beforetoggle' works" [true]
 0:08.50 pid:138439 console.debug: "ASSERT" "assert_true" "Changing a popover from auto to auto (via attr), and then undefined during 'beforetoggle' works" [true, "popover should remain open when not cha
 0:08.50 pid:138439 console.debug: "ASSERT" "assert_false" "Changing a popover from auto to auto (via attr), and then undefined during 'beforetoggle' works" [false]
 0:08.50 pid:138439 console.debug: "TEST STEP" "Changing a popover from auto to auto (via attr), and then undefined during 'beforetoggle' works"
 0:08.50 pid:138439 console.log: "TEST DONE" 0 "Changing a popover from auto to auto (via attr), and then undefined during 'beforetoggle' works"
 0:08.50 pid:138439 console.log: "TEST START" "Changing a popover from auto to manual (via attr), and then auto during 'beforetoggle' works"

Need to learn the code around Document::GetTopLayerTop.

Comment 2

[Mirko Brodesser (:mbrodesser-Igalia)]

The assertion is not violated when the sub-test "Changing a popover from auto to manual (via attr), and then auto during 'beforetoggle' works" is executed as the first test. That indicates, a previous test didn't clean up.

Popover elements are removed from the top layer only in Document::HidePopover (https://searchfox.org/mozilla-central/rev/f4d3fe187cf7dffa4c13b354bbde9bc47b5ccd3f/dom/base/Document.cpp#15064). That removal doesn't happen, when the method returns early.

Comment 3

[Mirko Brodesser (:mbrodesser-Igalia)]

Indeed the sub-test "Changing a popover from auto to auto (via attr), and then undefined during 'beforetoggle' works" doesn't remove the popover element from the top layer.

Comment 4

[Mirko Brodesser (:mbrodesser-Igalia)]

The sub-test running before the one mentioned in c3, "Changing a popover from auto to auto (via attr), and then null during 'beforetoggle'", removes the element from the top layer. Need to understand why.

Comment 5

[Mirko Brodesser (:mbrodesser-Igalia)]

Because of https://searchfox.org/mozilla-central/rev/f4d3fe187cf7dffa4c13b354bbde9bc47b5ccd3f/testing/web-platform/tests/html/semantics/popovers/popover-attribute-basic.html#285-286,288, SetAttr and UnsetAttr must behave differently in this case.

Comment 6

[Mirko Brodesser (:mbrodesser-Igalia)]

Investigation to be continued. https://github.com/whatwg/html/issues/9161 might be related.

Comment 7

[Mirko Brodesser (:mbrodesser-Igalia)]

Minimal example which doesn't remove the element from the top layer: https://jsfiddle.net/uv8r39ef/.
Can be checked by adding the assertion:

MOZ_ASSERT(!OwnerDoc()->TopLayerContains(*this));

to https://searchfox.org/mozilla-central/rev/0c2945ad4769e2d4428c72e6ddd78d60eb920394/dom/html/nsGenericHTMLElement.cpp#720.

Comment 8

[Mirko Brodesser (:mbrodesser-Igalia)]

Attachment: Bug 1834972: when removing the popover attribute, remove the corresponding element from the top layer after hiding the popover. r=emilio

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3e9395255b3b
when removing the popover attribute, remove the corresponding element from the top layer after hiding the popover. r=emilio

Comment 10

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/3e9395255b3b