Closed Bug 1411138 Opened 3 years ago Closed 3 years ago

Assertion failure: !OwnerDoc()->IsScrollingElement(this) (How can we have a scrollframe if we're the scrollingElement for our document?), at /builds/worker/workspace/build/src/dom/base/Element.cpp:700

Categories

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

55 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: jkratzer, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(4 files)

Attached file trigger.html
Testcase found while fuzzing mozilla-central rev 0bd9b61304e2.
Flags: in-testsuite?
Attached file log_minidump.txt
Attached file log_stderr.txt
Version: unspecified → 58 Branch
INFO: Last good revision: 9883fe74207112ee8451d127a834835f2ac28604
INFO: First bad revision: dd1c5aecc373a8bbba824c445a4a8c4e127adda0
INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9883fe74207112ee8451d127a834835f2ac28604&tochange=dd1c5aecc373a8bbba824c445a4a8c4e127adda0

So all the way back to when the assert got added.
Blocks: 1364360
Has Regression Range: --- → yes
Flags: needinfo?(bzbarsky)
Version: 58 Branch → 55 Branch
So https://drafts.csswg.org/cssom-view/#dom-document-scrollingelement is defined in terms of https://drafts.csswg.org/cssom-view/#the-html-body-element which is the first <body> child of <html>.

Our scroll propagation is implemented in terms of nsHTMLDocument::GetBody which implements https://html.spec.whatwg.org/multipage/dom.html#dom-document-body which is defined in terms of https://html.spec.whatwg.org/multipage/dom.html#the-body-element-2

This mismatch is what's causing the assertion: We don't propagate scroll, but think we're the scrollingElement.

https://www.w3.org/TR/CSS21/visufx.html#overflow-clipping says:

   UAs must apply the 'overflow' property set on the root element to the viewport.
   When the root element is an HTML "HTML" element or an XHTML "html" element, and
   that element has an HTML "BODY" element or an XHTML "body" element as a child,
   user agents must instead apply the 'overflow' property from the first such child
   element to the viewport, if the value on the root element is 'visible'.

which suggests to me that scroll propagation should also use nsIDocument::GetBodyElement() instead of nsHTMLDocument::GetBody...
Looks like Safari does the scroll propagation sort of right and no one else does.  I also filed https://github.com/w3c/csswg-drafts/issues/1905 on the css-overflow draft having a worse definition than CSS2.1...
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Flags: needinfo?(bzbarsky)
Comment on attachment 8921971 [details] [diff] [review]
Be consistent (and follow the spec) in terms of how we determine the element that propagates styles to the viewport

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

::: testing/web-platform/meta/MANIFEST.json
@@ +417141,5 @@
>     "d2c5d8d1696112b771a332011c4f33065817ed9a",
>     "testharness"
>    ],
>    "XMLHttpRequest/open-url-encoding.htm": [
> +   "a36c7b0e5919af7842883582ef9fc415d8f7ef25",

FWIW: all these unrelated hash changes in the manifest are triggering patch-rejection for me, when I try to apply the patch -- it seems they've already been made on current mozilla-inbound, over in https://hg.mozilla.org/integration/mozilla-inbound/rev/f95a678186b4 (bug 1410245)
Comment on attachment 8921971 [details] [diff] [review]
Be consistent (and follow the spec) in terms of how we determine the element that propagates styles to the viewport

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

r=me, but I've got one observation/suggestion for the reference case:

::: testing/web-platform/tests/css/CSS2/visufx/support/overflow-propagation-001-ref.html
@@ +5,5 @@
> +    document.body.remove();
> +    var b = document.createElement("body");
> +    b.style = "overflow: hidden; margin: 100px; width: 100px; height: 100px; border: 1px solid green; position: absolute; top: 0; left: 0";
> +    b.textContent = "The body should have visible overflow of the text that totally doesn't fit in the little box.";
> +    document.documentElement.appendChild(b);

This seems a bit complex/scripted, for a reference case. (and unnecessarily so, IIUC)

Perhaps this should just be static HTML, without 'overflow' set on the <body> at all?  (Something close to overflow-propagation-001a.html, but with 'overflow' set directly on <html>)

  <!DOCTYPE html>
  <meta charset=utf-8>
  <html style="overflow:hidden">
    <body style="margin: 100px; width: 100px; height: 100px;
                 border: 1px solid green; position: absolute;
                 top: 0; left: 0">
      The body should have visible overflow of the
      text that totally doesn't fit in the little box.
    </body>
  </html>
Attachment #8921971 - Flags: review?(dholbert) → review+
> This seems a bit complex/scripted, for a reference case.

Er.. that's just wrong.  the actual reference should be:

  <!doctype html>
  <html style="overflow: hidden">
  <meta charset=utf-8>
  <body style="margin: 100px; width: 100px; height: 100px; border: 1px solid green; position: absolute; top: 0; left: 0">
      The body should have visible overflow of the text that totally doesn't fit
      in the little box.
  </body>
Thanks! Yeah, that's what I had in mind.
Fwiw, my try run where I purposefully broke scroll propagation from <body> caught that problem too, by not failing the new reftests... ;)
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1fb8f188a56
Be consistent (and follow the spec) in terms of how we determine the element that propagates styles to the viewport.  r=dholbert
https://hg.mozilla.org/mozilla-central/rev/c1fb8f188a56
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
See Also: → 1415373
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.